Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Sergey Semin <Sergey.Semin@baikalelectronics.ru>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: <linux-serial@vger.kernel.org>,
	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>
Subject: Re: [PATCH] serial: 8250_dw: Fix common clocks usage race condition
Date: Thu, 12 Mar 2020 21:47:02 +0300
Message-ID: <20200312184450.co5o6zm6qniptfuv@ubsrv2.baikal.int> (raw)
In-Reply-To: <20200310141715.7063280307CA@mail.baikalelectronics.ru>

On Tue, Mar 10, 2020 at 04:17:08PM +0200, Andy Shevchenko wrote:
> It's pity we are discussing this off-list.
> Cc linux-serial@ now and leaving original writings as is.
> 
> On Tue, Mar 10, 2020 at 03:13:53AM +0300, Sergey Semin wrote:
> > On Fri, Mar 06, 2020 at 03:40:44PM +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 06, 2020 at 04:02:20PM +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 clocks 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 clocks rate while serial console is using it the
> > > > DW 8250 UART port might not only end up with an invalid uartclk value
> > > > saved, but might also experience a distorted output data since
> > > > baud-clock might have been changed. In order to fix this lets enable
> > > > an exclusive clocks rate access in case if "baudclk" device is
> > > > specified and is successfully enabled.
> > > > 
> > > > So if some other device also acquires the rate exclusivity, then
> > > > DW UART 8250 driver won't be able to alter the baud-clock. It shall
> > > > just use the available clocks rate. Similarly another device also
> > > > won't manage to change the rate. 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 needs to in accordance
> > > > with the currently implemented logic.
> > > 
> > > Thank you for the patch.
> > > 
> > > I honestly consider that this is kinda bad hardware design. If two devices are
> > > fighting for the best rate, there is a room to make the second one always miss.
> > > 
> > > (Imagine 1834200 UART clock and first asked for 115200, while second
> > >  desperately tries for 4M).
> > > 
> > > I think that the clock driver of the corresponding platform should actually
> > > keep track of the users and give the best for both.
> > > 
> > 
> > Hmm, this is discussable.
> > 
> > While I do agree that two serial ports using a common reference clock
> > was a bad hw design (it was nothing for the hw designers to add an
> > extra clock divider), and do understand the concern you mentioned in the
> > example I disagree that the hw clock driver should handle the clock usage
> > races. There is a possible alternative but the current solution seemed
> > to me most suitable and here is why.
> > 
> > First of all, if I implemented something like a users tracking algo in
> > the hw clock driver, It would be similar to the already developed and
> > suggested to be used in this patch clock exclusivity approach anyway,
> > since there would be no way to change the clock rate behind the uart drivers
> > back to adjust the clock frequency so making it suitable for both users.
> > In this case we would have the same race condition you described in the
> > parentheses.
> > 
> > Secondly why would we need to have the clk_rate_exlusive_{get,put}()
> > interface exported from the clk subsystem anyway if not for the cases
> > like described in this commit message? These functions were designed for
> > devices, which require the exclusive rate lock at some point. As for me
> > I would have added the clock rate exclusive lock to all the drivers
> > handling devices (not only UARTs), which depend not only on a clock being
> > enabled but also on it having a particular rate value. But this is too
> > much work, which alas I don't have time for at the moment.
> > 
> > Thirdly. Much easier solution would be to pretend, that the actual clock
> > device doesn't provide the rate change capability and work with a value
> > pre-initialized in the clock divider register (this is necessary because
> > on most of known to me Baikal-T1-based platforms one of these UARTs is
> > always used as a system/boot serial console). But in this case we would
> > have to artificially bind a different driver to the clock divider interface,
> > which wouldn't reflect the actual clock capability.
> > 
> > Finally in my opinion the suggested in this patch solution is the most
> > suitable. It solves the problem on the common clock interface usage
> > by two independent UART ports. It also provides a portable solution for
> > platforms, which have the similar problem. And it doesn't affect the
> > devices lacking the described clock rate race condition.
> > 
> > By the way I came up with an upgrade, which would make the solution even
> > better. Instead of using the clock exclusive rate lock/unlock in the
> > probe()/remove() methods, we could be calling them from the UART-port
> > startup()/shutdown() functions instead. In this case the clock rate would
> > be fixed only if both ports are opened at the same time. This would
> > lighten the limitations on the rates change conditions. What do you think
> > of this?
> 
> The use of clk_set_rate_exclusive()/clk_rate_exclusive_put() seems better.
> But, while from our (Intel) perspective this looks like no-op, for others,
> especially zillions of ARM boards using DesignWare UART it might be crucial.
> 

Using clk_set_rate_exclusive() isn't that appropriate at the port->startup()
callback. Firstly because it may fail due to not being able to update the rate,
then I would have to try to perform just clk_rate_exclusive_get(),
retrieve currently set clock rate and update the port uartclk field. Secondly
an attempt to update the rate will be performed in set_termios() anyway,
so the calling of clk_set_rate() in the port->startup() is redundant. Thirdly
I also can't use  clk_set_rate_exclusive() in set_termios(), because it doesn't
have some antagonist like unset_termios() to perform the same number of
clk_rate_exclusive_put() to appropriately free the exclusive lock of the clock.
And I don't want to manually calculate the numbers of exclusive locking.

So the easiest way would be to just lock the reference clock in the
port->startup() method and update the port->uartclk field with current
reference clock rate. Clock unlocking will be performed in the port-shutdown()
callback. I'll send v2 patch with this solution. You'll see what it
looks like.

> I would suggest to Cc new version to some ARM people (homework: filter
> MAINTAINERS along with `git log -- drivers/tty/serial/8250/8520_dw.c`
> to see who may have relation to this driver).
> 

Nice homework. Although I'd prefer to utilize the get_maintainers.pl script
for the same purpose. It has the --git argument for it, which also tunable
for parameters like deepness of the git-history, number of signatures, number
of maintainers, etc.

> > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > 
> > > Why second SoB is here, it's not the same as the submitter's (usually,
> > > submitter's one goes last in the chain)? Commit message or another tags,
> > > like Co-developed-by, doesn't shed a light either.
> > 
> > Alexey as being our team-leader was in these patches delivery path, which
> > according to [1] permits to add the SoB tag with his address:
> > 
> > "The Signed-off-by: tag indicates that the signer was involved in the
> > development of the patch, or that he/she was in the patch’s delivery path."
> > 
> > So the SoB signify that all the changes provided by the patches I've sent
> > and will yet send have been passed through his revision and can be
> > opened at the corresponding license.
> > 
> > [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
> 
> Yes, and AFAIU the last person in a chain submits the work.
> 
> So, it should reflect how patch went thru people's trees. If you send from your
> tree, where patches are created originally, there shouldn't be other SoBs (use
> suitable Reviewed-by, etc). At least this is safest side. However, better to
> ask maintainer (Greg KH in this case) and other senior developers.
> 

Ok. Last time I read the patches posting procedure document, it didn't
have anything regarding the order. It does since commit in 2018.

Anyway I talked with Alexey regarding this issue. So I'll add his signature
only to the patches/drivers in which actual development he was involved to
with further Co-Developed-by tag supplied.

Regards,
-Sergey

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

  parent reply index

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     ` Andy Shevchenko
     [not found]     ` <20200310141715.7063280307CA@mail.baikalelectronics.ru>
2020-03-12 18:47       ` Sergey Semin [this message]
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
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=20200312184450.co5o6zm6qniptfuv@ubsrv2.baikal.int \
    --to=sergey.semin@baikalelectronics.ru \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Ekaterina.Skachko@baikalelectronics.ru \
    --cc=Maxim.Kaurkin@baikalelectronics.ru \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=Ramil.Zaripov@baikalelectronics.ru \
    --cc=V.Vlasov@baikalelectronics.ru \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jslaby@suse.com \
    --cc=linux-serial@vger.kernel.org \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=tsbogend@alpha.franken.de \
    /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