> -----Original Message----- > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > Sent: 2023年3月15日 19:10 > To: Sherry Sun <sherry.sun@xxxxxxx> > Cc: jirislaby@xxxxxxxxxx; robh@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > 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@xxxxxxx> > > --- > > 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