Thread (10 messages) 10 messages, 2 authors, 2020-06-19

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help