Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200423220056.29450-1-john.stultz@linaro.org>
 <jhj1rodyeu1.mognet@arm.com> <CALAqxLW+CBMxj_5gCF5yLcX8dhM7Fg6oOL-zot0ZZT6PW6R04g@mail.gmail.com>
 <jhj1ro9bzhg.mognet@arm.com> <CAHp75VeE_J-GE9o6QVxBk6RJ2fjSwATfR1etaT0CXCgAiidjPQ@mail.gmail.com>
 <jhjimhkrnw1.mognet@arm.com>
In-Reply-To: <jhjimhkrnw1.mognet@arm.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 28 Apr 2020 13:03:25 +0300
Message-ID: <CAHp75VfJCVh0HRw4G8o0603XEJe6LdUnvrrugxgU0oyOWRCBPA@mail.gmail.com>
Subject: Re: [RFC][PATCH] serial: amba-pl011: Make sure we initialize the
 port.lock spinlock
To:     Valentin Schneider <valentin.schneider@arm.com>
Cc:     John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 28, 2020 at 11:54 AM Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
> On 27/04/20 10:02, Andy Shevchenko wrote:
> >> 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").
>
> Thanks for the pointer. I'm still a puzzled as to why it goes fine on one
> board and not on another, but at this point I don't have any better
> suggestion than the unconditional init.

My patch relied on the behaviour of 8250 [1] and that comment (near to
spin lock initialization routine).
It seems AMBA UART drivers unconditionally assign consoles ([2], [3])
without registering it properly at console_initcall().

Least invasive fix is what John's patch does, but real fix is to do
something like 8250 does.

So, the rule of thumb is simple: if we assign console to the port we
must initialize the lock even if we are not registering console.
I dunno the history of different behaviours among drivers and what
change(s) brought us to the messy spin lock initialization code in
them.

[1]: https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/tty/serial/8250/8250_core.c#L684
[2]: https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/tty/serial/amba-pl010.c#L691
[3]: https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/tty/serial/amba-pl011.c#L2496

-- 
With Best Regards,
Andy Shevchenko