> -----Original Message----- > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > Sent: 2023年3月15日 19:33 > To: Sherry Sun <sherry.sun@xxxxxxx> > Cc: jirislaby@xxxxxxxxxx; robh@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for > serdev framework > > On Wed, Mar 15, 2023 at 09:49:53AM +0000, Sherry Sun wrote: > > > > > > > -----Original Message----- > > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > > > Sent: 2023年3月15日 15:40 > > > To: Sherry Sun <sherry.sun@xxxxxxx> > > > Cc: jirislaby@xxxxxxxxxx; robh@xxxxxxxxxx; > > > linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > > dl-linux-imx <linux-imx@xxxxxxx> > > > Subject: Re: [PATCH] tty: serdev: serdev-ttyport: set correct > > > tty->dev for serdev framework > > > > > > On Wed, Mar 15, 2023 at 03:21:43PM +0800, Sherry Sun wrote: > > > > ttyport_open() calls tty_init_dev() to initialize a tty device, > > > > but > > > > tty_get_device() cannot get the correct tty->dev for serdev tty in > > > > alloc_tty_struct(), because serdev framework does not set > > > > tty_class, so class_find_device_by_devt(tty_class, devt) may always > return NULL. > > > > > > > > For serdev framework, we need to assign the correct ctrl->dev to > > > > tty->dev. > > > > > > > > Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver") > > > > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx> > > > > --- > > > > drivers/tty/serdev/serdev-ttyport.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c > > > > b/drivers/tty/serdev/serdev-ttyport.c > > > > index d367803e2044..bba37ab90215 100644 > > > > --- a/drivers/tty/serdev/serdev-ttyport.c > > > > +++ b/drivers/tty/serdev/serdev-ttyport.c > > > > @@ -112,6 +112,7 @@ static int ttyport_open(struct > > > > serdev_controller > > > *ctrl) > > > > tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > > > > if (IS_ERR(tty)) > > > > return PTR_ERR(tty); > > > > + tty->dev = &ctrl->dev; > > > > > > What in-kernel driver needs this change? How has it not been a > > > problem so far? > > > > > > > Hi Greg, I searched the users of tty->dev under serial floder, found the > following drivers need it. > > drivers/tty/serial/stm32-usart.c:780: pm_wakeup_event(tport- > >tty->dev, 0); > > drivers/tty/serial/fsl_lpuart.c:3018: tty_dev = tty->dev; > > drivers/tty/serial/st-asc.c:266: pm_wakeup_event(tport->tty->dev, > 0); > > > > Actually this issue was found when I tested the nxp Bluetooth driver which > use serdev framework along with fsl_lpuart.c driver, when system is > suspending, the following NULL pointer kernel panic is observed. > > This is because lpuart driver will check the device_may_wakeup(tty->dev) > to determine if wakeup register bits need to be enabled or not before > suspend, it works well the the ldisc tty, but since serdev tty doesn't set > correct tty->dev, so here cause the NULL pointer panic. > > > > root@imx8ulpevk:~# echo mem > /sys/power/state > > [ 42.657779] PM: suspend entry (deep) > > [ 42.664333] Filesystems sync: 0.002 seconds > > [ 42.717624] Freezing user space processes ... (elapsed 0.001 seconds) > done. > > [ 42.727063] OOM killer disabled. > > [ 42.730383] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) > done. > > [ 42.753652] fec 29950000.ethernet eth0: Link is Down > > [ 42.780681] Unable to handle kernel NULL pointer dereference at virtual > address 00000000000000dc > > [ 42.789603] Mem abort info: > > [ 42.792430] ESR = 0x0000000096000004 > > [ 42.796242] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 42.801661] SET = 0, FnV = 0 > > ...... > > > > > And why are you saving off a reference counted pointer without > > > incrementing the reference to the pointer? > > > > Sorry, forgive me I am not clearly understand the requirement here, do you > mean we need to add the following changes? > > get_device(&ctrl->dev); > > tty->dev = &ctrl->dev; > > put_device((&ctrl->dev); > > Ick, no, only do put_device() when you are finished with the pointer and are > not going to access it anymore. > > > And per my understanding, the reference count needs to be increased and > decreased from the user side, here we only do a initialization for the tty->dev. > > Then something is not set up properly here, sorry. Hi Greg, Thanks for your comments, but what I want to say here is that the caller of tty->dev will care about the reference count, here we only initialize the tty->dev pointer, like what we do in alloc_tty_struct(), seems no need the reference count. struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx) { ... tty->dev = tty_get_device(tty); return tty; } For the caller of tty->dev, such as uart_suspend_port(), it will call get_device() in device_find_child() before get the tty->dev, and will call put_device() when when it is no longer accessed. The caller will handle this like what we do for the original tty framework. What do you think? 2314 int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) 2315 { ... 2323 tty_dev = device_find_child(uport->dev, &match, serial_match_port); 2324 if (tty_dev && device_may_wakeup(tty_dev)) { 2325 enable_irq_wake(uport->irq); 2326 put_device(tty_dev); 2327 mutex_unlock(&port->mutex); 2328 return 0; 2329 } 2330 put_device(tty_dev); Best Regards Sherry