[1/7] serial: core: Add support of runtime PM

Commit Message

Tony Lindgren Nov. 15, 2021, 8:41 a.m. UTC

From: Andy Shevchenko <[e-mail]>

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.

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 <[e-mail]>
Co-developed-by: Tony Lindgren <[e-mail]>
Signed-off-by: Tony Lindgren <[e-mail]>
---
 drivers/tty/serial/serial_core.c | 150 ++++++++++++++++++++++++++++---
 1 file changed, 140 insertions(+), 10 deletions(-)

Comments

kernel test robot Nov. 16, 2021, 7:40 a.m. UTC | #1

Hi Tony,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v5.16-rc1 next-20211116]
[cannot apply to tty/tty-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tony-Lindgren/Serial-port-generic-PM-to-fix-8250-PM/20211115-164354
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: hexagon-randconfig-r045-20211115 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project fbe72e41b99dc7994daac300d208a955be3e4a0a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d9568fa846ba1319eacfd03d39b48c3af05fe9f9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tony-Lindgren/Serial-port-generic-PM-to-fix-8250-PM/20211115-164354
        git checkout d9568fa846ba1319eacfd03d39b48c3af05fe9f9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/serial_core.c:2353:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (tty_port_suspended(port)) {
               ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/serial_core.c:2401:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/tty/serial/serial_core.c:2353:2: note: remove the 'if' if its condition is always true
           if (tty_port_suspended(port)) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/serial_core.c:2310:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +2353 drivers/tty/serial/serial_core.c

^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2302  
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2303  int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2304  {
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2305  	struct uart_state *state = drv->state + uport->line;
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2306  	struct tty_port *port = &state->port;
03a74dcc7eebe6 drivers/serial/serial_core.c     Arjan van de Ven      2008-05-23  2307  	struct device *tty_dev;
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2308  	struct uart_match match = {uport, drv};
ba15ab0e8de0d4 drivers/serial/serial_core.c     Deepak Saxena         2009-09-19  2309  	struct ktermios termios;
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2310  	int ret;
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2311  
a2bceae065ed8c drivers/serial/serial_core.c     Alan Cox              2009-09-19  2312  	mutex_lock(&port->mutex);
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2313  
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2314  	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2315  	if (!uport->suspended && device_may_wakeup(tty_dev)) {
aef3ad103a686f drivers/tty/serial/serial_core.c Andy Shevchenko       2017-08-13  2316  		if (irqd_is_wakeup_set(irq_get_irq_data((uport->irq))))
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2317  			disable_irq_wake(uport->irq);
5a65dcc04cda41 drivers/tty/serial/serial_core.c Federico Vaga         2013-04-15  2318  		put_device(tty_dev);
a2bceae065ed8c drivers/serial/serial_core.c     Alan Cox              2009-09-19  2319  		mutex_unlock(&port->mutex);
b3b708fa2780cd drivers/serial/serial_core.c     Guennadi Liakhovetski 2007-10-16  2320  		return 0;
b3b708fa2780cd drivers/serial/serial_core.c     Guennadi Liakhovetski 2007-10-16  2321  	}
5a65dcc04cda41 drivers/tty/serial/serial_core.c Federico Vaga         2013-04-15  2322  	put_device(tty_dev);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2323  	uport->suspended = 0;
b3b708fa2780cd drivers/serial/serial_core.c     Guennadi Liakhovetski 2007-10-16  2324  
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2325  	/*
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2326  	 * Re-enable the console device after suspending.
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2327  	 */
5933a161abcb8d drivers/tty/serial/serial_core.c Yin Kangkai           2011-01-30  2328  	if (uart_console(uport)) {
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2329  		/*
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2330  		 * First try to use the console cflag setting.
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2331  		 */
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2332  		memset(&termios, 0, sizeof(struct ktermios));
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2333  		termios.c_cflag = uport->cons->cflag;
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2334  
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2335  		/*
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2336  		 * If that's unset, use the tty termios setting.
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2337  		 */
adc8d746caa67f drivers/tty/serial/serial_core.c Alan Cox              2012-07-14  2338  		if (port->tty && termios.c_cflag == 0)
adc8d746caa67f drivers/tty/serial/serial_core.c Alan Cox              2012-07-14  2339  			termios = port->tty->termios;
891b9dd1076435 drivers/serial/serial_core.c     Jason Wang            2010-08-21  2340  
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2341  		ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2342  		if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2343  			goto unlock;
94abc56f4d90f2 drivers/tty/serial/serial_core.c Ning Jiang            2011-09-05  2344  		if (console_suspend_enabled)
6f538fe31c1d45 drivers/tty/serial/serial_core.c Linus Walleij         2012-12-07  2345  			uart_change_pm(state, UART_PM_STATE_ON);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2346  		uport->ops->set_termios(uport, &termios, NULL);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2347  		serial_pm_autosuspend(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2348  
5933a161abcb8d drivers/tty/serial/serial_core.c Yin Kangkai           2011-01-30  2349  		if (console_suspend_enabled)
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2350  			console_start(uport->cons);
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2351  	}
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2352  
80f02d5424301b drivers/tty/serial/serial_core.c Peter Hurley          2016-04-09 @2353  	if (tty_port_suspended(port)) {
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2354  		const struct uart_ops *ops = uport->ops;
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2355  
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2356  		ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2357  		if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2358  			goto unlock;
6f538fe31c1d45 drivers/tty/serial/serial_core.c Linus Walleij         2012-12-07  2359  		uart_change_pm(state, UART_PM_STATE_ON);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2360  		spin_lock_irq(&uport->lock);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2361  		ops->set_mctrl(uport, 0);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2362  		spin_unlock_irq(&uport->lock);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2363  		serial_pm_autosuspend(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2364  
4547be7809a3b7 drivers/serial/serial_core.c     Stanislav Brabec      2009-12-02  2365  		if (console_suspend_enabled || !uart_console(uport)) {
192251352f912b drivers/serial/serial_core.c     Alan Cox              2010-06-01  2366  			/* Protected by port mutex for now */
192251352f912b drivers/serial/serial_core.c     Alan Cox              2010-06-01  2367  			struct tty_struct *tty = port->tty;
4ed71addf51a62 drivers/tty/serial/serial_core.c Tamseel Shams         2020-07-16  2368  
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2369  			ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2370  			if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2371  				goto unlock;
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2372  			ret = ops->startup(uport);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2373  			serial_pm_autosuspend(uport->dev);
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2374  			if (ret == 0) {
192251352f912b drivers/serial/serial_core.c     Alan Cox              2010-06-01  2375  				if (tty)
192251352f912b drivers/serial/serial_core.c     Alan Cox              2010-06-01  2376  					uart_change_speed(tty, state, NULL);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2377  				ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2378  				if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2379  					goto unlock;
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2380  				spin_lock_irq(&uport->lock);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2381  				ops->set_mctrl(uport, uport->mctrl);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2382  				ops->start_tx(uport);
ccce6debb62d94 drivers/serial/serial_core.c     Alan Cox              2009-09-19  2383  				spin_unlock_irq(&uport->lock);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2384  				serial_pm_autosuspend(uport->dev);
d41861ca19c9e9 drivers/tty/serial/serial_core.c Peter Hurley          2016-04-09  2385  				tty_port_set_initialized(port, 1);
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2386  			} else {
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2387  				/*
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2388  				 * Failed to resume - maybe hardware went away?
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2389  				 * Clear the "initialized" flag so we won't try
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2390  				 * to call the low level drivers shutdown method.
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2391  				 */
192251352f912b drivers/serial/serial_core.c     Alan Cox              2010-06-01  2392  				uart_shutdown(tty, state);
ee31b337852ca8 drivers/serial/serial_core.c     Russell King          2005-11-13  2393  			}
4547be7809a3b7 drivers/serial/serial_core.c     Stanislav Brabec      2009-12-02  2394  		}
a6b93a90850881 drivers/serial/serial_core.c     Russell King          2006-10-01  2395  
80f02d5424301b drivers/tty/serial/serial_core.c Peter Hurley          2016-04-09  2396  		tty_port_set_suspended(port, 0);
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2397  	}
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2398  unlock:
a2bceae065ed8c drivers/serial/serial_core.c     Alan Cox              2009-09-19  2399  	mutex_unlock(&port->mutex);
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2400  
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko       2021-11-15  2401  	return ret;
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2402  }
^1da177e4c3f41 drivers/serial/serial_core.c     Linus Torvalds        2005-04-16  2403  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Johan Hovold Nov. 30, 2021, 10:28 a.m. UTC | #2

On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> From: Andy Shevchenko <[e-mail]>
> 
> 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 <[e-mail]>
> Co-developed-by: Tony Lindgren <[e-mail]>
> Signed-off-by: Tony Lindgren <[e-mail]>
> ---
>  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

Tony Lindgren Dec. 9, 2021, 7:29 a.m. UTC | #3

Hi,

* Johan Hovold <[e-mail]> [211130 10:29]:
> On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > From: Andy Shevchenko <[e-mail]>
> > 
> > 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.

Yes that would be nice to do eventually :)

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

Good point.

> That is, shouldn't the suspend go after uart_change_pm()? And similar in
> other places.

Yes agreed, runtime PM may disable the clock and shut down the UART so
should be done after uart_change_pm().

BTW, Andy has follow-up patches to also drop the old uart_pm in favor of
runtime PM :)

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

As this is kernel console related we may be able to just leave out the
runtime PM calls here, see the two commits below. Andy, do you have some
comments?

> Can you expand on this and also on how this relates to console ports
> presumably never being runtime suspended?

See the following two commits for kernel console handling:

bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")
a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")

Thanks for looking through the patches again, I'll take a look at all
your comments and will repost after the merge window.

Regards,

Tony

Andy Shevchenko Dec. 9, 2021, 10:35 a.m. UTC | #4

On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> * Johan Hovold <[e-mail]> [211130 10:29]:
> > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > From: Andy Shevchenko <[e-mail]>
> > > 
> > > 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.
> 
> Yes that would be nice to do eventually :)
> 
> > > @@ -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?
> 
> Good point.
> 
> > That is, shouldn't the suspend go after uart_change_pm()? And similar in
> > other places.
> 
> Yes agreed, runtime PM may disable the clock and shut down the UART so
> should be done after uart_change_pm().
> 
> BTW, Andy has follow-up patches to also drop the old uart_pm in favor of
> runtime PM :)

Yeah, but they are just removals without any proper replacements where the
current drivers already are using some kind of runtime PM. I was focused and
able to test only 8250 part.

> > > @@ -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.
> 
> As this is kernel console related we may be able to just leave out the
> runtime PM calls here, see the two commits below. Andy, do you have some
> comments?

IIRC the uart_set_options() can be called during early boot stages where no
device is present and we need to handle those cases, otherwise it will be
KABOOMs. I.o.w. we3 may not get rid of those checks (at least in a simple way).

> > Can you expand on this and also on how this relates to console ports
> > presumably never being runtime suspended?
> 
> See the following two commits for kernel console handling:
> 
> bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")
> a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> 
> Thanks for looking through the patches again, I'll take a look at all
> your comments and will repost after the merge window.

Thanks, Tony, for moving this forward and Johan for good review!

Tony Lindgren Jan. 18, 2022, 9:41 a.m. UTC | #5

Hi,

* Andy Shevchenko <[e-mail]> [211209 10:37]:
> On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> > * Johan Hovold <[e-mail]> [211130 10:29]:
> > > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > > From: Andy Shevchenko <[e-mail]>
> > > > 
> > > > 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.
> > 
> > Yes that would be nice to do eventually :)

I think we should do the "third option" above as the first preparatory patch.
It can be done separately from the rest of the series, and we avoid adding
serial layer specific wrappers around runtime PM calls in the later patches.

Below is what I came up with for the preparatory patch, can you guys please
take a look and see if you have better ideas?

For system suspend and resume, it seems we don't need to do anything as
runtime PM is anyways disabled already in prepare.

Andy, care to give the following also a try for your serial port test
cases?

Regards,

Tony

8< --------------------
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
@@ -997,6 +997,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
@@ -1352,6 +1352,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>
@@ -184,6 +185,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		int init_hw)
 {
 	struct uart_port *uport = uart_port_check(state);
+	struct device *dev = uport->dev;
 	unsigned long flags;
 	unsigned long page;
 	int retval = 0;
@@ -191,6 +193,32 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 	if (uport->type == PORT_UNKNOWN)
 		return 1;
 
+	/* 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 handler the returned errors for runtime PM calls.
+	 */
+	if (!pm_runtime_enabled(dev)) {
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	} else {
+		uport->implements_pm_runtime = 1;
+	}
+
+	/*
+	 * Keep the port enabled unless autosuspend is supported.
+	 * Released in uart_shutdown().
+	 */
+	if (!uport->supports_autosuspend) {
+		retval = pm_runtime_resume_and_get(dev);
+		if (retval < 0)
+			return retval;
+	}
+
 	/*
 	 * Make sure the device is in D0 state.
 	 */
@@ -279,6 +307,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 {
 	struct uart_port *uport = uart_port_check(state);
 	struct tty_port *port = &state->port;
+	struct device *dev = uport->dev;
 	unsigned long flags;
 	char *xmit_buf = NULL;
 
@@ -313,6 +342,19 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 	 */
 	tty_port_set_suspended(port, 0);
 
+	/* Runtime PM paired with configuration done in uart_port_startup() */
+	if (uport) {
+		dev = uport->dev;
+		pm_runtime_dont_use_autosuspend(dev);
+		pm_suspend_ignore_children(dev, true);
+		if (!uport->supports_autosuspend)
+			pm_runtime_put_sync(dev);
+		if (!uport->implements_pm_runtime) {
+			pm_runtime_set_suspended(dev);
+			pm_runtime_disable(dev);
+		}
+	}
+
 	/*
 	 * Do not free() the transmit buffer page under the port lock since
 	 * this can create various circular locking scenarios. For instance,
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
@@ -249,6 +249,8 @@ struct uart_port {
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		suspended;
 	unsigned char		console_reinit;
+	unsigned long		implements_pm_runtime:1;
+	unsigned long		supports_autosuspend:1;
 	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) */

Andy Shevchenko April 11, 2022, 10:36 a.m. UTC | #6

First of all, my apology for the long delay in answering / commenting on this.
Second, Cc'ed to Ilpo.

On Tue, Jan 18, 2022 at 11:41:03AM +0200, Tony Lindgren wrote:
> * Andy Shevchenko <[e-mail]> [211209 10:37]:
> > On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> > > * Johan Hovold <[e-mail]> [211130 10:29]:
> > > > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > > > From: Andy Shevchenko <[e-mail]>
> > > > > 
> > > > > 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.
> > > 
> > > Yes that would be nice to do eventually :)
> 
> I think we should do the "third option" above as the first preparatory patch.
> It can be done separately from the rest of the series, and we avoid adding
> serial layer specific wrappers around runtime PM calls in the later patches.
> 
> Below is what I came up with for the preparatory patch, can you guys please
> take a look and see if you have better ideas?
> 
> For system suspend and resume, it seems we don't need to do anything as
> runtime PM is anyways disabled already in prepare.
> 
> Andy, care to give the following also a try for your serial port test
> cases?

I'm a bit off of the UART work nowadays, but luckily we have Ilpo who is
pretty much ramped up on the topic. Please, include him in all your work
WRT 8250 UART improvements. I hope Ilpo might have time to test the patch
mentioned in this message or give an idea how to do better, if possibly.

Tony Lindgren April 11, 2022, 10:55 a.m. UTC | #7

* Andy Shevchenko <[e-mail]> [220411 10:37]:
> I'm a bit off of the UART work nowadays, but luckily we have Ilpo who is
> pretty much ramped up on the topic. Please, include him in all your work
> WRT 8250 UART improvements. I hope Ilpo might have time to test the patch
> mentioned in this message or give an idea how to do better, if possibly.

OK, I'll be sending out a preparatory patch that starts managing serial
port devices in addition to serial port with serial_core.c.

Regards,

Tony

Patch

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);
+}
+
 /*
  * 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;
 
+	err = serial_pm_resume_and_get(port->dev);
+	if (err < 0)
+		return;
 	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;
 	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;
+		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;
 	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;
 		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);
 
@@ -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) {
+		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);
 
 		/*
 		 * 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);
 
+		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);
+
 		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);
 			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);
+		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);