Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: Sergey.Semin@baikalelectronics.ru
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	Serge Semin <fancer.lancer@gmail.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: Mon, 23 Mar 2020 11:01:09 +0100
Message-ID: <20200323100109.k2gckdyneyzo23fb@gilmour.lan> (raw)
In-Reply-To: <20200323024611.16039-1-Sergey.Semin@baikalelectronics.ru>


[-- Attachment #1: Type: text/plain, Size: 5274 bytes --]

Hi,

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.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> CC: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@bootlin.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Move exclusive ref clock lock/unlock precudures to the 8250 port
>   startup/shutdown methods.
> - The changelog message has also been slightly modified due to the
>   alteration.
> - Remove Alexey' SoB tag.
> - Cc someone from ARM who might be concerned regarding this change.
> - Cc someone from Clocks Framework to get their comments on this patch.
> ---
>  drivers/tty/serial/8250/8250_dw.c | 36 +++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index aab3cccc6789..08f3f745ed54 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -319,6 +319,40 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
>  	serial8250_do_set_ldisc(p, termios);
>  }
>
> +static int dw8250_startup(struct uart_port *p)
> +{
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +
> +	/*
> +	 * Some platforms may provide a reference clock shared between several
> +	 * devices. In this case before using the serial port first we have to
> +	 * make sure nothing will change the rate behind our back and second
> +	 * the tty/serial subsystem knows the actual reference clock rate of
> +	 * the port.
> +	 */
> +	if (clk_rate_exclusive_get(d->clk)) {
> +		dev_warn(p->dev, "Couldn't lock the clock rate\n");
> +	} else if (d->clk) {
> +		p->uartclk = clk_get_rate(d->clk);
> +		if (!p->uartclk) {
> +			clk_rate_exclusive_put(d->clk);
> +			dev_err(p->dev, "Clock rate not defined\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return serial8250_do_startup(p);
> +}

I've been facing that issue, so it would be great to get it fixed, but
I'm not sure this is the right solution.

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.

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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  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     ` [PATCH] " 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 [this message]
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=20200323100109.k2gckdyneyzo23fb@gilmour.lan \
    --to=maxime@cerno.tech \
    --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=Sergey.Semin@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=fancer.lancer@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=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

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