Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes: > 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. Good! I mean: baaad :) > >> 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. I considered it, but decided putting lock inside UCR2 initialization sequence would negatively affect readability of the code. OTOH, 2-3 more asm instructions under the lock shouldn't be a big deal, right? In addition, I've got further patches on top of this one, and there I need to read-modify-write the UCR2, so I need to take the lock before taking initial value. I'll move the lock down in this patch if you still think it's worth it. > 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. Yeah, I will. I assume you mean /* called with port.lock taken and irqs off */ comment? The "and irqs off" part doesn't seem to be true for calls from set_termios() though, so I'd need to get rid of it for these new comments, right? Thanks! -- Sergey
