[v9,1/1] serial: core: Start managing serial controllers to enable runtime PM

We want to enable runtime PM for serial port device drivers in a generic
way. To do this, we want to have the serial core layer manage the
registered physical serial controller devices.

To do this, let's set up a struct bus and struct device for the serial
core controller as suggested by Greg and Jiri. The serial core controller
devices are children of the physical serial port device. The serial core
controller device is needed to support multiple different kind of ports
connected to single physical serial port device.

Let's also set up a struct device for the serial core port. The serial
core port instances are children of the serial core controller device.

With the serial core port device we can now flush pending TX on the
runtime PM resume as suggested by Johan.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Suggested-by: Johan Hovold <johan@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
Changes since v8:

- Drop unnecessary free for name noticed by Andy, the name is freed
  on put_device()

- Cosmetic fix for comments in serial_port.c noted by Andy

- Spelling fix for word uninitialized in serial_base_get_port()

Changes since v7:

- Add release() put_device() to serial_base.c as noted by Andy

- Make struct serial_base_device private to serial_base.c by adding
  serial_base_get_port()

- Add more comments to __uart_start()

- Coding style improvments for serial_base.c from Andy

Changes since v6:

- Fix up a memory leak and a bunch of issues in serial_base.c as noted
  by Andy

- Replace bool with new_ctrl_dev for freeing the added device on
  error path

- Drop unused pm ops for serial_ctrl.c as noted by kernel test robot

- Drop documentation updates for acpi devices for now to avoid a merge
  conflict and make testing easier between -rc2 and Linux next

Changes since v5:

- Replace platform bus and device with bus_add() and device_add(),
  Greg did not like platform bus and device here. This also gets
  rid of the need for platform data with struct serial_base_device,
  see new file serial_base.c

- Update documentation to drop reference to struct uart_device as
  suggested by Andy

Changes since v4:

- Fix issue noted by Ilpo by calling serial_core_add_one_port() after
  the devices are created

Changes since v3:

- Simplify things by adding a serial core control device as the child of
  the physical serial port as suggested by Jiri

- Drop the tinkering of the physical serial port device for runtime PM.
  Serial core just needs to manage port->port_dev with the addition of
  the serial core control device and the device hierarchy will keep the
  pysical serial port device enabled as needed

- Simplify patch description with all the runtime PM tinkering gone

- Coding style improvments as noted by Andy

- Post as a single RFC patch as we're close to the merge window

Changes since v2:

- Make each serial port a proper device as suggested by Greg. This is
  a separate patch that flushes the TX on runtime PM resume

Changes since v1:

- Use kref as suggested by Andy

- Fix memory leak on error as noted by Andy

- Use use unsigned char for supports_autosuspend as suggested by Andy

- Coding style improvments as suggested by Andy

---
 drivers/tty/serial/8250/8250_core.c |   1 +
 drivers/tty/serial/Makefile         |   2 +-
 drivers/tty/serial/serial_base.c    | 142 ++++++++++++++++++++++++
 drivers/tty/serial/serial_base.h    |  21 ++++
 drivers/tty/serial/serial_core.c    | 160 ++++++++++++++++++++++++++--
 drivers/tty/serial/serial_ctrl.c    |  71 ++++++++++++
 drivers/tty/serial/serial_port.c    | 102 ++++++++++++++++++
 include/linux/serial_core.h         |   4 +-
 8 files changed, 494 insertions(+), 9 deletions(-)
 create mode 100644 drivers/tty/serial/serial_base.c
 create mode 100644 drivers/tty/serial/serial_base.h
 create mode 100644 drivers/tty/serial/serial_ctrl.c
 create mode 100644 drivers/tty/serial/serial_port.c

On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Given that the driver core is "GPL-2.0-only", why is this -or-later?

Sorry, I have to ask.


> +
> +/*
> + * Serial core base layer for controllers
> + *
> + * The serial core bus manages the serial core controller instances.

No copyright notice?  That's fine, but again, I have to ask.

thanks,

greg k-h

On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> +obj-$(CONFIG_SERIAL_CORE) += serial_base.o serial_core.o serial_ctrl.o serial_port.o

Why is this 3 new modules and not just all go into serial_base?  What's
going to auto-load the other modules you created here?  Feels like this
should all end up in the same .ko as they all depend on each other,
right?

>  
>  obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
>  obj-$(CONFIG_SERIAL_EARLYCON_SEMIHOST) += earlycon-semihost.o
> diff --git a/drivers/tty/serial/serial_base.c b/drivers/tty/serial/serial_base.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Serial core base layer for controllers
> + *
> + * The serial core bus manages the serial core controller instances.
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +struct serial_base_device {
> +	struct device dev;
> +	struct uart_port *port;
> +};
> +
> +#define to_serial_base_device(d) container_of((d), struct serial_base_device, dev)
> +
> +struct uart_port *serial_base_get_port(struct device *dev)
> +{
> +	struct serial_base_device *sbd;
> +
> +	if (!dev)
> +		return NULL;
> +
> +	sbd = to_serial_base_device(dev);
> +
> +	/* Check in case serial_core_add_one_port() happened to fail */
> +	if (!sbd->port->state) {

This is odd, how can it fail and then this function be called after that
failure?

> +		dev_warn(dev, "uninitialized serial port?\n");
> +		return NULL;
> +	}
> +
> +	return sbd->port;
> +}
> +EXPORT_SYMBOL_NS(serial_base_get_port, SERIAL_BASE);
> +
> +static int serial_base_match(struct device *dev, struct device_driver *drv)
> +{
> +	int len = strlen(drv->name);
> +
> +	return !strncmp(dev_name(dev), drv->name, len);
> +}
> +
> +static struct bus_type serial_base_bus_type = {
> +	.name = "serial-base",
> +	.match = serial_base_match,
> +};
> +
> +int serial_base_driver_register(struct device_driver *driver)
> +{
> +	driver->bus = &serial_base_bus_type;
> +
> +	return driver_register(driver);
> +}
> +EXPORT_SYMBOL_NS(serial_base_driver_register, SERIAL_BASE);
> +
> +void serial_base_driver_unregister(struct device_driver *driver)
> +{
> +	driver_unregister(driver);
> +}
> +EXPORT_SYMBOL_NS(serial_base_driver_unregister, SERIAL_BASE);
> +
> +static void serial_base_release(struct device *dev)
> +{
> +	struct serial_base_device *sbd = to_serial_base_device(dev);
> +
> +	kfree(sbd);
> +}
> +
> +struct device *serial_base_device_add(struct uart_port *port, const char *name,
> +				      struct device *parent_dev)
> +{
> +	struct serial_base_device *sbd;
> +	int err, id;
> +
> +	sbd = kzalloc(sizeof(*sbd), GFP_KERNEL);
> +	if (!sbd)
> +		return NULL;
> +
> +	device_initialize(&sbd->dev);
> +	sbd->dev.parent = parent_dev;
> +	sbd->dev.bus = &serial_base_bus_type;
> +	sbd->dev.release = &serial_base_release;
> +
> +	if (str_has_prefix(name, "ctrl")) {

That's a magic string, shouldn't it be documented somewhere?

> +		id = port->ctrl_id;
> +	} else {
> +		id = port->line;
> +		sbd->port = port;
> +	}
> +
> +	err = dev_set_name(&sbd->dev, "%s.%s.%d", name, dev_name(port->dev), id);
> +	if (err)
> +		goto err_free_dev;
> +
> +	err = device_add(&sbd->dev);
> +	if (err)
> +		goto err_put_device;
> +
> +	return &sbd->dev;
> +
> +err_put_device:
> +	put_device(&sbd->dev);
> +
> +err_free_dev:
> +	kfree(sbd);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_NS(serial_base_device_add, SERIAL_BASE);
> +
> +void serial_base_device_remove(struct device *dev)
> +{
> +	if (!dev)
> +		return;
> +
> +	device_del(dev);
> +}
> +EXPORT_SYMBOL_NS(serial_base_device_remove, SERIAL_BASE);
> +
> +static int serial_base_init(void)
> +{
> +	return bus_register(&serial_base_bus_type);
> +}
> +module_init(serial_base_init);
> +
> +static void serial_base_exit(void)
> +{
> +	bus_unregister(&serial_base_bus_type);
> +}
> +module_exit(serial_base_exit);
> +
> +MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
> +MODULE_DESCRIPTION("Serial core bus");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/* Serial core related functions, serial port device drivers do not need this. */
> +
> +struct uart_driver;
> +struct uart_port;
> +struct device_driver;
> +struct device;
> +
> +int serial_base_driver_register(struct device_driver *driver);
> +void serial_base_driver_unregister(struct device_driver *driver);
> +struct device *serial_base_device_add(struct uart_port *port, const char *name,
> +				      struct device *parent_dev);
> +void serial_base_device_remove(struct device *dev);
> +struct uart_port *serial_base_get_port(struct device *dev);
> +
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
> 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
> @@ -17,6 +17,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
> @@ -31,6 +32,8 @@
>  #include <linux/irq.h>
>  #include <linux/uaccess.h>
>  
> +#include "serial_base.h"
> +
>  /*
>   * This is used to lock changes in serial line configuration.
>   */
> @@ -134,9 +137,30 @@ static void __uart_start(struct tty_struct *tty)
>  {
>  	struct uart_state *state = tty->driver_data;
>  	struct uart_port *port = state->uart_port;
> +	struct device *port_dev;
> +	int err;
> +
> +	if (!port || uart_tx_stopped(port))
> +		return;
> +
> +	port_dev = port->port_dev;
> +
> +	/* Increment the runtime PM usage count for the active check below */
> +	err = pm_runtime_get(port_dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(port_dev);
> +		return;
> +	}
>  
> -	if (port && !uart_tx_stopped(port))
> +	/*
> +	 * Start TX if enabled, and kick runtime PM. If the device is not
> +	 * enabled, serial_port_runtime_resume() calls start_tx() again
> +	 * after enabling the device.
> +	 */
> +	if (pm_runtime_active(port_dev))
>  		port->ops->start_tx(port);
> +	pm_runtime_mark_last_busy(port_dev);
> +	pm_runtime_put_autosuspend(port_dev);
>  }
>  
>  static void uart_start(struct tty_struct *tty)
> @@ -3036,7 +3060,7 @@ static const struct attribute_group tty_dev_attr_group = {
>  };
>  
>  /**
> - * uart_add_one_port - attach a driver-defined port structure
> + * serial_core_add_one_port - attach a driver-defined port structure
>   * @drv: pointer to the uart low level driver structure for this port
>   * @uport: uart port structure to use for this port.
>   *
> @@ -3046,7 +3070,7 @@ static const struct attribute_group tty_dev_attr_group = {
>   * core driver. The main purpose is to allow the low level uart drivers to
>   * expand uart_port, rather than having yet more levels of structures.
>   */
> -int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  {
>  	struct uart_state *state;
>  	struct tty_port *port;
> @@ -3136,10 +3160,9 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(uart_add_one_port);
>  
>  /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
>   * @drv: pointer to the uart low level driver structure for this port
>   * @uport: uart port structure for this port
>   *
> @@ -3148,7 +3171,8 @@ EXPORT_SYMBOL(uart_add_one_port);
>   * This unhooks (and hangs up) the specified port structure from the core
>   * driver. No further calls will be made to the low-level code for this port.
>   */
> -int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_remove_one_port(struct uart_driver *drv,
> +				       struct uart_port *uport)
>  {
>  	struct uart_state *state = drv->state + uport->line;
>  	struct tty_port *port = &state->port;
> @@ -3205,6 +3229,8 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>  	 * Indicate that there isn't a port here anymore.
>  	 */
>  	uport->type = PORT_UNKNOWN;
> +	uport->port_dev = NULL;
> +	uport->ctrl_id = -ENODEV;
>  
>  	mutex_lock(&port->mutex);
>  	WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3216,7 +3242,6 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(uart_remove_one_port);
>  
>  /**
>   * uart_match_port - are the two ports equivalent?
> @@ -3251,6 +3276,127 @@ bool uart_match_port(const struct uart_port *port1,
>  }
>  EXPORT_SYMBOL(uart_match_port);
>  
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct device *serial_core_ctrl_find(struct uart_driver *drv,
> +					    struct device *phys_dev,
> +					    int ctrl_id)
> +{
> +	struct uart_state *state;
> +	int i;
> +
> +	if (ctrl_id < 0)
> +		return NULL;

Why is a negative number special here?


> +
> +	lockdep_assert_held(&port_mutex);
> +
> +	for (i = 0; i < drv->nr; i++) {
> +		state = drv->state + i;
> +		if (!state->uart_port || !state->uart_port->port_dev)
> +			continue;
> +
> +		if (state->uart_port->dev == phys_dev &&
> +		    state->uart_port->ctrl_id == ctrl_id)
> +			return state->uart_port->port_dev->parent;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct device *serial_core_ctrl_device_add(struct uart_port *port)
> +{
> +	return serial_base_device_add(port, "ctrl", port->dev);
> +}
> +
> +static int serial_core_port_device_add(struct device *ctrl_dev, struct uart_port *port)
> +{
> +	struct device *dev;
> +
> +	dev = serial_base_device_add(port, "port", ctrl_dev);

magic strings again :)

Do you really just want two different "types" of devices on this bus,
controllers and ports?  If so, just do that, don't make the name magic
here.

Then you can have:
	serial_base_port_add()
	serial_base_ctrl_add()

and one cleanup function will still work.

Otherwise this looks good to me, thanks for doing all of this work.

greg k-h

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230329 09:19]:
> On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> > +obj-$(CONFIG_SERIAL_CORE) += serial_base.o serial_core.o serial_ctrl.o serial_port.o
> 
> Why is this 3 new modules and not just all go into serial_base?  What's
> going to auto-load the other modules you created here?  Feels like this
> should all end up in the same .ko as they all depend on each other,
> right?

OK sure, I'll build them into serial_base. We now have uart_add_one_port()
and uart_remove_one_port() exported in serial_port so that ends up loading
the serial_base related modules.

> > +struct uart_port *serial_base_get_port(struct device *dev)
> > +{
> > +	struct serial_base_device *sbd;
> > +
> > +	if (!dev)
> > +		return NULL;
> > +
> > +	sbd = to_serial_base_device(dev);
> > +
> > +	/* Check in case serial_core_add_one_port() happened to fail */
> > +	if (!sbd->port->state) {
> 
> This is odd, how can it fail and then this function be called after that
> failure?

On uart_add_one_port(), runtime PM resume function in serial_port gets
called before the port registration has completed. Sounds like I need
to recheck this, maybe we can just enable runtime PM for serial_port
after registration has completed.

> > +/*
> > + * Find a registered serial core controller device if one exists. Returns
> > + * the first device matching the ctrl_id. Caller must hold port_mutex.
> > + */
> > +static struct device *serial_core_ctrl_find(struct uart_driver *drv,
> > +					    struct device *phys_dev,
> > +					    int ctrl_id)
> > +{
> > +	struct uart_state *state;
> > +	int i;
> > +
> > +	if (ctrl_id < 0)
> > +		return NULL;
> 
> Why is a negative number special here?

I think this can go, will check.

> > +	dev = serial_base_device_add(port, "port", ctrl_dev);
> 
> magic strings again :)
> 
> Do you really just want two different "types" of devices on this bus,
> controllers and ports?  If so, just do that, don't make the name magic
> here.
> 
> Then you can have:
> 	serial_base_port_add()
> 	serial_base_ctrl_add()
> 
> and one cleanup function will still work.

Yes two different types should do here, I'll take a look.

> Otherwise this looks good to me, thanks for doing all of this work.

OK great thanks,

Tony

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230329 09:08]:
> On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > --- /dev/null
> > +++ b/drivers/tty/serial/serial_base.c
> > @@ -0,0 +1,142 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> Given that the driver core is "GPL-2.0-only", why is this -or-later?

Thanks for noticing, the intention was to set it to what serial_core.c has:

$ git grep -e SPDX -e MODULE_LICENSE drivers/tty/serial/serial_core.c
drivers/tty/serial/serial_core.c:// SPDX-License-Identifier: GPL-2.0+
drivers/tty/serial/serial_core.c:MODULE_LICENSE("GPL");

> No copyright notice?  That's fine, but again, I have to ask.

Will add thanks.

Tony

On Thu, Mar 30, 2023 at 02:32:31PM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230329 09:08]:
> > On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/serial_base.c
> > > @@ -0,0 +1,142 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > 
> > Given that the driver core is "GPL-2.0-only", why is this -or-later?
> 
> Thanks for noticing, the intention was to set it to what serial_core.c has:
> 
> $ git grep -e SPDX -e MODULE_LICENSE drivers/tty/serial/serial_core.c
> drivers/tty/serial/serial_core.c:// SPDX-License-Identifier: GPL-2.0+
> drivers/tty/serial/serial_core.c:MODULE_LICENSE("GPL");

Doesn't mean you have to copy it, it's your choice :)

thanks,

greg k-h

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230330 12:52]:
> On Thu, Mar 30, 2023 at 02:32:31PM +0300, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230329 09:08]:
> > > On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > > > --- /dev/null
> > > > +++ b/drivers/tty/serial/serial_base.c
> > > > @@ -0,0 +1,142 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > 
> > > Given that the driver core is "GPL-2.0-only", why is this -or-later?
> > 
> > Thanks for noticing, the intention was to set it to what serial_core.c has:
> > 
> > $ git grep -e SPDX -e MODULE_LICENSE drivers/tty/serial/serial_core.c
> > drivers/tty/serial/serial_core.c:// SPDX-License-Identifier: GPL-2.0+
> > drivers/tty/serial/serial_core.c:MODULE_LICENSE("GPL");
> 
> Doesn't mean you have to copy it, it's your choice :)

In this case my preference is to use the same as for serial_core.c. This is
so folks can start moving serial port specific functions from serial_core.c
to serial_port.c. And not have to worry about the possible differences with
various licenses at that point :)

Tony

On Thu, Mar 30, 2023 at 03:59:31PM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230330 12:52]:
> > On Thu, Mar 30, 2023 at 02:32:31PM +0300, Tony Lindgren wrote:
> > > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230329 09:08]:
> > > > On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > > > > --- /dev/null
> > > > > +++ b/drivers/tty/serial/serial_base.c
> > > > > @@ -0,0 +1,142 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > 
> > > > Given that the driver core is "GPL-2.0-only", why is this -or-later?
> > > 
> > > Thanks for noticing, the intention was to set it to what serial_core.c has:
> > > 
> > > $ git grep -e SPDX -e MODULE_LICENSE drivers/tty/serial/serial_core.c
> > > drivers/tty/serial/serial_core.c:// SPDX-License-Identifier: GPL-2.0+
> > > drivers/tty/serial/serial_core.c:MODULE_LICENSE("GPL");
> > 
> > Doesn't mean you have to copy it, it's your choice :)
> 
> In this case my preference is to use the same as for serial_core.c. This is
> so folks can start moving serial port specific functions from serial_core.c
> to serial_port.c. And not have to worry about the possible differences with
> various licenses at that point :)

That's great, no objection from me, sorry, but I had to ask, the "no
fun" part of my job...

greg k-h