From: Greg KH <gregkh@linuxfoundation.org>
To: Sherry Sun <sherry.sun@nxp.com>
Cc: "jirislaby@kernel.org" <jirislaby@kernel.org>,
"robh@kernel.org" <robh@kernel.org>,
"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
"linux-kernel@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
Date: Wed, 15 Mar 2023 12:33:18 +0100 [thread overview]
Message-ID: <ZBGs/sWZGnIbqHbd@kroah.com> (raw)
In-Reply-To: <AS8PR04MB840408DC92F4001AB440353292BF9@AS8PR04MB8404.eurprd04.prod.outlook.com>
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
next prev parent reply other threads:[~2023-03-15 11:34 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 7:21 [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework Sherry Sun
2023-03-15 7:39 ` Greg KH
2023-03-15 9:49 ` Sherry Sun
2023-03-15 11:33 ` Greg KH [this message]
2023-03-16 5:05 ` Sherry Sun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZBGs/sWZGnIbqHbd@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=robh@kernel.org \
--cc=sherry.sun@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).