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

[Serge Semin]

On Thu, Jun 18, 2020 at 11:17:47AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> 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 <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Maxim Kaurkin <Maxim.Kaurkin@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Ramil Zaripov <Ramil.Zaripov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Ekaterina Skachko <Ekaterina.Skachko@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Vadim Vlasov <V.Vlasov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-serial@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> >
> > 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