On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
The question to CLK framework maintainers, is it correct approach in general
for this case?
> There are races possible in the dw8250_set_termios() callback method
> and while the device is in PM suspend state. A race condition may
> happen if the baudrate clock source device is shared with some other
> device (in our machine 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 enable an exclusive
> reference clock rate access in case if "baudclk" device is specified.
>
> So if some other device also acquires the rate exclusivity during the
> time of a DW UART 8250 port being opened, then DW UART 8250 driver
> won't be able to alter the baud-clock. It shall just use the available
> clock rate. Similarly another device also won't manage to change the
> rate at that time. If nothing else have the exclusive rate access
> acquired except DW UART 8250 driver, then the driver will be able to
> alter the rate as much as it needs to in accordance with the currently
> implemented logic.
Thank you for an update, my comments below.
...
> +static int dw8250_startup(struct uart_port *p)
> +{
> + struct dw8250_data *d = to_dw8250_data(p->private_data);
> +
> + /*
> + * 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 nothing will change the rate behind our back and second
> + * the tty/serial subsystem knows the actual reference clock rate of
> + * the port.
> + */
> + if (clk_rate_exclusive_get(d->clk)) {
> + dev_warn(p->dev, "Couldn't lock the clock rate\n");
So, if this fails, in ->shutdown you will disbalance reference count, or did I
miss something?
> + } else if (d->clk) {
> + p->uartclk = clk_get_rate(d->clk);
> + if (!p->uartclk) {
> + clk_rate_exclusive_put(d->clk);
> + dev_err(p->dev, "Clock rate not defined\n");
> + return -EINVAL;
> + }
This operations I didn't get. If we have d->clk and suddenly get 0 as a rate
(and note, that we still update uartclk member!), we try to put (why?) the
exclusiveness of rate.
> + }
> +
> + 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);
> +
> + clk_rate_exclusive_put(d->clk);
> +}
--
With Best Regards,
Andy Shevchenko
