linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Maxime Ripard <mripard@kernel.org>, Will Deacon <will@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/3] serial: 8250_dw: Fix clk-notifier/port suspend deadlock
Date: Tue, 29 Sep 2020 23:51:45 +0300	[thread overview]
Message-ID: <20200929205145.23itpxwns32uhjnx@mobilestation> (raw)
In-Reply-To: <e508b9c8-bcc4-00a6-0ca0-0b4a0c34f626@redhat.com>

Hello,

On Sun, Sep 27, 2020 at 05:01:52PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/23/20 6:19 PM, Serge Semin wrote:
> > Hans has discovered that there is a potential deadlock between the ref
> > clock change notifier and the port suspension procedures {see the link at
> > the bottom of the letter}. Indeed the deadlock is possible if the port
> > suspension is initiated during the ref clock rate change:
> > 
> >      CPU0 (suspend CPU/UART)   CPU1 (update clock)
> >               ----                    ----
> >      lock(&port->mutex);
> >                                lock((work_completion)(&data->clk_work));
> >                                lock(&port->mutex);
> >      lock((work_completion)(&data->clk_work));
> > 
> >      *** DEADLOCK ***
> > 
> > So the CPU performing the UART port shutdown procedure will wait until the
> > ref clock change notifier is finished (worker is flushed), while the later
> > will wait for a port mutex being released.
> > 
> > A possible solution to bypass the deadlock is to move the worker flush out
> > of the critical section protected by the TTY port mutex. For instance we
> > can register and de-register the clock change notifier in the port probe
> > and remove methods instead of having them called from the port
> > startup/shutdown callbacks. But in order to do that we need to make sure
> > that the serial8250_update_uartclk() method is safe to be used while the
> > port is shutted down. Alas the current implementation doesn't provide that
> > safety. The solution described above is introduced in the framework of
> > this patchset. See individual patches for details.
> > 
> > Link: https://lore.kernel.org/linux-serial/f1cd5c75-9cda-6896-a4e2-42c5bfc3f5c3@redhat.com
> > 
> > Hans, could you test the patchset out on your Cherry Trail (x86)-based
> > devices? After that we can merge it in into the kernels 5.8 and 5.9 if
> > there is no objections against the fix.
> 
> Done, I can confirm that this fixes the lockdep issue for me, so you
> can add my:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Great! Thank you very much.

Greg, could you merge the series in if you have no objection against the
solution design? Seeing the bug has been introduced together with the
original series integrated in the kernel 5.9, the fix provided by this
patchset will be only needed in 5.9.

-Sergey

> 
> To the entire series.
> 
> Regards,
> 
> Hans
> 

      reply	other threads:[~2020-09-29 20:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23 16:19 [PATCH 0/3] serial: 8250_dw: Fix clk-notifier/port suspend deadlock Serge Semin
2020-09-23 16:19 ` [PATCH 1/3] serial: 8250: Discard RTS/DTS setting from clock update method Serge Semin
2020-09-23 16:19 ` [PATCH 2/3] serial: 8250: Skip uninitialized TTY port baud rate update Serge Semin
2020-09-23 16:19 ` [PATCH 3/3] serial: 8250_dw: Fix clk-notifier/port suspend deadlock Serge Semin
2020-10-18  0:32   ` Jonathan Liu
2020-09-27 15:01 ` [PATCH 0/3] " Hans de Goede
2020-09-29 20:51   ` Serge Semin [this message]

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=20200929205145.23itpxwns32uhjnx@mobilestation \
    --to=sergey.semin@baikalelectronics.ru \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=jirislaby@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mripard@kernel.org \
    --cc=will@kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).