Hello Sergey, On Tue, Jun 11, 2019 at 10:34:24AM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes: > >> 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? It seems I wasn't affected by the reduced readability. But I don't care much, so if you prefer it that way, keep it as is. > 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? Sometimes the settermios callback is called with irqs disabled, at least that's what Documentation/serial/driver.rst claims. I think this needs fixing. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
