* [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev
@ 2023-03-15 10:54 Sherry Sun
2023-03-15 11:10 ` Greg KH
2023-03-20 5:55 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Sherry Sun @ 2023-03-15 10:54 UTC (permalink / raw)
To: gregkh, jirislaby, robh; +Cc: linux-serial, linux-kernel, linux-imx
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 <sherry.sun@nxp.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev 2023-03-15 10:54 [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev Sherry Sun @ 2023-03-15 11:10 ` Greg KH 2023-03-15 13:37 ` Sherry Sun 2023-03-20 5:55 ` Dan Carpenter 1 sibling, 1 reply; 4+ messages in thread From: Greg KH @ 2023-03-15 11:10 UTC (permalink / raw) To: Sherry Sun; +Cc: jirislaby, robh, linux-serial, linux-kernel, linux-imx 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 <sherry.sun@nxp.com> > --- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev 2023-03-15 11:10 ` Greg KH @ 2023-03-15 13:37 ` Sherry Sun 0 siblings, 0 replies; 4+ messages in thread From: Sherry Sun @ 2023-03-15 13:37 UTC (permalink / raw) To: Greg KH Cc: jirislaby@kernel.org, robh@kernel.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, dl-linux-imx > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: 2023年3月15日 19:10 > 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: 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 <sherry.sun@nxp.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev 2023-03-15 10:54 [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev Sherry Sun 2023-03-15 11:10 ` Greg KH @ 2023-03-20 5:55 ` Dan Carpenter 1 sibling, 0 replies; 4+ messages in thread From: Dan Carpenter @ 2023-03-20 5:55 UTC (permalink / raw) To: oe-kbuild, Sherry Sun, gregkh, jirislaby, robh Cc: lkp, oe-kbuild-all, linux-serial, linux-kernel, linux-imx 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/202303160201.bnmM2caW-lkp@intel.com/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 <lkp@intel.com> | Reported-by: Dan Carpenter <error27@gmail.com> | Link: https://lore.kernel.org/r/202303160201.bnmM2caW-lkp@intel.com/ 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-20 5:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-15 10:54 [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev Sherry Sun 2023-03-15 11:10 ` Greg KH 2023-03-15 13:37 ` Sherry Sun 2023-03-20 5:55 ` Dan Carpenter
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).