Re: [PATCH v4 3/3] serial: 8250_dw: Fix common clocks usage race condition

On Tue, May 26, 2020 at 07:03:16PM +0300, Serge Semin 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.

...

+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) {

	if (rate <= 0)
		return;

?

+		up = serial8250_get_port(d->data.line);
+
+		serial8250_update_uartclk(&up->port, rate);
+	}
+}

...

+static int dw8250_startup(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	int ret;
+
+	/*
+	 * Some platforms may provide a reference clock shared between several
+	 * devices. In this case before using the serial port first we have to
+	 * make sure that any clock state change is known to the UART port at
+	 * least post factum.
+	 */

+	if (d->clk) {

Do you need this?

+		ret = clk_notifier_register(d->clk, &d->clk_notifier);

Okay, seems clk_notifier_register() and its counterpart should be fixed for
optional clocks.

+		if (ret)
+			dev_warn(p->dev, "Failed to set the clock notifier\n");

So, what does this warning mean on the platforms which does not need notifier
at all (i.o.w. all but baikal)?

+		/*
+		 * Get current reference clock rate to make sure the UART port
+		 * is equipped with an up-to-date value before it's started up.
+		 */

Why? We call ->set_termios() for it, no?

+		p->uartclk = clk_get_rate(d->clk);
+		if (!p->uartclk) {
+			dev_err(p->dev, "Clock rate not defined\n");
+			return -EINVAL;
+		}
+	}
+
+	return serial8250_do_startup(p);
+}
+
+static void dw8250_shutdown(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+
+	serial8250_do_shutdown(p);
+

+	if (d->clk) {

Ditto.

+		clk_notifier_unregister(d->clk, &d->clk_notifier);
+
+		flush_work(&d->clk_work);
+	}
+}

-- 
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