From: Sergey Semin <Sergey.Semin@baikalelectronics.ru>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <maxime@cerno.tech>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.com>,
Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>,
Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>,
Vadim Vlasov <V.Vlasov@baikalelectronics.ru>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Paul Burton <paulburton@kernel.org>,
Ralf Baechle <ralf@linux-mips.org>, Chen-Yu Tsai <wens@csie.org>,
Ray Jui <rjui@broadcom.com>,
Scott Branden <sbranden@broadcom.com>,
Florian Fainelli <f.fainelli@gmail.com>,
Wei Xu <xuwei5@hisilicon.com>,
Jason Cooper <jason@lakedaemon.net>, Andrew Lunn <andrew@lunn.ch>,
Gregory Clement <gregory.clement@bootlin.com>,
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
Jisheng Zhang <Jisheng.Zhang@synaptics.com>,
Heiko Stuebner <heiko@sntech.de>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Russell King <linux@armlinux.org.uk>,
<linux-arm-kernel@lists.infradead.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>, <linux-clk@vger.kernel.org>,
Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
<linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition
Date: Wed, 25 Mar 2020 20:11:09 +0300 [thread overview]
Message-ID: <20200325171109.cohnsw3s57ckaqud@ubsrv2.baikal.int> (raw)
In-Reply-To: <20200324101243.GG1922688@smile.fi.intel.com>
On Tue, Mar 24, 2020 at 12:12:43PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote:
> > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote:
> > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > >
> > > > There are races possible in the dw8250_set_termios() callback method
> > > > and while the device is in PM suspend state. A race condition may
> > > > happen if the baudrate clock source device is shared with some other
> > > > device (in our machine it's another DW UART port). In this case if that
> > > > device changes the clock rate while serial console is using it the
> > > > DW 8250 UART port might not only end up with an invalid uartclk value
> > > > saved, but may also experience a distorted output data since baud-clock
> > > > could have been changed. In order to fix this lets enable an exclusive
> > > > reference clock rate access in case if "baudclk" device is specified.
> > > >
> > > > So if some other device also acquires the rate exclusivity during the
> > > > time of a DW UART 8250 port being opened, then DW UART 8250 driver
> > > > won't be able to alter the baud-clock. It shall just use the available
> > > > clock rate. Similarly another device also won't manage to change the
> > > > rate at that time. If nothing else have the exclusive rate access
> > > > acquired except DW UART 8250 driver, then the driver will be able to
> > > > alter the rate as much as it needs to in accordance with the currently
> > > > implemented logic.
>
> > > clk_rate_exclusive_get is pretty intrusive, and due to the usual
> > > topology of clock trees, this will lock down 3-4 parent clocks to
> > > their current rate as well. In the Allwinner SoCs case for example,
> > > this will lock down the same PLL than the one used by the CPU,
> > > preventing cpufreq from running.
> >
> > Speaking about weak design of a SoC' clock tree. Our problems are nothing
> > with respect to the Allwinner SoC, in which case of changing the
> > CPU-frequency may cause the UART glitches subsequently causing data
> > transfer artefacts.) Moreover as I can see the same issue may raise for
> > I2C, QSPI, PWM devices there.
> >
> > Anyway your concern does make sense.
> >
> > > However, the 8250 has a pretty wide range of dividers and can adapt to
> > > any reasonable parent clock rate, so we don't really need to lock the
> > > rate either, we can simply react to a parent clock rate change using
> > > the clock notifiers, just like the SiFive UART is doing.
> > >
> > > I tried to do that, but given that I don't really have an extensive
> > > knowledge of the 8250, I couldn't find a way to stop the TX of chars
> > > while we change the clock rate. I'm not sure if this is a big deal or
> > > not, the SiFive UART doesn't seem to care.
> >
> > Yes, your solution is also possible, but even in case of stopping Tx/Rx it
> > doesn't lack drawbacks. First of all AFAIK there is no easy way to just
> > pause the transfers. We'd have to first wait for the current transfers
> > to be completed, then somehow lock the port usage (both Tx and Rx
> > traffic), permit the reference clock rate change, accordingly adjust the
> > UART clock divider, and finally unlock the port. While if we don't mind
> > to occasionally have UART data glitches, we can just adjust the UART ref
> > divider synchronously with ref clock rate change as you and SiFive UART
> > driver suggest.
> >
> > So we are now at a zugzwang - a fork to three not that good solutions:
> > 1) lock the whole clock branch and provide a glitchless interfaces. But
> > by doing so we may (in case of Allwinner SoCs we will) lockup some very
> > important functionality like CPU-frequency change while the UART port is
> > started up. In this case we won't have the data glitches.
> > 2) just adjust the UART clock divider in case of reference clock rate
> > change (use the SiFive UART driver approach). In this case we may have the
> > data corruption.
> > 3) somehow implement the algo: wait for the transfers to be completed,
> > lock UART interface (it's possible for Tx, but for Rx in case of no handshake
> > enabled it's simply impossible), permit the ref clock rate change,
> > adjust the UART divider, then unlock the UART interface. In this case the data
> > glitches still may happen (if no modem control is available or
> > handshakes are disabled).
> >
> > As for the cases of Baikal-T1 UARTs the first solutions is the most suitable.
> > We don't lock anything valuable, since a base PLL output isn't directly
> > connected to any device and it's rate once setup isn't changed during the
> > system running. On the other hand I don't mind to implement the second
> > solution, even though it's prone to data glitches. Regarding the solution
> > 3) I won't even try. It's too complicated, I don't have time and
> > test-infrastructure for this.
> >
> > So Andy what do you think?
>
> From Intel HW perspective the first two are okay, but since Maxime is against
> first, you have the only option from your list. Perhaps somebody may give
> option 4) here...
>
Ok then. I'll implement the option 2) in v3 if noone gives any alternatives
before that.
Regards,
-Sergey
> --
> With Best Regards,
> Andy Shevchenko
>
>
next prev parent reply other threads:[~2020-03-25 17:11 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200306130231.05BBC8030795@mail.baikalelectronics.ru>
[not found] ` <20200306134049.86F8180307C2@mail.baikalelectronics.ru>
[not found] ` <20200310001444.B0F50803087C@mail.baikalelectronics.ru>
2020-03-10 14:17 ` [PATCH] serial: 8250_dw: Fix common clocks usage race condition Andy Shevchenko
[not found] ` <20200310141715.7063280307CA@mail.baikalelectronics.ru>
2020-03-12 18:47 ` Sergey Semin
2020-03-18 15:19 ` Andy Shevchenko
2020-03-23 2:46 ` [PATCH v2] " Sergey.Semin
2020-03-23 9:20 ` Andy Shevchenko
2020-03-23 11:11 ` Sergey Semin
2020-03-23 11:52 ` Andy Shevchenko
2020-03-23 17:07 ` Sergey Semin
2020-03-23 10:01 ` Maxime Ripard
2020-03-23 13:50 ` Sergey Semin
2020-03-24 9:41 ` Maxime Ripard
2020-03-24 10:12 ` Andy Shevchenko
2020-03-25 17:11 ` Sergey Semin [this message]
2020-03-26 1:44 ` Stephen Boyd
2020-03-27 9:12 ` Sergey Semin
2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin
2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin
2020-05-15 13:48 ` Andy Shevchenko
2020-05-06 23:31 ` [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method Serge Semin
2020-05-15 12:45 ` Greg Kroah-Hartman
2020-05-15 14:32 ` Serge Semin
2020-05-06 23:31 ` [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin
2020-05-15 14:05 ` Andy Shevchenko
2020-05-15 14:50 ` Serge Semin
2020-05-15 15:05 ` Andy Shevchenko
2020-05-06 23:31 ` [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition Serge Semin
2020-05-15 14:10 ` Andy Shevchenko
2020-05-15 15:19 ` Serge Semin
2020-05-07 18:29 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
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=20200325171109.cohnsw3s57ckaqud@ubsrv2.baikal.int \
--to=sergey.semin@baikalelectronics.ru \
--cc=Alexey.Malahov@baikalelectronics.ru \
--cc=Ekaterina.Skachko@baikalelectronics.ru \
--cc=Jisheng.Zhang@synaptics.com \
--cc=Maxim.Kaurkin@baikalelectronics.ru \
--cc=Pavel.Parkhomenko@baikalelectronics.ru \
--cc=Ramil.Zaripov@baikalelectronics.ru \
--cc=V.Vlasov@baikalelectronics.ru \
--cc=andrew@lunn.ch \
--cc=andriy.shevchenko@linux.intel.com \
--cc=catalin.marinas@arm.com \
--cc=f.fainelli@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=gregory.clement@bootlin.com \
--cc=heikki.krogerus@linux.intel.com \
--cc=heiko@sntech.de \
--cc=jason@lakedaemon.net \
--cc=jslaby@suse.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=maxime@cerno.tech \
--cc=mturquette@baylibre.com \
--cc=paulburton@kernel.org \
--cc=ralf@linux-mips.org \
--cc=rjui@broadcom.com \
--cc=sboyd@kernel.org \
--cc=sbranden@broadcom.com \
--cc=sebastian.hesselbarth@gmail.com \
--cc=tsbogend@alpha.franken.de \
--cc=wangkefeng.wang@huawei.com \
--cc=wens@csie.org \
--cc=will@kernel.org \
--cc=xuwei5@hisilicon.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
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).