Message-ID: <YaX82wxybOZnPKpy@hovoldconsulting.com>
Date:   Tue, 30 Nov 2021 11:28:43 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Jiri Slaby <jirislaby@...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,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 1/7] serial: core: Add support of runtime PM

On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> From: Andy Shevchenko <andriy.shevchenko@...ux.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.

> 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@...ux.intel.com>
> Co-developed-by: Tony Lindgren <tony@...mide.com>
> Signed-off-by: Tony Lindgren <tony@...mide.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.

> +
>  /*
>   * 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).

>  	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.

>  	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?

> +		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.

>  	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.

>  		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.

> @@ -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?

> +		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.

>  		/*
>  		 * 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().

>  
> +		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.

>  			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.

> +		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

Powered by blists - more mailing lists