| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e6922dd6-8968-b54-afbc-d5ad969f64@linux.intel.com>
Date: Fri, 25 Nov 2022 16:02:45 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Tony Lindgren <tony@...mide.com>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Shevchenko <andriy.shevchenko@...el.com>,
Jiri Slaby <jirislaby@...nel.org>,
Johan Hovold <johan@...nel.org>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Vignesh Raghavendra <vigneshr@...com>,
linux-serial <linux-serial@...r.kernel.org>,
linux-omap@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] serial: core: Start managing serial controllers
to enable runtime PM
On Wed, 23 Nov 2022, 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 physical serial controller devices.
>
> The serial layer has a few challenges to deal with:
>
> 1. The serial port mapping to a physical serial port controller device
> is currently not easily available after the physical serial controller
> struct device gets registered uart_add_one_port() time
>
> 2. The serial port device drivers have their own driver data. So we cannot
> currently start making use of serial core generic data easily without
> changing all the serial port device drivers
>
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. On registering a port,
> we can use the drv->state array to find the associated serial port
> controller and initialize the serial core controller.
>
> 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 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. We just enable runtime PM and keep the runtime PM usage
> count until all the serial controller ports are unregistered. For drivers
> implementing PM runtime, we just keep track of the runtime PM
> configuration.
>
> For the serial port drivers, the serial core layer has the following use
> cases to deal with:
>
> 1. 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
>
> 2. 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
>
> 3. 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.
>
> Suggested-by: Johan Hovold <johan@...nel.org>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
> 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/8250/8250_omap.c | 1 +
> drivers/tty/serial/serial_core.c | 179 ++++++++++++++++++++++++++++
> include/linux/serial_core.h | 4 +
> 4 files changed, 185 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
> @@ -1004,6 +1004,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
> @@ -1330,6 +1330,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,6 +16,7 @@
> #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/seq_file.h>
> #include <linux/device.h>
> @@ -30,6 +31,20 @@
> #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; /* Physical controller device */
> + struct uart_driver *drv; /* For port specific uart_state */
> + struct kref ref; /* Registered port count */
> + unsigned long implements_pm_runtime:1;
> + unsigned long supports_autosuspend:1;
> +};
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -177,6 +192,162 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
> uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
> }
>
> +static struct serial_controller *to_controller(struct uart_port *port)
> +{
> + if (!port->dev)
> + return NULL;
> +
> + return port->state->controller;
> +}
> +
> +/*
> + * Starts runtime PM for the serial controller device if not already started
> + * by the serial port driver. Called from uart_add_one_port() with port_mutex
> + * held.
> + */
> +static int serial_core_pm_runtime_start(struct uart_port *port)
> +{
> + struct serial_controller *controller = to_controller(port);
> + 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);
Should this be done regardless of autosuspend if the port is console?
There's a problem in the current place where this being called from
though, uart_console_enabled() doesn't return the correct value with at
least some devices this early:
https://lore.kernel.org/linux-serial/AS8PR04MB84047F39CD10C00CEE29213F92219@AS8PR04MB8404.eurprd04.prod.outlook.com/
--
i.
Powered by blists - more mailing lists