Date:   Fri, 19 Jun 2020 20:16:17 +0300
From:   Serge Semin <Sergey.Semin@...kalelectronics.ru>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Serge Semin <fancer.lancer@...il.com>,
        Tony Lindgren <tony@...mide.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Maxim Kaurkin <Maxim.Kaurkin@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Alexey Kolotnikov <Alexey.Kolotnikov@...kalelectronics.ru>,
        Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>,
        Ekaterina Skachko <Ekaterina.Skachko@...kalelectronics.ru>,
        Vadim Vlasov <V.Vlasov@...kalelectronics.ru>,
        Arnd Bergmann <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Will Deacon <will@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        <linux-mips@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage

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@...kalelectronics.ru> 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@...kalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> > Cc: Maxim Kaurkin <Maxim.Kaurkin@...kalelectronics.ru>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@...kalelectronics.ru>
> > Cc: Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>
> > Cc: Ekaterina Skachko <Ekaterina.Skachko@...kalelectronics.ru>
> > Cc: Vadim Vlasov <V.Vlasov@...kalelectronics.ru>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@...kalelectronics.ru>
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > Cc: Maxime Ripard <mripard@...nel.org>
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Russell King <linux@...linux.org.uk>
> > Cc: linux-mips@...r.kernel.org
> > Cc: linux-arm-kernel@...ts.infradead.org
> > Cc: linux-serial@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org
> >
> > 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

Powered by blists - more mailing lists