Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Johan Hovold <johan@kernel.org>, Tony Lindgren <tony@atomide.com>
Cc: 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 12:27:15 +0300
Message-ID: <20200910092715.GM1891694@smile.fi.intel.com> (raw)
In-Reply-To: <20200910073527.GC24441@localhost>

+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



  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-09 14:30 [PATCH 0/2] " Johan Hovold
2020-09-09 14:31 ` [PATCH 1/2] serial: core: fix port-lock initialisation Johan Hovold
     [not found]   ` <20200909152158.GC1891694@smile.fi.intel.com>
2020-09-10  7:08     ` Johan Hovold
2020-09-09 14:31 ` [PATCH 2/2] serial: core: fix console port-lock regression Johan Hovold
     [not found]   ` <20200909154815.GD1891694@smile.fi.intel.com>
2020-09-10  7:35     ` Johan Hovold
2020-09-10  9:27       ` Andy Shevchenko [this message]
2020-09-10 11:10         ` Johan Hovold
2020-09-14  8:09         ` Tony Lindgren
2020-09-16 12:23           ` Andy Shevchenko
2020-09-18  7:16             ` Tony Lindgren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200910092715.GM1891694@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git