All of lore.kernel.org
 help / color / mirror / Atom feed
* [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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ messages in thread
From: Sherry Sun @ 2023-03-15 13:37 UTC (permalink / raw)
  To: Greg KH; +Cc: jirislaby, robh, linux-serial, linux-kernel, 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] 5+ 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; 5+ 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] 5+ messages in thread
* Re: [PATCH] tty: serdev: serdev-ttyport: add devt for ctrl->dev
@ 2023-03-15 19:12 kernel test robot
  0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2023-03-15 19:12 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230315105400.23426-1-sherry.sun@nxp.com>
References: <20230315105400.23426-1-sherry.sun@nxp.com>
TO: Sherry Sun <sherry.sun@nxp.com>
TO: gregkh@linuxfoundation.org
TO: jirislaby@kernel.org
TO: robh@kernel.org
CC: linux-serial@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-imx@nxp.com

Hi Sherry,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.3-rc2 next-20230315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
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
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
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  263  
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;
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)
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  }
bed35c6dfa6a36 Rob Herring  2017-02-02  305  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-20  5:55 UTC | newest]

Thread overview: 5+ 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
2023-03-15 19:12 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.