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

From: Johan Hovold <johan@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Johan Hovold <johan@kernel.org>, Tony Lindgren <tony@atomide.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH 2/2] serial: core: fix console port-lock regression
Date: Thu, 10 Sep 2020 13:10:15 +0200	[thread overview]
Message-ID: <20200910111015.GE24441@localhost> (raw)
In-Reply-To: <20200910092715.GM1891694@smile.fi.intel.com>

On Thu, Sep 10, 2020 at 12:27:15PM +0300, Andy Shevchenko wrote:
> +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.

I'm not saying it's necessarily hard, I'm suggesting it should have been
considered before merging. But there could still be complications as the
console have always been considered a special case that's been hacked
around.

> >	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.

Sure, but just removing the __init without vetting the drivers and
testing the new interface may not be much better than letting them oops.

> > 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?

I guess that depends on how broken this is. I only gave a few examples
of the top of my head of issues that needs to be considered. 

But if this is to stay then making the feature opt-in by only exposing
the attribute for console drivers that implement the new exit() callback
or some other serial-driver flag might work.

Johan