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

[Andy Shevchenko]

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.

> 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