Date:   Wed, 15 Jun 2022 12:17:17 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Johan Hovold <johan@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] serial: core: Start managing serial controllers
 to enable runtime PM

On Wed, Jun 15, 2022 at 09:24:55AM +0300, Tony Lindgren wrote:
> 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 serial port controllers. For runtime PM, we need a way to find
> the serial ports for each serial port controller device.
> 
> The serial core manages ports. Each serial controller can have multiple
> ports. As serial core has no struct device, and the serial port device
> drivers have their own driver data, we cannot currently start making
> use of serial core generic data easily without changing all the serial
> port device drivers.
> 
> We could consider adding a serial core specific struct device. It would
> be a child of the serial port device, and would allow us eventually to use
> device_links to add generic runtime PM calls for example. But as the serial
> core layer is not a device driver, driver specific features would need to
> be added, and are probably not justified for a virtual device.
> 
> Considering the above, let's improve the serial core layer so we can
> manage the serial port controllers better. Let's register the controllers
> with the serial core layer in addition to the serial ports.
> 
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. Let's add the registered
> devices into a radix_tree so we can look up the controller easily even
> with many controllers registered. This allows us to keep track of the
> runtime PM state for each serial port controller device.
> 
> As some serial port device drivers enable runtime PM in their probe before
> registering with the serial core layer, and some do not enable runtime PM
> at all currently, we need check the state in the serial core layer on
> uart_port_startup(). We need to also consider that a serial port device
> may have multiple ports.
> 
> Initially we just want to enable runtime PM for all the serial port
> controller devices. This allows us to add runtime PM calls and properly
> handle any errors without a need for serial layer specific runtime PM
> wrapper functions.
> 
> After this patch no functional changes for the serial port device drivers
> are intended. For most cases, we just enable runtime PM and keep the
> runtime PM usage count until all the serial controller ports are
> unregistered. For drivers implementing runtime PM, we just keep track of
> the configuration.
> 
> The serial core layer has the following use cases to deal with:
> 
> - If a serial port device driver does not implement runtime PM, the
>   device state is set to active state, and the runtime PM usage count
>   is kept until the last port for a device is unregistered
> 
> - If a serial port device driver implements runtime PM, the runtime PM
>   usage count is kept until the last port for the device is unregistered
> 
> - If a serial port device driver implements runtime PM autosuspend,
>   autosuspend is not prevented. This currently gets set only for the
>   8250_omap driver to keep runtime PM working for it
> 
> For system suspend, things should be mostly detached from the runtime PM.
> The serial port device drivers may call pm_runtime_force_suspend() and
> pm_runtime_force_resume() as needed.

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

A couple of nit-picks below.

> Suggested-by: Johan Hovold <johan@...nel.org>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
> 
> 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/8250/8250_omap.c |   1 +
>  drivers/tty/serial/serial_core.c    | 148 ++++++++++++++++++++++++++++
>  include/linux/serial_core.h         |   3 +
>  4 files changed, 153 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -995,6 +995,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  		uart->port.regshift     = up->port.regshift;
>  		uart->port.iotype       = up->port.iotype;
>  		uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> +		uart->port.supports_autosuspend = up->port.supports_autosuspend;
>  		uart->bugs		= up->bugs;
>  		uart->port.mapbase      = up->port.mapbase;
>  		uart->port.mapsize      = up->port.mapsize;
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1338,6 +1338,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  	up.rs485_start_tx = serial8250_em485_start_tx;
>  	up.rs485_stop_tx = serial8250_em485_stop_tx;
>  	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
> +	up.port.supports_autosuspend = 1;
>  
>  	ret = of_alias_get_id(np, "serial");
>  	if (ret < 0) {
> 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
> @@ -16,7 +16,9 @@
>  #include <linux/console.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/proc_fs.h>
> +#include <linux/radix-tree.h>
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/serial.h> /* for serial_state and serial_icounter_struct */
> @@ -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).

> +	struct uart_driver *drv;		/* For port specific uart_state */
> +	struct kref ref;			/* Enable count for runtime PM */
> +	unsigned long implements_pm_runtime:1;
> +	unsigned long supports_autosuspend:1;
> +};
> +
> +/*
> + * Serial core port device instances. Update protected by port_mutex.
> + */
> +static RADIX_TREE(serial_core_devices, GFP_NOWAIT);
> +
>  /*
>   * This is used to lock changes in serial line configuration.
>   */
> @@ -175,6 +196,125 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
>  		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
>  }
>  
> +/* Called from uart_add_one_port() with port_mutex held */
> +static int serial_core_pm_runtime_start(struct uart_port *port)
> +{
> +	struct uart_state *state = port->state;
> +	struct serial_controller *controller = state->controller;
> +	struct device *dev = port->dev;
> +	int ret = 0;
> +
> +	if (kref_get_unless_zero(&controller->ref))
> +		return 0;
> +
> +	/* Init controller device on first reference */
> +	kref_init(&controller->ref);
> +
> +	/* Always enable autosuspend and consider child devices for serdev */
> +	pm_runtime_use_autosuspend(dev);
> +	pm_suspend_ignore_children(dev, false);
> +
> +	/*
> +	 * If the port driver did not enable runtime PM in probe, do it now.
> +	 * Devices that did not enable runtime PM get set active so we can
> +	 * properly handle the returned errors for runtime PM calls.
> +	 */
> +	if (!pm_runtime_enabled(dev)) {
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +	} else {
> +		controller->implements_pm_runtime = 1;
> +	}
> +
> +	/*
> +	 * Keep the port device enabled unless autosuspend is supported.
> +	 * Released on port shutdown.
> +	 */
> +	if (!controller->supports_autosuspend) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			pm_runtime_dont_use_autosuspend(dev);
> +			pm_runtime_disable(dev);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/* Clean up the runtime PM settings done on serial_core_register_port() */
> +static void serial_core_pm_runtime_cleanup(struct kref *ref)
> +{
> +	struct serial_controller *controller =
> +		 container_of(ref, struct serial_controller, ref);
> +	struct device *dev = controller->dev;
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_suspend_ignore_children(dev, true);
> +	if (!controller->supports_autosuspend)
> +		pm_runtime_put_sync(dev);
> +	if (!controller->implements_pm_runtime) {
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_disable(dev);
> +	}
> +}
> +
> +/* Called from uart_remove_one_port() and on error path with port_mutex held */
> +static void serial_core_unregister_port(struct uart_port *port)
> +{
> +	unsigned long idx = (unsigned long)port->dev;
> +	struct serial_controller *controller;
> +
> +	/* Check for a registered controller, no struct device early on */
> +	controller = radix_tree_lookup(&serial_core_devices, idx);
> +	if (!controller)
> +		return;
> +
> +	kref_put(&controller->ref, serial_core_pm_runtime_cleanup);
> +
> +	controller = radix_tree_delete(&serial_core_devices, idx);
> +	kfree(controller);
> +}
> +
> +/* Called from uart_add_one_port() with port_mutex held */
> +static int serial_core_register_port(struct uart_port *port,
> +				     struct uart_driver *drv)
> +{
> +	unsigned long idx = (unsigned long)port->dev;
> +	struct serial_controller *controller;
> +	int ret;
> +
> +	if (!idx)
> +		return 0;
> +
> +	controller = radix_tree_lookup(&serial_core_devices, idx);
> +	if (controller) {
> +		port->state->controller = controller;
> +		WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
> +		return serial_core_pm_runtime_start(port);
> +	}
> +
> +	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +	if (!controller)
> +		return -ENOMEM;
> +
> +	controller->drv = drv;
> +	controller->dev = port->dev;
> +	controller->supports_autosuspend = port->supports_autosuspend;
> +	port->state->controller = controller;
> +
> +	ret = radix_tree_insert(&serial_core_devices, idx, controller);
> +	if (ret) {
> +		kfree(controller);
> +		return ret;
> +	}
> +
> +	ret = serial_core_pm_runtime_start(port);
> +	if (ret < 0)
> +		serial_core_unregister_port(port);
> +
> +	return ret;
> +}
> +
>  /*
>   * Startup the port.  This will be called once per open.  All calls
>   * will be serialised by the per-port mutex.
> @@ -2956,6 +3096,10 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  		goto out;
>  	}
>  
> +	ret = serial_core_register_port(uport, drv);
> +	if (ret)
> +		goto out;
> +
>  	/*
>  	 * If this port is in use as a console then the spinlock is already
>  	 * initialised.
> @@ -2979,6 +3123,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  				    GFP_KERNEL);
>  	if (!uport->tty_groups) {
>  		ret = -ENOMEM;
> +		serial_core_unregister_port(uport);
>  		goto out;
>  	}
>  	uport->tty_groups[0] = &tty_dev_attr_group;
> @@ -3048,6 +3193,9 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>  		goto out;
>  	}
>  	uport->flags |= UPF_DEAD;
> +
> +	serial_core_unregister_port(uport);
> +
>  	mutex_unlock(&port->mutex);
>  
>  	/*
> 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.

>  	struct tty_port		port;
>  
>  	enum uart_pm_state	pm_state;
> -- 
> 2.36.1

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists