Re: [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition
From: Andy Shevchenko <hidden>
Date: 2020-06-18 08:29:52
Also in:
linux-mips, linux-serial, lkml
On Thu, Jun 18, 2020 at 1:50 AM Serge Semin [off-list ref] wrote:
The race condition may happen if the UART reference clock is shared with some other device (on Baikal-T1 SoC 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 at least try to adjust the 8250 port setting like UART clock rate in case if the reference clock rate change is discovered. The driver will call the new method to update 8250 UART port clock rate settings. It's done by means of the clock event notifier registered at the port startup and unregistered in the shutdown callback method. Note 1. In order to avoid deadlocks we had to execute the UART port update method in a dedicated deferred work. This is due to (in my opinion redundant) the clock update implemented in the dw8250_set_termios() method. Note 2. Before the ref clock is manually changed by the custom set_termios() function we swap the port uartclk value with new rate adjusted to be suitable for the requested baud. It is necessary in order to effectively disable a functionality of the ref clock events handler for the current UART port, since uartclk update will be done a bit further in the generic serial8250_do_set_termios() function.
So, regarding runtime PM...
+static void dw8250_clk_work_cb(struct work_struct *work)
+{
+ struct dw8250_data *d = work_to_dw8250_data(work);
+ struct uart_8250_port *up;
+ unsigned long rate;
+
+ rate = clk_get_rate(d->clk);
+ if (rate <= 0)
+ return;
++ up = serial8250_get_port(d->data.line);
(Btw, this can be done directly in the definition block above.)
+ serial8250_update_uartclk(&up->port, rate);
This I think should require a device to be powered on. What in your opinion is a better place to have it done? To me it looks like serial8250_update_uartclk() misses it.
+}
-- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel