Re: [RFC][PATCH] serial: amba-pl011: Make sure we initialize the port.lock spinlock

[Andy Shevchenko]

On Mon, Apr 27, 2020 at 2:29 AM Valentin Schneider
<valentin.schneider@xxxxxxx> wrote:
>
>
> On 25/04/20 05:04, John Stultz wrote:
> > On Thu, Apr 23, 2020 at 4:14 PM Valentin Schneider
> > <valentin.schneider@xxxxxxx> wrote:
> >> On 23/04/20 23:00, John Stultz wrote:
> >> > Which seems to be due to the fact that after allocating the uap
> >> > structure, the pl011 code doesn't initialize the spinlock.
> >> >
> >> > This patch fixes it by initializing the spinlock and the warning
> >> > has gone away.
> >> >
> >>
> >> Thanks for having a look. It does seem like the reasonable thing to do, and
> >> I no longer get the warning on h960.
> >>
> >> That said, I got more curious as this doesn't show up on my Juno (same
> >> Image). Digging into it I see that uart_add_one_port() has a call to
> >> uart_port_spin_lock_init() a few lines before uart_configure_port() (in
> >> which the above warning gets triggered). That thing says:
> >>
> >>  * Ensure that the serial console lock is initialised early.
> >>  * If this port is a console, then the spinlock is already initialised.
> >>
> >> Which requires me to ask: are we doing the right thing here?
> >
> > So I got a little bit of time to look at this before I got pulled off
> > to other things (and now its Friday night, so I figured I'd reply
> > before I forget it on Monday).
> >
> > I did check and lockdep is tripping when we add ttyAMA6 which is the
> > serial console on the board. I wasn't able to trace back to why we
> > hadn't already called spin_lock_init() in the console code, but it
> > seems we haven't.
> >
>
> So on the Juno (ttyAMA0), the first time I see us hitting
> uart_port_spin_lock_init() is via:
>
>   uart_add_one_port() ->  uart_port_spin_lock_init()
>
> Since port->cons->(index, line) is (-1, 0) at this point in time,
> uart_console(port) returns false and we init the spinlock. When then
> happily trickle down to uart_configure_port() -> register_console()
>
>
> On the Hikey960 (ttyAMA6) I see the same initial flow, but (index, line)
> is (6, 6), so uart_console(port) returns true and we skip the
> spin_lock_init(). When then hit the splat on the rest of the way down
> uart_add_one_port().
>
>
> I did a tiny bit of git spelunking; I found a commit that changed
> uart_console_enabled() into uart_console() within
> uart_port_spin_lock_init():
>
>   a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
>
> Reverting just that one change in uart_port_spin_lock_init() seems to go
> fine on both Juno & HiKey960, but I think that doesn't play well with the
> rest of the aforementioned commit. I think that this initial (index, line)
> tuple is to blame, though I've added Andy in Cc just in case.

The above mentioned commit reveals the issue in the code which doesn't
register console properly.

See what I put in 0f87aa66e8c31 ("serial: sunhv: Initialize lock for
non-registered console").

-- 
With Best Regards,
Andy Shevchenko