Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Johan Hovold <johan@kernel.org>]

On Mon, Jun 01, 2020 at 05:18:13PM -0700, Tony Lindgren wrote:
> We can get an imprecise external abort on uart_shutdown() at
> serial8250_do_set_mctrl() if the UART is autoidled.
> 
> We don't want to add PM runtime calls to serial8250_do_set_mctrl()
> beyond checking the usage count as it gets called from interrupts
> disabled and spinlock held from uart_update_mctrl().
> 
> We can just bail out early from serial8250_do_set_mctrl() if the UART
> is inactive. We have uart_shutdown() call uart_port_dtr_rts() with
> value of 0 just to disable DTR and RTS.

No, sorry. This is just putting another band-aid on this broken mess (I
never realised it was this bad).

As others have apparently already pointed out in the past, there are
paths that will end up calling sleeping pm_runtime_get_sync() in atomic
context (e.g serial8250_stop_tx()).

In other places this all seems to work mostly by chance by relying on
autosuspend keeping the clocks enabled long enough to not hit broken
paths (e.g. serial8250_do_set_mctrl()) which fail to enable clocks.

Note that uart_port_dtr_rts() is called from other paths, for example on
close and hangup, which would now fail to lower DTR/RTS as expected (it
still appears to work mostly by chance since there's later call in
serial8250_do_shutdown() which updates MCR to clear TIOCM_OUT2).

There's shouldn't be anything fundamental preventing you from adding the
missing resume calls to the mctrl paths even if it may require reworking
(and fixing) the whole RPM implementation (which would be a good thing
of course).

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2001,11 +2001,20 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>  	return serial8250_do_get_mctrl(port);
>  }
>  
> +/*
> + * Called from uart_update_mctrl() with spinlock held, so we don't want
> + * add PM runtime calls here beyond checking the usage count. If the
> + * UART is not active, we can just bail out early.
> + */
>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned char mcr;
>  
> +	if (up->capabilities & UART_CAP_RPM &&
> +	    !pm_runtime_get_if_in_use(up->port.dev))
> +		return;
> +
>  	if (port->rs485.flags & SER_RS485_ENABLED) {
>  		if (serial8250_in_MCR(up) & UART_MCR_RTS)
>  			mctrl |= TIOCM_RTS;
> @@ -2018,6 +2027,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
>  
>  	serial8250_out_MCR(up, mcr);
> +
> +	if (up->capabilities & UART_CAP_RPM)
> +		pm_runtime_put(up->port.dev);
>  }
>  EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);

Johan