[PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[Sherry Sun]

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@nxp.com>
---
 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;
 	serport->tty = tty;
 
 	if (!tty->ops->open || !tty->ops->close) {
-- 
2.17.1

Re: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[Greg KH]

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@nxp.com>
> ---
>  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?

And why are you saving off a reference counted pointer without
incrementing the reference to the pointer?

thanks,

greg k-h

RE: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[Sherry Sun]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2023年3月15日 15:40
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: jirislaby@kernel.org; robh@kernel.org; linux-serial@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> 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@nxp.com>
> > ---
> >  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);

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.

Best Regards
Sherry

Re: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[Greg KH]

On Wed, Mar 15, 2023 at 09:49:53AM +0000, Sherry Sun wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: 2023年3月15日 15:40
> > To: Sherry Sun <sherry.sun@nxp.com>
> > Cc: jirislaby@kernel.org; robh@kernel.org; linux-serial@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > 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@nxp.com>
> > > ---
> > >  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.

greg k-h

RE: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

[Sherry Sun]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2023年3月15日 19:33
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: jirislaby@kernel.org; robh@kernel.org; linux-serial@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> 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@linuxfoundation.org>
> > > Sent: 2023年3月15日 15:40
> > > To: Sherry Sun <sherry.sun@nxp.com>
> > > Cc: jirislaby@kernel.org; robh@kernel.org;
> > > linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>
> > > 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@nxp.com>
> > > > ---
> > > >  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