Re: [PATCH v2 1/1] serial: core: Start managing serial controllers to enable runtime PM

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> [220615 09:12]:
> On Wed, Jun 15, 2022 at 09:24:55AM +0300, Tony Lindgren wrote:
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -30,6 +32,25 @@
> >  #include <linux/irq.h>
> >  #include <linux/uaccess.h>
> >  
> > +/*
> > + * Serial port device specific data for serial core.
> > + *
> > + * Each port device can have multiple ports with struct uart_state allocated
> > + * for each port. The array of ports is kept in struct uart_driver.
> > + */
> > +struct serial_controller {
> > +	struct device *dev;			/* Serial port device */
> 
> Serial port device is a bit unclear for non-prepared reader. Perhaps add
> the word "physical" or another to specify the nature of the device (because
> to me "serial port device" sounds like a duplication of something in struct
> uart_port, but I have doubts).

Hmm so we could add a list of all the registered struct uart_port or
uart_state to struct serial_controller. Then looking up struct device would
be just looking at the first list entry. We need to take port_mutex, but
that should be mostly when the device does runtime PM. We'll be needing that
list anyways later on to flush pending TX on runtime PM resume for each
port associated with the device.

> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -250,6 +250,7 @@ struct uart_port {
> >  	unsigned char		hub6;			/* this should be in the 8250 driver */
> >  	unsigned char		suspended;
> >  	unsigned char		console_reinit;
> > +	unsigned char		supports_autosuspend;
> >  	const char		*name;			/* port name */
> >  	struct attribute_group	*attr_group;		/* port specific attributes */
> >  	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
> > @@ -285,6 +286,8 @@ enum uart_pm_state {
> >   * This is the state information which is persistent across opens.
> >   */
> >  struct uart_state {
> > +	struct serial_controller *controller;
> 
> While good looking here, I believe resource wise is better to leave @port to be
> the first member. The rationale is to get rid of pointer arithmetics at compile
> time (and I believe the port is used much more and in more critical places).
> However, I dunno if it will get a lot of benefit, would be nice to see
> bloat-o-meter output for your variant and my proposal.

OK makes sense. And thanks for reviewing again :)

Regards,

Tony