Re: [PATCH 2/2] serial: core: fix console port-lock regression

[Andy Shevchenko]

+Cc: Tony, let me add Tony to the discussion.

On Thu, Sep 10, 2020 at 09:35:27AM +0200, Johan Hovold wrote:
> On Wed, Sep 09, 2020 at 06:48:15PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 09, 2020 at 04:31:01PM +0200, Johan Hovold wrote:
> > > Fix the port-lock initialisation regression introduced by commit
> > > a3cb39d258ef ("serial: core: Allow detach and attach serial device for
> > > console") by making sure that the lock is again initialised during
> > > console setup.
> > > 
> > > The console may be registered before the serial controller has been
> > > probed in which case the port lock needs to be initialised during
> > > console setup by a call to uart_set_options(). The console-detach
> > > changes introduced a regression in several drivers by effectively
> > > removing that initialisation by not initialising the lock when the port
> > > is used as a console (which is always the case during console setup).
> > > 
> > > Add back the early lock initialisation and instead use a new
> > > console-reinit flag to handle the case where a console is being
> > > re-attached through sysfs.
> > > 
> > > The question whether the console-detach interface should have been added
> > > in the first place is left for another discussion.
> > 
> > It was discussed in [1]. TL;DR: OMAP would like to keep runtime PM available
> > for UART while at the same time we disable it for kernel consoles in
> > bedb404e91bb.
> > 
> > [1]: https://lists.openwall.net/linux-kernel/2018/09/29/65
> 
> Yeah, I remember that. My fear is just that the new interface opens up a
> can of worms as it removes the earlier assumption that the console would
> essentially never be deregistered without really fixing all those
> drivers, and core functions, written under that assumption. Just to
> mention a few issues; we have drivers enabling clocks and other
> resources during console setup which can now be done repeatedly,

The series introduced the console ->exit() callback, so it should be easy to
fix.

>	and
> several drivers whose setup callbacks are marked __init and will oops
> the minute you reattach the console.

I believe this can be fixed relatively easy. As a last resort it can be a quirk
that disables console detachment for problematic consoles.

> And what about power management
> which was the reason for wanting this on OMAP in the first place; tty
> core never calls shutdown() for a console port, not even when it's been
> detached using the new interface.

That is interesting... Tony, do we have OMAP case working because of luck?

> I know, the console setup is all a mess, but this still seems a little
> rushed to me. I'm even inclined to suggest a revert until the above and
> similar issues have been addressed properly rather keeping a known buggy
> interface.

You know that it will be a dead end. Any solution how to move forward?

> > > Note that the console-enabled check in uart_set_options() is not
> > > redundant because of kgdboc, which can end up reinitialising an already
> > > enabled console (see commit 42b6a1baa3ec ("serial_core: Don't
> > > re-initialize a previously initialized spinlock.")).

-- 
With Best Regards,
Andy Shevchenko