Re: [PATCH 1/7] serial: core: Add support of runtime PM
From: Johan Hovold <johan@kernel.org>
Date: 2021-11-30 10:29:13
Also in:
linux-omap, lkml
On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 8250 driver has wrong implementation of runtime power management, i.e. it uses an irq_safe flag. The irq_safe flag takes a permanent usage count on the parent device preventing the parent from idling. This patch prepares for making runtime power management generic by adding runtime PM calls to serial core once for all UART drivers. As we have serial drivers that do not enable runtime PM, and drivers that enable runtime PM, we add new functions for serial_pm_resume_and_get() and serial_pm_autosuspend() functions to handle errors and allow the use also for cases when runtime PM is not enabled. The other option considered was to not check for runtime PM enable errors. But some CPUs can hang when the clocks are not enabled for the device, so ignoring the errors is not a good option. Eventually with the serial port drivers updated, we should be able to just switch to using the standard runtime PM calls with no need for the wrapper functions.
A third option which needs to be considered is to always enable runtime pm in core but to keep the ports active while they are opened unless a driver opts in for more aggressive power management. This is how USB devices are handled for example. A next step could then be to move over uart_change_pm() to be handled from the pm callbacks.
quoted hunk ↗ jump to hunk
Note that this patch only adds runtime PM calls to the functions where we can call them for synchronous wake-up without the need for irq_safe flag. Additionally we also need asynchronous wake-up support for __uart_start(), that is added in a separate patch. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Co-developed-by: Tony Lindgren <tony@atomide.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/tty/serial/serial_core.c | 150 ++++++++++++++++++++++++++++--- 1 file changed, 140 insertions(+), 10 deletions(-)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>@@ -91,6 +92,27 @@ static inline struct uart_port *uart_port_check(struct uart_state *state) return state->uart_port; } +/* + * Enables runtime PM suspended serial port. If runtime PM is not + * enabled by the serial port driver, only increments the runtime PM + * usage count. May sleep. + */ +static int serial_pm_resume_and_get(struct device *dev) +{ + if (!pm_runtime_enabled(dev)) { + pm_runtime_get_noresume(dev); + return 0; + } + + return pm_runtime_resume_and_get(dev); +} + +static void serial_pm_autosuspend(struct device *dev) +{ + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); +}
The put helper needs a "put" in its name. If these are at all needed, they could both reflect the underlying helpers names, that is: serial_pm_resume_and_get() serial_pm_put_autosuspend() When wrapping the generic helpers with subsystem specific helpers like this you should also pass in a pointer to the uart_port instead of a struct device.
quoted hunk ↗ jump to hunk
+ /* * This routine is used by the interrupt handler to schedule processing in * the software interrupt portion of the driver.@@ -143,13 +165,18 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear) { unsigned long flags; unsigned int old; + int err;
Nit: Pleas use "ret" consistently rather than introducing "err" in some of the helpers.
+ err = serial_pm_resume_and_get(port->dev); + if (err < 0) + return;
Adding a newline after get and before put would make several functions more readable (e.g. when doing more than just a single subdriver call).
quoted hunk ↗ jump to hunk
spin_lock_irqsave(&port->lock, flags); old = port->mctrl; port->mctrl = (old & ~clear) | set; if (old != port->mctrl) port->ops->set_mctrl(port, port->mctrl); spin_unlock_irqrestore(&port->lock, flags); + serial_pm_autosuspend(port->dev); } #define uart_set_mctrl(port, set) uart_update_mctrl(port, set, 0)@@ -218,7 +245,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, free_page(page); } + retval = serial_pm_resume_and_get(uport->dev); + if (retval < 0) + goto out;
Just return retval here.
quoted hunk ↗ jump to hunk
retval = uport->ops->startup(uport); + serial_pm_autosuspend(uport->dev); + if (retval == 0) { if (uart_console(uport) && uport->cons->cflag) { tty->termios.c_cflag = uport->cons->cflag;@@ -244,7 +276,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, */ if (retval && capable(CAP_SYS_ADMIN)) return 1; - +out: return retval; }@@ -481,6 +513,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state, struct uart_port *uport = uart_port_check(state); struct ktermios *termios; int hw_stopped; + int err; /* * If we have no tty, termios, or the port does not exist,@@ -490,6 +523,11 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state, return; termios = &tty->termios; + + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + return; + uport->ops->set_termios(uport, termios, old_termios); /*@@ -518,6 +556,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state, __uart_start(tty); } spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev); } static int uart_put_char(struct tty_struct *tty, unsigned char c)@@ -1015,7 +1054,14 @@ static int uart_get_lsr_info(struct tty_struct *tty, struct uart_port *uport = uart_port_check(state); unsigned int result; + result = serial_pm_resume_and_get(uport->dev); + if (result < 0) { + result = TIOCSER_TEMT;
Why not return an error?
quoted hunk ↗ jump to hunk
+ goto out; + } + result = uport->ops->tx_empty(uport); + serial_pm_autosuspend(uport->dev); /* * If we're about to load something into the transmit@@ -1027,7 +1073,7 @@ static int uart_get_lsr_info(struct tty_struct *tty, ((uart_circ_chars_pending(&state->xmit) > 0) && !uart_tx_stopped(uport))) result &= ~TIOCSER_TEMT; - +out: return put_user(result, value); }@@ -1045,9 +1091,14 @@ static int uart_tiocmget(struct tty_struct *tty) if (!tty_io_error(tty)) { result = uport->mctrl; + + result = serial_pm_resume_and_get(uport->dev); + if (result < 0) + goto out; spin_lock_irq(&uport->lock); result |= uport->ops->get_mctrl(uport); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev); } out: mutex_unlock(&port->mutex);@@ -1088,8 +1139,12 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state) if (!uport) goto out; + ret = serial_pm_resume_and_get(uport->dev); + if (ret < 0) + goto out; if (uport->type != PORT_UNKNOWN && uport->ops->break_ctl) uport->ops->break_ctl(uport, break_state); + serial_pm_autosuspend(uport->dev); ret = 0; out: mutex_unlock(&port->mutex);@@ -1138,7 +1193,11 @@ static int uart_do_autoconfig(struct tty_struct *tty, struct uart_state *state) * This will claim the ports resources if * a port is found. */ + ret = serial_pm_resume_and_get(uport->dev); + if (ret < 0) + goto out; uport->ops->config_port(uport, flags); + serial_pm_autosuspend(uport->dev); ret = uart_startup(tty, state, 1); if (ret == 0)@@ -1443,14 +1502,21 @@ static void uart_set_ldisc(struct tty_struct *tty) struct uart_state *state = tty->driver_data; struct uart_port *uport; struct tty_port *port = &state->port; + int err; if (!tty_port_initialized(port)) return; mutex_lock(&state->port.mutex); uport = uart_port_check(state); - if (uport && uport->ops->set_ldisc) + if (uport && uport->ops->set_ldisc) { + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + goto out; uport->ops->set_ldisc(uport, &tty->termios); + serial_pm_autosuspend(uport->dev); + } +out: mutex_unlock(&state->port.mutex); }@@ -1542,6 +1608,7 @@ static void uart_tty_port_shutdown(struct tty_port *port) { struct uart_state *state = container_of(port, struct uart_state, port); struct uart_port *uport = uart_port_check(state); + int err; /* * At this point, we stop accepting input. To do this, we@@ -1550,9 +1617,13 @@ static void uart_tty_port_shutdown(struct tty_port *port) if (WARN(!uport, "detached port still initialized!\n")) return; + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + return;
You probably need to do whatever you can to continue on errors here rather than just bail out.
quoted hunk ↗ jump to hunk
spin_lock_irq(&uport->lock); uport->ops->stop_rx(uport); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev); uart_port_shutdown(port);@@ -1668,6 +1739,7 @@ static void uart_port_shutdown(struct tty_port *port) { struct uart_state *state = container_of(port, struct uart_state, port); struct uart_port *uport = uart_port_check(state); + int err; /* * clear delta_msr_wait queue to avoid mem leaks: we may free@@ -1681,8 +1753,13 @@ static void uart_port_shutdown(struct tty_port *port) /* * Free the IRQ and disable the port. */ - if (uport) + if (uport) { + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + return;
Same here.
quoted hunk ↗ jump to hunk
uport->ops->shutdown(uport); + serial_pm_autosuspend(uport->dev); + } /* * Ensure that the IRQ handler isn't running on another CPU.@@ -1803,7 +1880,7 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i) struct uart_port *uport; char stat_buf[32]; unsigned int status; - int mmio; + int mmio, err; mutex_lock(&port->mutex); uport = uart_port_check(state);@@ -1824,12 +1901,16 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i) } if (capable(CAP_SYS_ADMIN)) { + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + goto out; pm_state = state->pm_state; if (pm_state != UART_PM_STATE_ON) uart_change_pm(state, UART_PM_STATE_ON); spin_lock_irq(&uport->lock); status = uport->ops->get_mctrl(uport); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev); if (pm_state != UART_PM_STATE_ON) uart_change_pm(state, pm_state);
The interaction with uart_change_pm() looks inconsistent. Why resume before the state change and also suspend *before* updating the pm state? That is, shouldn't the suspend go after uart_change_pm()? And similar in other places.
quoted hunk ↗ jump to hunk
@@ -2050,6 +2131,7 @@ uart_set_options(struct uart_port *port, struct console *co, { struct ktermios termios; static struct ktermios dummy; + int ret; /* * Ensure that the serial-console lock is initialised early.@@ -2089,7 +2171,17 @@ uart_set_options(struct uart_port *port, struct console *co, */ port->mctrl |= TIOCM_DTR; - port->ops->set_termios(port, &termios, &dummy); + /* At early stage device is not created yet, we can't do PM */ + if (port->dev) {
Checking port->dev here looks a bit hacky. Can you expand on this and also on how this relates to console ports presumably never being runtime suspended?
quoted hunk ↗ jump to hunk
+ ret = serial_pm_resume_and_get(port->dev); + if (ret < 0) + return ret; + port->ops->set_termios(port, &termios, &dummy); + serial_pm_autosuspend(port->dev); + } else { + port->ops->set_termios(port, &termios, &dummy); + } + /* * Allow the setting of the UART parameters with a NULL console * too:@@ -2143,6 +2235,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) struct tty_port *port = &state->port; struct device *tty_dev; struct uart_match match = {uport, drv}; + int err; mutex_lock(&port->mutex);@@ -2168,11 +2261,15 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) tty_port_set_suspended(port, 1); tty_port_set_initialized(port, 0); + err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + goto unlock; spin_lock_irq(&uport->lock); ops->stop_tx(uport); ops->set_mctrl(uport, 0); ops->stop_rx(uport); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev);
Just keep the device active until after ->shutdown() below.
quoted hunk ↗ jump to hunk
/* * Wait for the transmitter to empty.@@ -2183,7 +2280,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) dev_err(uport->dev, "%s: Unable to drain transmitter\n", uport->name);
Note that there's a call to tx_empty() just above which also requires the device to be resumed. This is also missing in wait_until_sent().
quoted hunk ↗ jump to hunk
+ err = serial_pm_resume_and_get(uport->dev); + if (err < 0) + goto unlock; ops->shutdown(uport); + serial_pm_autosuspend(uport->dev); } /*@@ -2206,6 +2307,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport) struct device *tty_dev; struct uart_match match = {uport, drv}; struct ktermios termios; + int ret; mutex_lock(&port->mutex);@@ -2236,33 +2338,50 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport) if (port->tty && termios.c_cflag == 0) termios = port->tty->termios; + ret = serial_pm_resume_and_get(uport->dev); + if (ret < 0) + goto unlock; if (console_suspend_enabled) uart_change_pm(state, UART_PM_STATE_ON); uport->ops->set_termios(uport, &termios, NULL); + serial_pm_autosuspend(uport->dev); + if (console_suspend_enabled) console_start(uport->cons); } if (tty_port_suspended(port)) { const struct uart_ops *ops = uport->ops; - int ret; + ret = serial_pm_resume_and_get(uport->dev); + if (ret < 0) + goto unlock; uart_change_pm(state, UART_PM_STATE_ON); spin_lock_irq(&uport->lock); ops->set_mctrl(uport, 0); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev);
Similar here, just keep the device active until you're done.
+
if (console_suspend_enabled || !uart_console(uport)) {
/* Protected by port mutex for now */
struct tty_struct *tty = port->tty;
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto unlock;
ret = ops->startup(uport);
+ serial_pm_autosuspend(uport->dev);Ditto.
quoted hunk ↗ jump to hunk
if (ret == 0) { if (tty) uart_change_speed(tty, state, NULL); + ret = serial_pm_resume_and_get(uport->dev); + if (ret < 0) + goto unlock; spin_lock_irq(&uport->lock); ops->set_mctrl(uport, uport->mctrl); ops->start_tx(uport); spin_unlock_irq(&uport->lock); + serial_pm_autosuspend(uport->dev); tty_port_set_initialized(port, 1); } else { /*@@ -2276,10 +2395,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport) tty_port_set_suspended(port, 0); } - +unlock: mutex_unlock(&port->mutex); - return 0; + return ret; } static inline void@@ -2329,6 +2448,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, struct uart_port *port) { unsigned int flags; + int err; /* * If there isn't a port here, don't do anything further.@@ -2356,6 +2476,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, uart_report_port(drv, port); + err = serial_pm_resume_and_get(port->dev); + if (err < 0) + return; + /* Power up port for set_mctrl() */ uart_change_pm(state, UART_PM_STATE_ON);@@ -2367,6 +2491,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, spin_lock_irqsave(&port->lock, flags); port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR); spin_unlock_irqrestore(&port->lock, flags); + serial_pm_autosuspend(port->dev); /* * If this driver supports console, and it hasn't been@@ -3084,11 +3209,16 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change); */ void uart_handle_cts_change(struct uart_port *uport, unsigned int status) { + int err; + lockdep_assert_held_once(&uport->lock); uport->icount.cts++; if (uart_softcts_mode(uport)) { + err = serial_pm_resume_and_get(uport->dev);
This won't fly as this callback is called in atomic context so you cannot do a synchronous resume here.
quoted hunk ↗ jump to hunk
+ if (err < 0) + return; if (uport->hw_stopped) { if (status) { uport->hw_stopped = 0;@@ -3101,7 +3231,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status) uport->ops->stop_tx(uport); } } - + serial_pm_autosuspend(uport->dev); } } EXPORT_SYMBOL_GPL(uart_handle_cts_change);
Johan