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

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

[Andy Shevchenko]

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)
+{
+	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()
-- 
2.27.0

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

[Marc Zyngier]

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

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

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200706 14:36]:
> Tony, can you also test that this doesn't change anything for the PM case for
> OMAP?

Still works for me just fine. Feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>

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

[Andy Shevchenko]

On Mon, Jul 06, 2020 at 05:36:56PM +0100, Marc Zyngier wrote:
> 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.

...

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

Actually I was thinking about that name, but decided to go without underscores
(b/c some developers like this approach, some like that one). I'm fine with
your proposal nevertheless.

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

Thanks!

-- 
With Best Regards,
Andy Shevchenko