On Thu, Jun 06, 2019 at 10:59:37AM +0300, Sergey Organov wrote: > imx_uart_set_termios() called imx_uart_rts_active(), or > imx_uart_rts_inactive() before taking port->port.lock. > > As a consequence, sport->port.mctrl that these functions modify > could have been changed without holding port->port.lock. > > Moved locking of port->port.lock above the calls to fix the issue. I'm convinced. The issue looks real. > Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx> > --- > drivers/tty/serial/imx.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index dff75dc..cb95ff71 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -1550,6 +1550,20 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios, > old_csize = CS8; > } > > + del_timer_sync(&sport->timer); > + > + /* > + * Ask the core to calculate the divisor for us. > + */ > + baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16); > + quot = uart_get_divisor(port, baud); > + > + /* > + * Take port lock before imx_uart_rts_*() calls, as they change > + * sport->port.mctrl > + */ > + spin_lock_irqsave(&sport->port.lock, flags); > + You can move this block a bit down (and so grab the lock later). The check for CSIZE doesn't need protection. Assuming you respin: Several functions are annotated to have to be called with the lock taken; I would put the comment to imx_uart_rts_* in the same way, instead of in imx_uart_set_termios. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
