[v2] serial: 8250_dw: Fix common clocks usage race condition

Commit Message

Serge Semin March 23, 2020, 2:46 a.m. UTC

From: Serge Semin <[e-mail]>

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 <[e-mail]>
Cc: Alexey Malahov <[e-mail]>
Cc: Maxim Kaurkin <[e-mail]>
Cc: Pavel Parkhomenko <[e-mail]>
Cc: Ramil Zaripov <[e-mail]>
Cc: Ekaterina Skachko <[e-mail]>
Cc: Vadim Vlasov <[e-mail]>
Cc: Thomas Bogendoerfer <[e-mail]>
Cc: Paul Burton <[e-mail]>
Cc: Ralf Baechle <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Chen-Yu Tsai <[e-mail]>
CC: Ray Jui <[e-mail]>
Cc: Scott Branden <[e-mail]>
Cc: Florian Fainelli <[e-mail]>
Cc: Wei Xu <[e-mail]>
Cc: Jason Cooper <[e-mail]>
Cc: Andrew Lunn <[e-mail]>
Cc: Gregory Clement <[e-mail]>
Cc: Sebastian Hesselbarth <[e-mail]>
Cc: Jisheng Zhang <[e-mail]>
Cc: Heiko Stuebner <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: [e-mail]
Cc: Michael Turquette <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: [e-mail]

---

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(+)

Comments

Andy Shevchenko March 23, 2020, 9:20 a.m. UTC | #1

On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> From: Serge Semin <[e-mail]>

The question to CLK framework maintainers, is it correct approach in general
for this case?

> 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.

Thank you for an update, my comments below.

...

> +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");

So, if this fails, in ->shutdown you will disbalance reference count, or did I
miss something?

> +	} 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;
> +		}

This operations I didn't get. If we have d->clk and suddenly get 0 as a rate
(and note, that we still update uartclk member!), we try to put (why?) the
exclusiveness of rate.

> +	}
> +
> +	return serial8250_do_startup(p);
> +}
> +
> +static void dw8250_shutdown(struct uart_port *p)
> +{
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +
> +	serial8250_do_shutdown(p);
> +
> +	clk_rate_exclusive_put(d->clk);
> +}

Maxime Ripard March 23, 2020, 10:01 a.m. UTC | #2

Hi,

On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> From: Serge Semin <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Maxim Kaurkin <[e-mail]>
> Cc: Pavel Parkhomenko <[e-mail]>
> Cc: Ramil Zaripov <[e-mail]>
> Cc: Ekaterina Skachko <[e-mail]>
> Cc: Vadim Vlasov <[e-mail]>
> Cc: Thomas Bogendoerfer <[e-mail]>
> Cc: Paul Burton <[e-mail]>
> Cc: Ralf Baechle <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> CC: Ray Jui <[e-mail]>
> Cc: Scott Branden <[e-mail]>
> Cc: Florian Fainelli <[e-mail]>
> Cc: Wei Xu <[e-mail]>
> Cc: Jason Cooper <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>
> Cc: Gregory Clement <[e-mail]>
> Cc: Sebastian Hesselbarth <[e-mail]>
> Cc: Jisheng Zhang <[e-mail]>
> Cc: Heiko Stuebner <[e-mail]>
> Cc: Catalin Marinas <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: [e-mail]
> Cc: Michael Turquette <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: [e-mail]
>
> ---
>
> 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

Serge Semin March 23, 2020, 11:11 a.m. UTC | #3

Hi

On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> > From: Serge Semin <[e-mail]>
> 
> The question to CLK framework maintainers, is it correct approach in general
> for this case?
> 

You should have been more specific then, if you wanted to see someone
special.

> On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote:
>> Also it would be nice to see come clock framework guys' opinions...

Who can give a better comments regarding the clk API if not the
subsystem maintainers?

> > 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.
> 
> Thank you for an update, my comments below.
> 
> ...
> 
> > +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");
> 
> So, if this fails, in ->shutdown you will disbalance reference count, or did I
> miss something?
> 

Hm, you are right. I didn't fully thought this through. The thing is
that according to the clk_rate_exclusive_get() function code currently
it never fails. Though this isn't excuse for introducing a prone to future
bugs code.

Anyway if according to design a function may return an error we must take
into account in the code using it. Due to this obligation and seeing we can't
easily detect whether clk_rate_exclusive_get() has been failed while the
driver is being executed in the shutdown method, the best approach would be
to just return an error in startup method in case of the clock rate exclusivity
acquisition failure. If you are ok with this, I'll have it fixed in v3
patchset.

> > +	} 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;
> > +		}
> 
> This operations I didn't get. If we have d->clk and suddenly get 0 as a rate
> (and note, that we still update uartclk member!), we try to put (why?) the
> exclusiveness of rate.
> 

Here is what I had in my mind while implementing this code. If d->clk
isn't NULL, then there is a "baudclk" clock handler and we can use it to
alter/retrieve the baud clock rate. But the same clock could be used by
some other driver and that driver could have changed the rate while we
didn't have this tty port started up (opened). In this case that driver
could also have the clock exclusively acquired. So instead of trying to
set the current p->uartclk rate to the clock, check the return value,
if it's an error, try to get the current clock rate, check the return
value, and so on, I just get the current baud clock rate and make sure
the value is not zero (clk_get_rate() returns a zero rate in case of
internal errors). At the same time dw8250_set_termios() will try to update
the baud clock rate anyway (also by the serial core at the point of the port
startup), so we don't need such complication in the DW 8250 port startup
code.

> (and note, that we still update uartclk member!),

Yes, if we can't determine the current baud clock rate, then the there is
a problem with the clock device, so we don't know at what rate it's
currently working. Zero is the most appropriate value to be set in this case.

> we try to put (why?) the > exclusiveness of rate.

Yes, we put the exclusivity and return an error, because this if-branch has
been taken only if the exclusivity has been successfully acquired.

Regards,
-Sergey

> > +	}
> > +
> > +	return serial8250_do_startup(p);
> > +}
> > +
> > +static void dw8250_shutdown(struct uart_port *p)
> > +{
> > +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> > +
> > +	serial8250_do_shutdown(p);
> > +
> > +	clk_rate_exclusive_put(d->clk);
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Andy Shevchenko March 23, 2020, 11:52 a.m. UTC | #4

On Mon, Mar 23, 2020 at 02:11:49PM +0300, Sergey Semin wrote:
> On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> > > From: Serge Semin <[e-mail]>
> > 
> > The question to CLK framework maintainers, is it correct approach in general
> > for this case?
> 
> You should have been more specific then, if you wanted to see someone
> special.

I didn't get your comment here. Since you put the question under a pile of
words in the commit message, and actually in the changelog, not even in the
message, I repeated it clearly that clock maintainers can see it.

> > On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote:
> >> Also it would be nice to see come clock framework guys' opinions...
> 
> Who can give a better comments regarding the clk API if not the
> subsystem maintainers?

You already got one from Maxime.

...

> > > +	/*
> > > +	 * 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");
> > 
> > So, if this fails, in ->shutdown you will disbalance reference count, or did I
> > miss something?
> > 
> 
> Hm, you are right. I didn't fully thought this through. The thing is
> that according to the clk_rate_exclusive_get() function code currently
> it never fails. Though this isn't excuse for introducing a prone to future
> bugs code.
> 
> Anyway if according to design a function may return an error we must take
> into account in the code using it. Due to this obligation and seeing we can't
> easily detect whether clk_rate_exclusive_get() has been failed while the
> driver is being executed in the shutdown method, the best approach would be
> to just return an error in startup method in case of the clock rate exclusivity
> acquisition failure. If you are ok with this, I'll have it fixed in v3
> patchset.

It needs to be carefully tested on other platforms than yours.

> > > +	} 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;
> > > +		}
> > 
> > This operations I didn't get. If we have d->clk and suddenly get 0 as a rate
> > (and note, that we still update uartclk member!), we try to put (why?) the
> > exclusiveness of rate.
> > 
> 
> Here is what I had in my mind while implementing this code. If d->clk
> isn't NULL, then there is a "baudclk" clock handler and we can use it to
> alter/retrieve the baud clock rate. But the same clock could be used by
> some other driver and that driver could have changed the rate while we
> didn't have this tty port started up (opened). In this case that driver
> could also have the clock exclusively acquired. So instead of trying to
> set the current p->uartclk rate to the clock, check the return value,
> if it's an error, try to get the current clock rate, check the return
> value, and so on, I just get the current baud clock rate and make sure
> the value is not zero

> (clk_get_rate() returns a zero rate in case of
> internal errors).

Have you considered !CLK case?

> At the same time dw8250_set_termios() will try to update
> the baud clock rate anyway (also by the serial core at the point of the port
> startup), so we don't need such complication in the DW 8250 port startup
> code.
> 
> > (and note, that we still update uartclk member!),
> 
> Yes, if we can't determine the current baud clock rate, then the there is
> a problem with the clock device, so we don't know at what rate it's
> currently working. Zero is the most appropriate value to be set in this case.
> 
> > we try to put (why?) the > exclusiveness of rate.
> 
> Yes, we put the exclusivity and return an error, because this if-branch has
> been taken only if the exclusivity has been successfully acquired.

So, this means that above code requires elaboration in the comments to explain
how it supposed to work.

Serge Semin March 23, 2020, 1:50 p.m. UTC | #5

Hello Maxime

On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> > From: Serge Semin <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Alexey Malahov <[e-mail]>
> > Cc: Maxim Kaurkin <[e-mail]>
> > Cc: Pavel Parkhomenko <[e-mail]>
> > Cc: Ramil Zaripov <[e-mail]>
> > Cc: Ekaterina Skachko <[e-mail]>
> > Cc: Vadim Vlasov <[e-mail]>
> > Cc: Thomas Bogendoerfer <[e-mail]>
> > Cc: Paul Burton <[e-mail]>
> > Cc: Ralf Baechle <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Chen-Yu Tsai <[e-mail]>
> > CC: Ray Jui <[e-mail]>
> > Cc: Scott Branden <[e-mail]>
> > Cc: Florian Fainelli <[e-mail]>
> > Cc: Wei Xu <[e-mail]>
> > Cc: Jason Cooper <[e-mail]>
> > Cc: Andrew Lunn <[e-mail]>
> > Cc: Gregory Clement <[e-mail]>
> > Cc: Sebastian Hesselbarth <[e-mail]>
> > Cc: Jisheng Zhang <[e-mail]>
> > Cc: Heiko Stuebner <[e-mail]>
> > Cc: Catalin Marinas <[e-mail]>
> > Cc: Will Deacon <[e-mail]>
> > Cc: Russell King <[e-mail]>
> > Cc: [e-mail]
> > Cc: Michael Turquette <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: [e-mail]
> >
> > ---
> >
> > 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.
> 

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.
> 
> Maxime

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?

Regards,
-Sergey

Serge Semin March 23, 2020, 5:07 p.m. UTC | #6

On Mon, Mar 23, 2020 at 01:52:25PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 02:11:49PM +0300, Sergey Semin wrote:
> > On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> > > > From: Serge Semin <[e-mail]>
> > > 
> > > The question to CLK framework maintainers, is it correct approach in general
> > > for this case?
> > 
> > You should have been more specific then, if you wanted to see someone
> > special.
> 
> I didn't get your comment here. Since you put the question under a pile of
> words in the commit message, and actually in the changelog, not even in the
> message, I repeated it clearly that clock maintainers can see it.
> 
> > > On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote:
> > >> Also it would be nice to see come clock framework guys' opinions...
> > 
> > Who can give a better comments regarding the clk API if not the
> > subsystem maintainers?
> 
> You already got one from Maxime.
> 
> ...
> 
> > > > +	/*
> > > > +	 * 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");
> > > 
> > > So, if this fails, in ->shutdown you will disbalance reference count, or did I
> > > miss something?
> > > 
> > 
> > Hm, you are right. I didn't fully thought this through. The thing is
> > that according to the clk_rate_exclusive_get() function code currently
> > it never fails. Though this isn't excuse for introducing a prone to future
> > bugs code.
> > 
> > Anyway if according to design a function may return an error we must take
> > into account in the code using it. Due to this obligation and seeing we can't
> > easily detect whether clk_rate_exclusive_get() has been failed while the
> > driver is being executed in the shutdown method, the best approach would be
> > to just return an error in startup method in case of the clock rate exclusivity
> > acquisition failure. If you are ok with this, I'll have it fixed in v3
> > patchset.
> 
> It needs to be carefully tested on other platforms than yours.
> 

Alas I don't have one. But it can be done by other kernel users at rc-s stage
of the next kernel release.

> > > > +	} 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;
> > > > +		}
> > > 
> > > This operations I didn't get. If we have d->clk and suddenly get 0 as a rate
> > > (and note, that we still update uartclk member!), we try to put (why?) the
> > > exclusiveness of rate.
> > > 
> > 
> > Here is what I had in my mind while implementing this code. If d->clk
> > isn't NULL, then there is a "baudclk" clock handler and we can use it to
> > alter/retrieve the baud clock rate. But the same clock could be used by
> > some other driver and that driver could have changed the rate while we
> > didn't have this tty port started up (opened). In this case that driver
> > could also have the clock exclusively acquired. So instead of trying to
> > set the current p->uartclk rate to the clock, check the return value,
> > if it's an error, try to get the current clock rate, check the return
> > value, and so on, I just get the current baud clock rate and make sure
> > the value is not zero
> 
> > (clk_get_rate() returns a zero rate in case of
> > internal errors).
> 
> Have you considered !CLK case?
> 

Yes. It's a case of optional clock. Have a look at how the clock API
works. You are already using it here in this driver when calling
clk_prepare_enable()/clk_disable_unprepare().

> > At the same time dw8250_set_termios() will try to update
> > the baud clock rate anyway (also by the serial core at the point of the port
> > startup), so we don't need such complication in the DW 8250 port startup
> > code.
> > 
> > > (and note, that we still update uartclk member!),
> > 
> > Yes, if we can't determine the current baud clock rate, then the there is
> > a problem with the clock device, so we don't know at what rate it's
> > currently working. Zero is the most appropriate value to be set in this case.
> > 
> > > we try to put (why?) the > exclusiveness of rate.
> > 
> > Yes, we put the exclusivity and return an error, because this if-branch has
> > been taken only if the exclusivity has been successfully acquired.
> 
> So, this means that above code requires elaboration in the comments to explain
> how it supposed to work.
> 

That's what I did by the comment: "... second the tty/serial subsystem knows
the actual reference clock rate of the port." If you think, that checking a
return value and undoing things in case of an error need elaboration in a
comment I'll do it in v3.

-Regards,
Sergey

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

Maxime Ripard March 24, 2020, 9:41 a.m. UTC | #7

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:
> > Hi,
> >
> > On Mon, Mar 23, 2020 at 05:46:09AM +0300, [e-mail] wrote:
> > > From: Serge Semin <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Alexey Malahov <[e-mail]>
> > > Cc: Maxim Kaurkin <[e-mail]>
> > > Cc: Pavel Parkhomenko <[e-mail]>
> > > Cc: Ramil Zaripov <[e-mail]>
> > > Cc: Ekaterina Skachko <[e-mail]>
> > > Cc: Vadim Vlasov <[e-mail]>
> > > Cc: Thomas Bogendoerfer <[e-mail]>
> > > Cc: Paul Burton <[e-mail]>
> > > Cc: Ralf Baechle <[e-mail]>
> > > Cc: Maxime Ripard <[e-mail]>
> > > Cc: Chen-Yu Tsai <[e-mail]>
> > > CC: Ray Jui <[e-mail]>
> > > Cc: Scott Branden <[e-mail]>
> > > Cc: Florian Fainelli <[e-mail]>
> > > Cc: Wei Xu <[e-mail]>
> > > Cc: Jason Cooper <[e-mail]>
> > > Cc: Andrew Lunn <[e-mail]>
> > > Cc: Gregory Clement <[e-mail]>
> > > Cc: Sebastian Hesselbarth <[e-mail]>
> > > Cc: Jisheng Zhang <[e-mail]>
> > > Cc: Heiko Stuebner <[e-mail]>
> > > Cc: Catalin Marinas <[e-mail]>
> > > Cc: Will Deacon <[e-mail]>
> > > Cc: Russell King <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Michael Turquette <[e-mail]>
> > > Cc: Stephen Boyd <[e-mail]>
> > > Cc: [e-mail]
> > >
> > > ---
> > >
> > > 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.
> >
>
> 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.)

I just remembered the RPi3/4 are in this situation too.

> Moreover as I can see the same issue may raise for I2C, QSPI, PWM
> devices there.

We also have the I2C on the same parent clock in the Allwinner SoCs
indeed :)

> 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.

This solution, at least without a whitelist of platforms where it
makes sense, would be NAK'd for me. And I'm strongly suspecting that
that whitelist would be very short, so the overall usefulness is
pretty limited.

Maxime

Andy Shevchenko March 24, 2020, 10:12 a.m. UTC | #8

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, [e-mail] wrote:
> > > From: Serge Semin <[e-mail]>
> > >
> > > 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...

Serge Semin March 25, 2020, 5:11 p.m. UTC | #9

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, [e-mail] wrote:
> > > > From: Serge Semin <[e-mail]>
> > > >
> > > > 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
> 
>

Stephen Boyd March 26, 2020, 1:44 a.m. UTC | #10

Quoting Sergey Semin (2020-03-25 10:11:09)
> 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:
> > 
> > > > 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.
> 

Sorry, I haven't really read the thread but I'll quickly reply with
this.

Maybe option 4 is to make the uart driver a clk provider that consumes
the single reference clk like it is already doing today? Then when the
rate changes up above for the clk implemented here the clk set rate op
for the newly implemented clk can go poke the uart registers to maintain
the baud or whatever?

That is close to how the notifier design would work, but it avoids
keeping the notifiers around given that the notifiers are not preferred.
It is also closer to reality, the uart has a divider or mux internally
that we don't model as a clk, but we could just as easily model as such.

Serge Semin March 27, 2020, 9:12 a.m. UTC | #11

Hello Stephen

Thanks for reply. My comment is below.

On Wed, Mar 25, 2020 at 06:44:53PM -0700, Stephen Boyd wrote:
> Quoting Sergey Semin (2020-03-25 10:11:09)
> > 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:
> > > 
> > > > > 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.
> > 
> 
> Sorry, I haven't really read the thread but I'll quickly reply with
> this.
> 
> Maybe option 4 is to make the uart driver a clk provider that consumes
> the single reference clk like it is already doing today? Then when the
> rate changes up above for the clk implemented here the clk set rate op
> for the newly implemented clk can go poke the uart registers to maintain
> the baud or whatever?
> 
> That is close to how the notifier design would work, but it avoids
> keeping the notifiers around given that the notifiers are not preferred.
> It is also closer to reality, the uart has a divider or mux internally
> that we don't model as a clk, but we could just as easily model as such.

AFAIU your suggestion is pretty similar to the option 2), but it concerns
the fixup implementation. So instead of subscribing to the reference clock
change event and directly adjusting the UART clock divider when the change
happens, you suggest to convert the divisor setting code into a clock
provider, which in case of the parental clocks rate change shall
automatically cause the rate adjustment of the clocks hierarchy below.

While your proposal looks neat and better suits a common approach of
the drivers design, it won't be that easy to be implemented for the serial
subsystem. As far as I can see serial and 8250 code is too coupled with
manual divisor and reference clock settings. Common 8250 port code gets
and sets the divisor and relies on the reference clock value. Similarly
the 8250-compatible vendor specific devices may also have custom divisor
settings. Moreover uartclk field, which indicates the reference clock rate
value, is used in many placed over the serial code, so if we implemented
your design we would have to update it value anyway, which means to
subscribe to the reference clock rate change event.

So in order to do what you said, the serial subsystem would have to be
seriously refactored, which taking into account the subsystem age and
number of driver, will be very painful.

-Sergey

Serge Semin May 6, 2020, 11:31 p.m. UTC | #12

It might be dangerous if an UART port reference clock rate is suddenly
changed. In particular the 8250 port drivers (and AFAICS most of the tty
drivers using common clock framework clocks) rely either on the
exclusive reference clock utilization or on the ref clock rate being
always constant. Needless to say that it turns out not true and if some
other service suddenly changes the clock rate behind an UART port driver
back it's no good. So the port might not only end up with an invalid
uartclk value saved, but may also experience a distorted output/input
data since such action will effectively update the programmed baud-clock.
We discovered such problem on Baikal-T1 SoC where two DW 8250 ports have
got a shared reference clock. Allwinner SoC is equipped with an UART,
which clock is derived from the CPU PLL clock source, so the CPU frequency
change might be propagated down up to the serial port reference clock.
This patchset provides a way to fix the problem to the 8250 serial port
controllers and mostly fixes it for the DW 8250-compatible UART. I say
mostly because due to not having a facility to pause/stop and resume/
restart on-going transfers we implemented the UART clock rate update
procedure executed post factum of the actual reference clock rate change.

In addition the patchset includes a few fixes we discovered when were
working the issue. First one concerns the maximum baud rate setting used
to determine a serial port baud based on the current UART port clock rate.
Another one simplifies the ref clock rate setting procedure a bit.

This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
0e698dfa2822 ("Linux 5.7-rc4")
tag: v5.7-rc4

Changelog v3:
- Refactor the original patch to adjust the UART port divisor instead of
  requesting an exclusive ref clock utilization.

Signed-off-by: Serge Semin <[e-mail]>
Cc: Alexey Malahov <[e-mail]>
Cc: Maxim Kaurkin <[e-mail]>
Cc: Pavel Parkhomenko <[e-mail]>
Cc: Alexey Kolotnikov <[e-mail]>
Cc: Ramil Zaripov <[e-mail]>
Cc: Ekaterina Skachko <[e-mail]>
Cc: Vadim Vlasov <[e-mail]>
Cc: Alexey Kolotnikov <[e-mail]>
Cc: Paul Burton <[e-mail]>
Cc: Ralf Baechle <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Long Cheng <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]

Serge Semin (4):
  serial: 8250: Fix max baud limit in generic 8250 port
  serial: 8250: Add 8250 port clock update method
  serial: 8250_dw: Simplify the ref clock rate setting procedure
  serial: 8250_dw: Fix common clocks usage race condition

 drivers/tty/serial/8250/8250_dw.c   | 125 +++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_port.c |  42 +++++++++-
 include/linux/serial_8250.h         |   2 +
 3 files changed, 156 insertions(+), 13 deletions(-)

Andy Shevchenko May 7, 2020, 6:29 p.m. UTC | #13

On Thu, May 07, 2020 at 02:31:31AM +0300, Serge Semin wrote:
> It might be dangerous if an UART port reference clock rate is suddenly
> changed. In particular the 8250 port drivers (and AFAICS most of the tty
> drivers using common clock framework clocks) rely either on the
> exclusive reference clock utilization or on the ref clock rate being
> always constant. Needless to say that it turns out not true and if some
> other service suddenly changes the clock rate behind an UART port driver
> back it's no good. So the port might not only end up with an invalid
> uartclk value saved, but may also experience a distorted output/input
> data since such action will effectively update the programmed baud-clock.
> We discovered such problem on Baikal-T1 SoC where two DW 8250 ports have
> got a shared reference clock. Allwinner SoC is equipped with an UART,
> which clock is derived from the CPU PLL clock source, so the CPU frequency
> change might be propagated down up to the serial port reference clock.
> This patchset provides a way to fix the problem to the 8250 serial port
> controllers and mostly fixes it for the DW 8250-compatible UART. I say
> mostly because due to not having a facility to pause/stop and resume/
> restart on-going transfers we implemented the UART clock rate update
> procedure executed post factum of the actual reference clock rate change.
> 
> In addition the patchset includes a few fixes we discovered when were
> working the issue. First one concerns the maximum baud rate setting used
> to determine a serial port baud based on the current UART port clock rate.
> Another one simplifies the ref clock rate setting procedure a bit.
> 
> This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> 0e698dfa2822 ("Linux 5.7-rc4")
> tag: v5.7-rc4

Thanks!

I will look at them later, but first impression that the first approach narrowed
to the certain SoC (by matching compatible string) looks better solution for
time being.

> Changelog v3:
> - Refactor the original patch to adjust the UART port divisor instead of
>   requesting an exclusive ref clock utilization.
> 
> Signed-off-by: Serge Semin <[e-mail]>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Maxim Kaurkin <[e-mail]>
> Cc: Pavel Parkhomenko <[e-mail]>
> Cc: Alexey Kolotnikov <[e-mail]>
> Cc: Ramil Zaripov <[e-mail]>
> Cc: Ekaterina Skachko <[e-mail]>
> Cc: Vadim Vlasov <[e-mail]>
> Cc: Alexey Kolotnikov <[e-mail]>
> Cc: Paul Burton <[e-mail]>
> Cc: Ralf Baechle <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Long Cheng <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Catalin Marinas <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> 
> Serge Semin (4):
>   serial: 8250: Fix max baud limit in generic 8250 port
>   serial: 8250: Add 8250 port clock update method
>   serial: 8250_dw: Simplify the ref clock rate setting procedure
>   serial: 8250_dw: Fix common clocks usage race condition
> 
>  drivers/tty/serial/8250/8250_dw.c   | 125 +++++++++++++++++++++++++---
>  drivers/tty/serial/8250/8250_port.c |  42 +++++++++-
>  include/linux/serial_8250.h         |   2 +
>  3 files changed, 156 insertions(+), 13 deletions(-)
> 
> -- 
> 2.25.1
>

Patch

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);
+}
+
+static void dw8250_shutdown(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+
+	serial8250_do_shutdown(p);
+
+	clk_rate_exclusive_put(d->clk);
+}
+
 /*
  * dw8250_fallback_dma_filter will prevent the UART from getting just any free
  * channel on platforms that have DMA engines, but don't have any channels
@@ -414,6 +448,8 @@  static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->startup	= dw8250_startup;
+	p->shutdown	= dw8250_shutdown;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)