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

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

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?

-- 
With Best Regards,
Andy Shevchenko