Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Jul 04, 2020 at 06:43:26PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 03, 2020 at 01:31:24PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Jul 2, 2020 at 4:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > On Mon, Feb 17, 2020 at 01:40:12PM +0200, Andy Shevchenko wrote:
> > > > > In the future we would like to disable power management on the serial devices
> > > > > used as kernel consoles to avoid weird behaviour in some cases. However,
> > > > > disabling PM may prevent system to go to deep sleep states, which in its turn
> > > > > leads to the higher power consumption.
> > > > >
> > > > > Tony Lindgren proposed a work around, i.e. allow user to detach such consoles
> > > > > to make PM working again. In case user wants to see what's going on, it also
> > > > > provides a mechanism to attach console back.
> > > > >
> > > > > Link: https://lists.openwall.net/linux-kernel/2018/09/29/65
> > > > > Suggested-by: Tony Lindgren <tony@atomide.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > > > --- a/drivers/tty/serial/serial_core.c
> > > > > +++ b/drivers/tty/serial/serial_core.c
> > > > > @@ -1919,7 +1919,7 @@ static inline bool uart_console_enabled(struct uart_port *port)
> > > > >   */
> > > > >  static inline void uart_port_spin_lock_init(struct uart_port *port)
> > > > >  {
> > > > > -     if (uart_console_enabled(port))
> > > > > +     if (uart_console(port))
> > > >
> > > > This results in lockdep splashes such as the one attached below. Is there
> > >
> > > Or "BUG: spinlock bad magic on CPU#3, swapper/0/1", cfr. [1].
> > > So far I hadn't noticed that, as the issue only shows up when using the
> > > legacy way of passing a "console=ttyS*" kernel command line parameter,
> > > and not when relying on the modern "chosen/stdout-path" DT property.
> > >
> > > > any special reason for this change ? It is not really explained in the
> > > > commit description.
> > >
> > > Indeed. Why this change?
> > >
> > > I also don't agree with your typical fix for drivers, which is like:
> > >
> > >     @@ -567,6 +567,9 @@ static int hv_probe(struct platform_device *op)
> > >             sunserial_console_match(&sunhv_console, op->dev.of_node,
> > >                                     &sunhv_reg, port->line, false);
> > >
> > >     +       /* We need to initialize lock even for non-registered console */
> > >     +       spin_lock_init(&port->lock);
> > >     +
> > >             err = uart_add_one_port(&sunhv_reg, port);
> > >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >                   calls uart_port_spin_lock_init()
> > >
> > >             if (err)
> > >                     goto out_unregister_driver;
> > >
> > > as this initializes the spinlock twice for non-console= ports.
> > 
> > I had a deeper look...
> > 
> >     /*
> >      * Ensure that the serial console lock is initialised early.
> >      * If this port is a console, then the spinlock is already initialised.
> >      */
> >     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);
> >     }
> > 
> > So according to the comment, the spinlock is assumed to be already
> > initialized, as the port is already in use as a console.  Makes sense.
> 
> Thanks, Geert! Yes, the change makes code aligned with a comment. I did it due
> to some issues with attaching / detaching consoles (I can try to reproduce
> later, perhaps next week, I'm a bit limited now to fulfil kernel work /
> testing).
> 
> > Now, where should it be initialized?
> >   1. For modern DT systems, chosen/stdout-path is used, and the spinlock
> >      is initialized in register_earlycon(), just before calling
> >      register_console(). And everything's fine.
> > 
> >   2. With "console=" (even on DT systems with chosen/stdout-path),
> >      the serial console must gets registered differently.
> >      Naively, I assumed that's done in the serial driver, but apparently
> >      that is no longer the case: the single register_console() call in
> >      drivers/tty/serial/sh-sci.c is used on legacy SuperH only.
> >      So we're back to drivers/tty/serial/serial_core.c, which calls
> >      register_console(), but does so _after_ taking the spinlock:
> > 
> >          uart_add_one_port()
> >              uart_port_spin_lock_init() /* skips spin_lock_init()! */
> >              uart_configure_port()
> >                  spin_lock_irqsave(&port->lock, flags); /* BUG! */
> >                  register_console())
> > 
> > So who's to blame for _not_ initializing the spinlock?
> 
> This is a very good question. Code is so old and I don't know why we have such
> interesting implementation among serial drivers. The 8250 does this
> initialisation at console_initcall() when it *properly* calls
> register_console() before adding port (yet).
> 
>     /*
>      * If this driver supports console, and it hasn't been
>      * successfully registered yet, try to re-register it.
>      * It may be that the port was not available.
>      */
>     if (port->cons && !(port->cons->flags & CON_ENABLED))
>             register_console(port->cons);
> 
> Seems like a chicken-egg problem. Any advice?

This comment even more interesting...

    /*
     * Ensure that the modem control lines are de-activated.
     * keep the DTR setting that is set in uart_set_options()
     * We probably don't need a spinlock around this, but
     */

Investigation shows that this comes from (see history.git from history group)
commit 33c0d1b0c3ebb61243d9b19ce70d9063acff2aac
Author: Russell King <rmk@arm.linux.org.uk>
Date:   Sun Jul 21 02:39:46 2002 -0700

    [PATCH] Serial driver stuff

According to documentation ->set_mctrl() is called with lock taken. Hmm...

So, removing that spin lock and moving spin lock initialisation call after
uart_configure_port should fix this for all. But I'm not sure we don't break
anything.

From d361b1b35f4e8d318c584f224d67240b775fbe7f Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Sat, 4 Jul 2020 19:30:39 +0300
Subject: [PATCH 1/1] serial: core: Drop ambiguous spin lock in
 uart_configure_port()

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3cc183acf7ba..b4ed1e20dd5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2374,11 +2374,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		/*
 		 * Ensure that the modem control lines are de-activated.
 		 * keep the DTR setting that is set in uart_set_options()
-		 * We probably don't need a spinlock around this, but
 		 */
-		spin_lock_irqsave(&port->lock, flags);
 		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
-		spin_unlock_irqrestore(&port->lock, flags);
 
 		/*
 		 * If this driver supports console, and it hasn't been
@@ -2886,8 +2883,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 
-	uart_port_spin_lock_init(uport);
-
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 
@@ -2896,6 +2891,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 
 	port->console = uart_console(uport);
 
+	uart_port_spin_lock_init(uport);
+
 	num_groups = 2;
 	if (uport->attr_group)
 		num_groups++;

-- 
With Best Regards,
Andy Shevchenko