[PATCH 0/2] serial: imx: Grab port lock in imx_uart_enable_wakeup()

[PATCH 0/2] serial: imx: Grab port lock in imx_uart_enable_wakeup()

[Esben Haabendal]

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Esben Haabendal (2):
      serial: imx: Grab port lock in imx_uart_enable_wakeup()
      serial: imx: Add more comments on port lock status

 drivers/tty/serial/imx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
---
base-commit: da3ea35007d0af457a0afc87e84fddaebc4e0b63
change-id: 20240913-serial-imx-lockfix-91141dcf3182

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

[PATCH 1/2] serial: imx: Grab port lock in imx_uart_enable_wakeup()

[Esben Haabendal]

The port lock needs to be held when doing read-modify-write on UCR1 and
UCR3.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/imx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 67d4a72eda77..efa3eb3a2c57 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2580,10 +2580,13 @@ static void imx_uart_save_context(struct imx_port *sport)
 	uart_port_unlock_irqrestore(&sport->port, flags);
 }
 
+/* called with irq off */
 static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 {
 	u32 ucr3;
 
+	uart_port_lock(&sport->port);
+
 	ucr3 = imx_uart_readl(sport, UCR3);
 	if (on) {
 		imx_uart_writel(sport, USR1_AWAKE, USR1);
@@ -2603,6 +2606,8 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 		}
 		imx_uart_writel(sport, ucr1, UCR1);
 	}
+
+	uart_port_unlock(&sport->port);
 }
 
 static int imx_uart_suspend_noirq(struct device *dev)

-- 
2.46.0

[PATCH 2/2] serial: imx: Add more comments on port lock status

[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;
@@ -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;

-- 
2.46.0

Re: [PATCH 2/2] serial: imx: Add more comments on port lock status

[Alexander Stein]

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;
> 
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/2] serial: imx: Add more comments on port lock status

[Esben Haabendal]

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