Re: [PATCH v2] serial: core: Initialise spin lock before use in uart_configure_port()

[Marc Zyngier <maz@kernel.org>]

On 2020-07-06 15:35, Andy Shevchenko wrote:
> The comment near to uart_port_spin_lock_init() says:
> 
>   Ensure that the serial console lock is initialised early.
>   If this port is a console, then the spinlock is already initialised.
> 
> and there is nothing about enabled or disabled consoles. The commit
> a3cb39d258ef ("serial: core: Allow detach and attach serial device
> for console") made a change, which follows the comment, and also to
> prevent reinitialisation of the lock in use, when user detaches and
> attaches back the same console device. But this change discovers
> another issue, that uart_add_one_port() tries to access a spin lock
> that now may be uninitialised. This happens when a driver expects
> the serial core to register a console on its behalf. In this case
> we must initialise a spin lock before use.
> 
> Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial
> device for console")
> Reported-by: Marc Zyngier <maz@kernel.org>
> Reported-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Reported-by: Anatoly Pugachev <matorola@gmail.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: my bad to send the version with one typo, now squashed and resent
> 
> I hope this is now properly root caused. To the people in Reported-by 
> list, can
> you revert the corresponding change you either reported or submitted to 
> the
> certain serial driver and apply this patch and retest?
> 
> Tony, can you also test that this doesn't change anything for the PM 
> case for
> OMAP?
> 
> Geert, I tried to explain above why the change had been made in the 
> first place.
> 
>  drivers/tty/serial/serial_core.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c 
> b/drivers/tty/serial/serial_core.c
> index 3cc183acf7ba..55f9615f0b50 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1915,6 +1915,12 @@ static inline bool uart_console_enabled(struct
> uart_port *port)
>  	return uart_console(port) && (port->cons->flags & CON_ENABLED);
>  }
> 
> +static inline void uart_any_port_spin_lock_init(struct uart_port 
> *port)

nit: __uart_port_spin_lock_init() looks like a better name to me
(as a primitive of uart_port_spin_lock_init). You can also drop
the inline which doesn't mean much these days (and this isn't
a hot path).

> +{
> +	spin_lock_init(&port->lock);
> +	lockdep_set_class(&port->lock, &port_lock_key);
> +}
> +
>  /*
>   * Ensure that the serial console lock is initialised early.
>   * If this port is a console, then the spinlock is already 
> initialised.
> @@ -1924,8 +1930,7 @@ static inline void
> uart_port_spin_lock_init(struct uart_port *port)
>  	if (uart_console(port))
>  		return;
> 
> -	spin_lock_init(&port->lock);
> -	lockdep_set_class(&port->lock, &port_lock_key);
> +	uart_any_port_spin_lock_init(port);
>  }
> 
>  #if defined(CONFIG_SERIAL_CORE_CONSOLE) || 
> defined(CONFIG_CONSOLE_POLL)
> @@ -2371,6 +2376,13 @@ uart_configure_port(struct uart_driver *drv,
> struct uart_state *state,
>  		/* Power up port for set_mctrl() */
>  		uart_change_pm(state, UART_PM_STATE_ON);
> 
> +		/*
> +		 * If this driver supports console, and it hasn't been
> +		 * successfully registered yet, initialise spin lock for it.
> +		 */
> +		if (port->cons && !(port->cons->flags & CON_ENABLED))
> +			uart_any_port_spin_lock_init(port);
> +
>  		/*
>  		 * Ensure that the modem control lines are de-activated.
>  		 * keep the DTR setting that is set in uart_set_options()

Otherwise looks OK to me (having tested an earlier version).
With the above addressed:

Acked-by: Marc Zyngier <maz@kernel.org>

         M.
-- 
Jazz is not dead. It just smells funny...