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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

On Thu, Jul 2, 2020 at 4:48 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> > > --- 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.
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?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux