Messages in this thread | | | Date | Mon, 23 Mar 2020 11:20:51 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition |
| |
On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
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
| |