| Message ID | 20240913-serial-imx-lockfix-v1-2-4d102746c89d@geanix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | serial: imx: Grab port lock in imx_uart_enable_wakeup() | expand |
Hi, Am Freitag, 13. September 2024, 10:39:50 CEST schrieb Esben Haabendal: > Comments regarding status of port.lock on internal functions is useful when > reviewing correct handling of registers that must be protected by this > lock. > > Signed-off-by: Esben Haabendal <esben@geanix.com> > --- > drivers/tty/serial/imx.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index efa3eb3a2c57..bea4510743ef 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -370,6 +370,7 @@ static void imx_uart_soft_reset(struct imx_port *sport) > sport->idle_counter = 0; > } > > +/* called with port.lock taken and irqs off */ > static void imx_uart_disable_loopback_rs485(struct imx_port *sport) > { > unsigned int uts; I think you are referring to sport.lock. On the other hand, instead of just adding comments, wouldn't it be better to make it explicit? Adding > lockdep_assert_held(&sport->port->lock); and/or sparse annoations > __must_hold(&sport->port->lock) seems more reasonable to me than adding non-enforcing comments. Best regards, Alexander > @@ -470,6 +471,7 @@ static void imx_uart_stop_tx(struct uart_port *port) > } > } > > +/* called with port.lock taken and irqs off */ > static void imx_uart_stop_rx_with_loopback_ctrl(struct uart_port *port, bool loopback) > { > struct imx_port *sport = to_imx_port(port); > @@ -803,6 +805,8 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id) > * issuing soft reset to the UART (just stop/start of RX does not help). Note > * that what we do here is sending isolated start bit about 2.4 times shorter > * than it is to be on UART configured baud rate. > + * > + * Called with port.lock taken and irqs off. > */ > static void imx_uart_check_flood(struct imx_port *sport, u32 usr2) > { > @@ -838,6 +842,7 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2) > } > } > > +/* called with port.lock taken and irqs off */ > static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) > { > struct imx_port *sport = dev_id; > @@ -916,6 +921,7 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport); > /* > * We have a modem side uart, so the meanings of RTS and CTS are inverted. > */ > +/* called with port.lock taken and irqs off */ > static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport) > { > unsigned int tmp = TIOCM_DSR; > @@ -938,6 +944,8 @@ static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport) > > /* > * Handle any change of modem status signal since we were last called. > + * > + * Called with port.lock taken and irqs off. > */ > static void imx_uart_mctrl_check(struct imx_port *sport) > { > @@ -1277,6 +1285,7 @@ static int imx_uart_start_rx_dma(struct imx_port *sport) > return 0; > } > > +/* called with port.lock taken and irqs off */ > static void imx_uart_clear_rx_errors(struct imx_port *sport) > { > struct tty_port *port = &sport->port.state->port; > @@ -1407,6 +1416,7 @@ static int imx_uart_dma_init(struct imx_port *sport) > return ret; > } > > +/* called with port.lock taken and irqs off */ > static void imx_uart_enable_dma(struct imx_port *sport) > { > u32 ucr1; > >
Alexander Stein <alexander.stein@ew.tq-group.com> writes: > Hi, > > Am Freitag, 13. September 2024, 10:39:50 CEST schrieb Esben Haabendal: >> Comments regarding status of port.lock on internal functions is useful when >> reviewing correct handling of registers that must be protected by this >> lock. >> >> Signed-off-by: Esben Haabendal <esben@geanix.com> >> --- >> drivers/tty/serial/imx.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index efa3eb3a2c57..bea4510743ef 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -370,6 +370,7 @@ static void imx_uart_soft_reset(struct imx_port *sport) >> sport->idle_counter = 0; >> } >> >> +/* called with port.lock taken and irqs off */ >> static void imx_uart_disable_loopback_rs485(struct imx_port *sport) >> { >> unsigned int uts; > > I think you are referring to sport.lock. Yes. > On the other hand, instead of just adding comments, wouldn't it be > better to make it explicit? > Adding >> lockdep_assert_held(&sport->port->lock); > and/or sparse annoations >> __must_hold(&sport->port->lock) > > seems more reasonable to me than adding non-enforcing comments. I fear that due to the way that legacy console works, assertations might trigger in special situations, such as printk during panic. Converting comments to assertations could definitely be a good idea, but I think it might be better to wait with that until the driver has been converted to NBCON (in progress, see https://lore.kernel.org/all/20240913-serial-imx-nbcon-v3-1-4c627302335b@geanix.com/), as that will change the code paths this code will be used in. /Esben
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index efa3eb3a2c57..bea4510743ef 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -370,6 +370,7 @@ static void imx_uart_soft_reset(struct imx_port *sport) sport->idle_counter = 0; } +/* called with port.lock taken and irqs off */ static void imx_uart_disable_loopback_rs485(struct imx_port *sport) { unsigned int uts; @@ -470,6 +471,7 @@ static void imx_uart_stop_tx(struct uart_port *port) } } +/* called with port.lock taken and irqs off */ static void imx_uart_stop_rx_with_loopback_ctrl(struct uart_port *port, bool loopback) { struct imx_port *sport = to_imx_port(port); @@ -803,6 +805,8 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id) * issuing soft reset to the UART (just stop/start of RX does not help). Note * that what we do here is sending isolated start bit about 2.4 times shorter * than it is to be on UART configured baud rate. + * + * Called with port.lock taken and irqs off. */ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2) { @@ -838,6 +842,7 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2) } } +/* called with port.lock taken and irqs off */ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) { struct imx_port *sport = dev_id; @@ -916,6 +921,7 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport); /* * We have a modem side uart, so the meanings of RTS and CTS are inverted. */ +/* called with port.lock taken and irqs off */ static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport) { unsigned int tmp = TIOCM_DSR; @@ -938,6 +944,8 @@ static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport) /* * Handle any change of modem status signal since we were last called. + * + * Called with port.lock taken and irqs off. */ static void imx_uart_mctrl_check(struct imx_port *sport) { @@ -1277,6 +1285,7 @@ static int imx_uart_start_rx_dma(struct imx_port *sport) return 0; } +/* called with port.lock taken and irqs off */ static void imx_uart_clear_rx_errors(struct imx_port *sport) { struct tty_port *port = &sport->port.state->port; @@ -1407,6 +1416,7 @@ static int imx_uart_dma_init(struct imx_port *sport) return ret; } +/* called with port.lock taken and irqs off */ static void imx_uart_enable_dma(struct imx_port *sport) { u32 ucr1;
Comments regarding status of port.lock on internal functions is useful when reviewing correct handling of registers that must be protected by this lock. Signed-off-by: Esben Haabendal <esben@geanix.com> --- drivers/tty/serial/imx.c | 10 ++++++++++ 1 file changed, 10 insertions(+)