[RESEND,v6,0/3] serial: 8250_dw: Fix ref clock usage

Message

Serge Semin June 17, 2020, 10:48 p.m. UTC

Greg, Jiri. We've missed the last merge window. It would be pity to miss
the next one. Please review/merge in the series.

Regarding the patchset. 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 no good can happen. 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 small optimization patch. It
simplifies the DW APB UART ref clock rate setting procedure a bit.

This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
base-commit: 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.

Changelog v4:
- Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
  8250 port") since Greg has already merged it into the tty-next branch.
- Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.

Changelog v5:
- Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
  return value for being erroneous and exit if it is.
- Don't update p->uartclk in the port startup. It will be updated later in
  the same procedure at the set_termios() function being invoked by the
  serial_core anyway.

Changelog v6:
- Resend

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: Arnd Bergmann <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]

Serge Semin (3):
  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   | 116 +++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_port.c |  38 +++++++++
 include/linux/serial_8250.h         |   2 +
 3 files changed, 144 insertions(+), 12 deletions(-)

Comments

Andy Shevchenko June 18, 2020, 8:17 a.m. UTC | #1

On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
<[e-mail]> wrote:
>
> Greg, Jiri. We've missed the last merge window. It would be pity to miss
> the next one. Please review/merge in the series.
>
> Regarding the patchset. 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 no good can happen. 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 small optimization patch. It
> simplifies the DW APB UART ref clock rate setting procedure a bit.
>
> This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
> tag: v5.7-rc4

I'm wondering how this will collaborate with runtime PM.

> Changelog v3:
> - Refactor the original patch to adjust the UART port divisor instead of
>   requesting an exclusive ref clock utilization.
>
> Changelog v4:
> - Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
>   8250 port") since Greg has already merged it into the tty-next branch.
> - Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.
>
> Changelog v5:
> - Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
>   return value for being erroneous and exit if it is.
> - Don't update p->uartclk in the port startup. It will be updated later in
>   the same procedure at the set_termios() function being invoked by the
>   serial_core anyway.
>
> Changelog v6:
> - Resend
>
> 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: Arnd Bergmann <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
>
> Serge Semin (3):
>   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   | 116 +++++++++++++++++++++++++---
>  drivers/tty/serial/8250/8250_port.c |  38 +++++++++
>  include/linux/serial_8250.h         |   2 +
>  3 files changed, 144 insertions(+), 12 deletions(-)
>
> --
> 2.26.2
>

Andy Shevchenko June 18, 2020, 8:31 a.m. UTC | #2

On Thu, Jun 18, 2020 at 11:17 AM Andy Shevchenko
<[e-mail]> wrote:
> On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> <[e-mail]> wrote:

> I'm wondering how this will collaborate with runtime PM.

Forgot to mention the WIP repository [1] for runtime PM rework  (it
doesn't mean my comment against patch 3 is not valid). Tony and I are
working on it, you actually may be interested as well.

[1]: https://gitlab.com/andy-shev/next

Serge Semin June 19, 2020, 5:16 p.m. UTC | #3

On Thu, Jun 18, 2020 at 11:17:47AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> <[e-mail]> wrote:
> >
> > Greg, Jiri. We've missed the last merge window. It would be pity to miss
> > the next one. Please review/merge in the series.
> >
> > Regarding the patchset. 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 no good can happen. 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 small optimization patch. It
> > simplifies the DW APB UART ref clock rate setting procedure a bit.
> >
> > This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> > base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
> > tag: v5.7-rc4
> 

> I'm wondering how this will collaborate with runtime PM.

Thanks for noticing this. You are right. PM should be taken into account.

-Sergey

> 
> > Changelog v3:
> > - Refactor the original patch to adjust the UART port divisor instead of
> >   requesting an exclusive ref clock utilization.
> >
> > Changelog v4:
> > - Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
> >   8250 port") since Greg has already merged it into the tty-next branch.
> > - Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.
> >
> > Changelog v5:
> > - Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
> >   return value for being erroneous and exit if it is.
> > - Don't update p->uartclk in the port startup. It will be updated later in
> >   the same procedure at the set_termios() function being invoked by the
> >   serial_core anyway.
> >
> > Changelog v6:
> > - Resend
> >
> > 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: Arnd Bergmann <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Will Deacon <[e-mail]>
> > Cc: Russell King <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> >
> > Serge Semin (3):
> >   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   | 116 +++++++++++++++++++++++++---
> >  drivers/tty/serial/8250/8250_port.c |  38 +++++++++
> >  include/linux/serial_8250.h         |   2 +
> >  3 files changed, 144 insertions(+), 12 deletions(-)
> >
> > --
> > 2.26.2
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Serge Semin June 19, 2020, 5:21 p.m. UTC | #4

On Thu, Jun 18, 2020 at 11:31:44AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 11:17 AM Andy Shevchenko
> <[e-mail]> wrote:
> > On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> > <[e-mail]> wrote:
> 
> > I'm wondering how this will collaborate with runtime PM.
> 

> Forgot to mention the WIP repository [1] for runtime PM rework  (it
> doesn't mean my comment against patch 3 is not valid). Tony and I are
> working on it, you actually may be interested as well.
> 
> [1]: https://gitlab.com/andy-shev/next

Thanks for the link. I'll take a look at the repo. Though our SoC doesn't
require the RPM moderation that critically.

-Sergey

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