For serdev framework, the serdev_controller device is the tty device,
which is also the child device of the uart_port device. If we don't set
devt property for ctrl->dev, device_find_child(uport->dev, ...) may
always return NULL in uart_suspend_port() function, which prevents us
from properly handling uart port suspend, so fix it here.
Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
Signed-off-by: Sherry Sun <[email protected]>
---
drivers/tty/serdev/serdev-ttyport.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index bba37ab90215..c58af8141380 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -268,6 +268,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
{
struct serdev_controller *ctrl;
struct serport *serport;
+ dev_t devt = MKDEV(drv->major, drv->minor_start) + idx;
int ret;
if (!port || !drv || !parent)
@@ -282,6 +283,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
serport->tty_idx = idx;
serport->tty_drv = drv;
+ ctrl->dev.devt = devt;
ctrl->ops = &ctrl_ops;
port->client_ops = &client_ops;
--
2.17.1
On Wed, Mar 15, 2023 at 06:54:00PM +0800, Sherry Sun wrote:
> For serdev framework, the serdev_controller device is the tty device,
> which is also the child device of the uart_port device. If we don't set
> devt property for ctrl->dev, device_find_child(uport->dev, ...) may
> always return NULL in uart_suspend_port() function, which prevents us
> from properly handling uart port suspend, so fix it here.
>
> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
> Signed-off-by: Sherry Sun <[email protected]>
> ---
> drivers/tty/serdev/serdev-ttyport.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bba37ab90215..c58af8141380 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -268,6 +268,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
> {
> struct serdev_controller *ctrl;
> struct serport *serport;
> + dev_t devt = MKDEV(drv->major, drv->minor_start) + idx;
> int ret;
>
> if (!port || !drv || !parent)
> @@ -282,6 +283,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
> serport->tty_idx = idx;
> serport->tty_drv = drv;
>
> + ctrl->dev.devt = devt;
This feels wrong as you can't just create a magic dev_t out of no where
and expect it to be handled properly. Where now is this dev_t exposed?
Something else feels wrong here, sorry, I do not think this is correct.
thanks,
greg k-h
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: 2023??3??15?? 19:10
> To: Sherry Sun <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev
>
> On Wed, Mar 15, 2023 at 06:54:00PM +0800, Sherry Sun wrote:
> > For serdev framework, the serdev_controller device is the tty device,
> > which is also the child device of the uart_port device. If we don't
> > set devt property for ctrl->dev, device_find_child(uport->dev, ...)
> > may always return NULL in uart_suspend_port() function, which prevents
> > us from properly handling uart port suspend, so fix it here.
> >
> > Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
> > Signed-off-by: Sherry Sun <[email protected]>
> > ---
> > drivers/tty/serdev/serdev-ttyport.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/tty/serdev/serdev-ttyport.c
> > b/drivers/tty/serdev/serdev-ttyport.c
> > index bba37ab90215..c58af8141380 100644
> > --- a/drivers/tty/serdev/serdev-ttyport.c
> > +++ b/drivers/tty/serdev/serdev-ttyport.c
> > @@ -268,6 +268,7 @@ struct device *serdev_tty_port_register(struct
> > tty_port *port, {
> > struct serdev_controller *ctrl;
> > struct serport *serport;
> > + dev_t devt = MKDEV(drv->major, drv->minor_start) + idx;
> > int ret;
> >
> > if (!port || !drv || !parent)
> > @@ -282,6 +283,7 @@ struct device *serdev_tty_port_register(struct
> tty_port *port,
> > serport->tty_idx = idx;
> > serport->tty_drv = drv;
> >
> > + ctrl->dev.devt = devt;
>
> This feels wrong as you can't just create a magic dev_t out of no where and
> expect it to be handled properly. Where now is this dev_t exposed?
>
Hi Greg,
Now dev_t is the key point to get the tty->dev in uart_suspend_port()/uart_resume_port() and alloc_tty_struct(), it is set in tty_register_device_attr() but not in serdev_tty_port_register().
To be frank, I am also not sure if this is the right way to fix the issue, maybe we can have more discussion here regarding how to get the correct tty_dev in uart_suspend_port().
At least with the code logic in serdev framework we are using now, we will always get NULL tty_dev from device_find_child(). I believe this is not what we want here.
static int serial_match_port(struct device *dev, void *data)
{
struct uart_match *match = data;
struct tty_driver *tty_drv = match->driver->tty_driver;
dev_t devt = MKDEV(tty_drv->major, tty_drv->minor_start) +
match->port->line;
return dev->devt == devt; /* Actually, only one tty per port */
}
int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
{
struct uart_state *state = drv->state + uport->line;
struct tty_port *port = &state->port;
struct device *tty_dev;
struct uart_match match = {uport, drv};
mutex_lock(&port->mutex);
tty_dev = device_find_child(uport->dev, &match, serial_match_port);
if (uport->line == 2)
if (tty_dev && device_may_wakeup(tty_dev)) {
enable_irq_wake(uport->irq);
put_device(tty_dev);
mutex_unlock(&port->mutex);
return 0;
}
put_device(tty_dev);
...
Best Regards
Sherry
> Something else feels wrong here, sorry, I do not think this is correct.
>
> thanks,
>
> greg k-h
Hi Sherry,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sherry-Sun/tty-serdev-serdev-ttyport-add-devt-for-ctrl-dev/20230315-185913
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230315105400.23426-1-sherry.sun%40nxp.com
patch subject: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20230316/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/tty/serdev/serdev-ttyport.c:273 serdev_tty_port_register() warn: variable dereferenced before check 'drv' (see line 270)
vim +/drv +273 drivers/tty/serdev/serdev-ttyport.c
bed35c6dfa6a36 Rob Herring 2017-02-02 264 struct device *serdev_tty_port_register(struct tty_port *port,
bed35c6dfa6a36 Rob Herring 2017-02-02 265 struct device *parent,
bed35c6dfa6a36 Rob Herring 2017-02-02 266 struct tty_driver *drv, int idx)
bed35c6dfa6a36 Rob Herring 2017-02-02 267 {
bed35c6dfa6a36 Rob Herring 2017-02-02 268 struct serdev_controller *ctrl;
bed35c6dfa6a36 Rob Herring 2017-02-02 269 struct serport *serport;
225acc66b125b9 Sherry Sun 2023-03-15 @270 dev_t devt = MKDEV(drv->major, drv->minor_start) + idx;
^^^^^ ^^^^^
Dereferences.
bed35c6dfa6a36 Rob Herring 2017-02-02 271 int ret;
bed35c6dfa6a36 Rob Herring 2017-02-02 272
bed35c6dfa6a36 Rob Herring 2017-02-02 @273 if (!port || !drv || !parent)
^^^^
Checked too late. Are these checks really required?
bed35c6dfa6a36 Rob Herring 2017-02-02 274 return ERR_PTR(-ENODEV);
bed35c6dfa6a36 Rob Herring 2017-02-02 275
bed35c6dfa6a36 Rob Herring 2017-02-02 276 ctrl = serdev_controller_alloc(parent, sizeof(struct serport));
bed35c6dfa6a36 Rob Herring 2017-02-02 277 if (!ctrl)
bed35c6dfa6a36 Rob Herring 2017-02-02 278 return ERR_PTR(-ENOMEM);
bed35c6dfa6a36 Rob Herring 2017-02-02 279 serport = serdev_controller_get_drvdata(ctrl);
bed35c6dfa6a36 Rob Herring 2017-02-02 280
bed35c6dfa6a36 Rob Herring 2017-02-02 281 serport->port = port;
bed35c6dfa6a36 Rob Herring 2017-02-02 282 serport->tty_idx = idx;
bed35c6dfa6a36 Rob Herring 2017-02-02 283 serport->tty_drv = drv;
bed35c6dfa6a36 Rob Herring 2017-02-02 284
225acc66b125b9 Sherry Sun 2023-03-15 285 ctrl->dev.devt = devt;
bed35c6dfa6a36 Rob Herring 2017-02-02 286 ctrl->ops = &ctrl_ops;
bed35c6dfa6a36 Rob Herring 2017-02-02 287
aee5da7838787f Johan Hovold 2017-04-11 288 port->client_ops = &client_ops;
aee5da7838787f Johan Hovold 2017-04-11 289 port->client_data = ctrl;
aee5da7838787f Johan Hovold 2017-04-11 290
bed35c6dfa6a36 Rob Herring 2017-02-02 291 ret = serdev_controller_add(ctrl);
bed35c6dfa6a36 Rob Herring 2017-02-02 292 if (ret)
aee5da7838787f Johan Hovold 2017-04-11 293 goto err_reset_data;
bed35c6dfa6a36 Rob Herring 2017-02-02 294
bed35c6dfa6a36 Rob Herring 2017-02-02 295 dev_info(&ctrl->dev, "tty port %s%d registered\n", drv->name, idx);
bed35c6dfa6a36 Rob Herring 2017-02-02 296 return &ctrl->dev;
bed35c6dfa6a36 Rob Herring 2017-02-02 297
aee5da7838787f Johan Hovold 2017-04-11 298 err_reset_data:
aee5da7838787f Johan Hovold 2017-04-11 299 port->client_data = NULL;
0c5aae59270fb1 Johan Hovold 2020-02-10 300 port->client_ops = &tty_port_default_client_ops;
bed35c6dfa6a36 Rob Herring 2017-02-02 301 serdev_controller_put(ctrl);
aee5da7838787f Johan Hovold 2017-04-11 302
bed35c6dfa6a36 Rob Herring 2017-02-02 303 return ERR_PTR(ret);
bed35c6dfa6a36 Rob Herring 2017-02-02 304 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests