1. Patchew
  2. linux
  3. View series

[PATCH] serial: imx: Use uart_port_lock_irqsave() instead of uart_port_lock()

Xiaolei Wang posted 1 patch 1 year ago 
drivers/tty/serial/imx.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[PATCH] serial: imx: Use uart_port_lock_irqsave() instead of uart_port_lock()
Posted by Xiaolei Wang 1 year ago 
When executing 'ehco mem > /sys/power/state', the following
deadlock occurs. Since there is output during the serial
port entering the suspend process, the suspend will be
interrupted, resulting in the nesting of locks. Therefore,
use uart_port_lock_irqsave instead of uart_port_unlock.

WARNING: inconsistent lock state
6.12.0-rc2-00002-g3c199ed5bd64-dirty #23 Not tainted
--------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
sh/494 [HC0[0]:SC0[0]:HE1:SE1] takes:
c4db5850 (&port_lock_key){?.-.}-{3:3}, at: imx_uart_enable_wakeup+0x14/0x254
{IN-HARDIRQ-W} state was registered at:
  lock_acquire+0x104/0x348
  _raw_spin_lock+0x48/0x84
  imx_uart_int+0x14/0x4dc
  __handle_irq_event_percpu+0xac/0x2fc
  handle_irq_event_percpu+0xc/0x40
  handle_irq_event+0x38/0x8c
  handle_fasteoi_irq+0xb4/0x1b8
  handle_irq_desc+0x1c/0x2c
  gic_handle_irq+0x6c/0xa0
  generic_handle_arch_irq+0x2c/0x64
  call_with_stack+0x18/0x20
  __irq_svc+0x9c/0xbc
  _raw_spin_unlock_irqrestore+0x2c/0x48
  uart_write+0xd8/0x3a0
  do_output_char+0x1a8/0x1e4
  n_tty_write+0x224/0x440
  file_tty_write.constprop.0+0x124/0x250
  do_iter_readv_writev+0x100/0x1e0
  vfs_writev+0xc4/0x448
  do_writev+0x68/0xf8
  ret_fast_syscall+0x0/0x1c
irq event stamp: 31593
hardirqs last  enabled at (31593): [<c1150e48>] _raw_spin_unlock_irqrestore+0x44/0x48
hardirqs last disabled at (31592): [<c07f32f0>] clk_enable_lock+0x60/0x120
softirqs last  enabled at (30334): [<c012d1d4>] handle_softirqs+0x2cc/0x478
softirqs last disabled at (30325): [<c012d510>] __irq_exit_rcu+0x120/0x15c

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&port_lock_key);
  <Interrupt>
    lock(&port_lock_key);

Fixes: 3c199ed5bd64 ("serial: imx: Grab port lock in imx_uart_enable_wakeup()")
Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
 drivers/tty/serial/imx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 17f70e4bee43..4470bcb3ef19 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2691,8 +2691,9 @@ static void imx_uart_save_context(struct imx_port *sport)
 static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 {
 	u32 ucr3;
+	unsigned long flags;
 
-	uart_port_lock(&sport->port);
+	uart_port_lock_irqsave(&sport->port, &flags);
 
 	ucr3 = imx_uart_readl(sport, UCR3);
 	if (on) {
@@ -2714,7 +2715,7 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 		imx_uart_writel(sport, ucr1, UCR1);
 	}
 
-	uart_port_unlock(&sport->port);
+	uart_port_unlock_irqrestore(&sport->port, flags);
 }
 
 static int imx_uart_suspend_noirq(struct device *dev)
-- 
2.25.1
Re: [PATCH] serial: imx: Use uart_port_lock_irqsave() instead of uart_port_lock()
Posted by Lucas Stach 1 year ago 
Am Montag, dem 09.12.2024 um 20:47 +0800 schrieb Xiaolei Wang:
> When executing 'ehco mem > /sys/power/state', the following
> deadlock occurs. Since there is output during the serial
> port entering the suspend process, the suspend will be
> interrupted, resulting in the nesting of locks. Therefore,
> use uart_port_lock_irqsave instead of uart_port_unlock.
> 
> WARNING: inconsistent lock state
> 6.12.0-rc2-00002-g3c199ed5bd64-dirty #23 Not tainted
> --------------------------------
> inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> sh/494 [HC0[0]:SC0[0]:HE1:SE1] takes:
> c4db5850 (&port_lock_key){?.-.}-{3:3}, at: imx_uart_enable_wakeup+0x14/0x254
> {IN-HARDIRQ-W} state was registered at:
>   lock_acquire+0x104/0x348
>   _raw_spin_lock+0x48/0x84
>   imx_uart_int+0x14/0x4dc
>   __handle_irq_event_percpu+0xac/0x2fc
>   handle_irq_event_percpu+0xc/0x40
>   handle_irq_event+0x38/0x8c
>   handle_fasteoi_irq+0xb4/0x1b8
>   handle_irq_desc+0x1c/0x2c
>   gic_handle_irq+0x6c/0xa0
>   generic_handle_arch_irq+0x2c/0x64
>   call_with_stack+0x18/0x20
>   __irq_svc+0x9c/0xbc
>   _raw_spin_unlock_irqrestore+0x2c/0x48
>   uart_write+0xd8/0x3a0
>   do_output_char+0x1a8/0x1e4
>   n_tty_write+0x224/0x440
>   file_tty_write.constprop.0+0x124/0x250
>   do_iter_readv_writev+0x100/0x1e0
>   vfs_writev+0xc4/0x448
>   do_writev+0x68/0xf8
>   ret_fast_syscall+0x0/0x1c
> irq event stamp: 31593
> hardirqs last  enabled at (31593): [<c1150e48>] _raw_spin_unlock_irqrestore+0x44/0x48
> hardirqs last disabled at (31592): [<c07f32f0>] clk_enable_lock+0x60/0x120
> softirqs last  enabled at (30334): [<c012d1d4>] handle_softirqs+0x2cc/0x478
> softirqs last disabled at (30325): [<c012d510>] __irq_exit_rcu+0x120/0x15c
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&port_lock_key);
>   <Interrupt>
>     lock(&port_lock_key);
> 
> Fixes: 3c199ed5bd64 ("serial: imx: Grab port lock in imx_uart_enable_wakeup()")
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>  drivers/tty/serial/imx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 17f70e4bee43..4470bcb3ef19 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2691,8 +2691,9 @@ static void imx_uart_save_context(struct imx_port *sport)
>  static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>  {
>  	u32 ucr3;
> +	unsigned long flags;
>  
> -	uart_port_lock(&sport->port);
> +	uart_port_lock_irqsave(&sport->port, &flags);

This function is only called from process context, so it should be fine
to use uart_port_lock_irq().

Regards,
Lucas

>  
>  	ucr3 = imx_uart_readl(sport, UCR3);
>  	if (on) {
> @@ -2714,7 +2715,7 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>  		imx_uart_writel(sport, ucr1, UCR1);
>  	}
>  
> -	uart_port_unlock(&sport->port);
> +	uart_port_unlock_irqrestore(&sport->port, flags);
>  }
>  
>  static int imx_uart_suspend_noirq(struct device *dev)
Re: [PATCH] serial: imx: Use uart_port_lock_irqsave() instead of uart_port_lock()
Posted by wang xiaolei 1 year ago 
On 12/10/24 1:37 AM, Lucas Stach wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Am Montag, dem 09.12.2024 um 20:47 +0800 schrieb Xiaolei Wang:
>> When executing 'ehco mem > /sys/power/state', the following
>> deadlock occurs. Since there is output during the serial
>> port entering the suspend process, the suspend will be
>> interrupted, resulting in the nesting of locks. Therefore,
>> use uart_port_lock_irqsave instead of uart_port_unlock.
>>
>> WARNING: inconsistent lock state
>> 6.12.0-rc2-00002-g3c199ed5bd64-dirty #23 Not tainted
>> --------------------------------
>> inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
>> sh/494 [HC0[0]:SC0[0]:HE1:SE1] takes:
>> c4db5850 (&port_lock_key){?.-.}-{3:3}, at: imx_uart_enable_wakeup+0x14/0x254
>> {IN-HARDIRQ-W} state was registered at:
>>    lock_acquire+0x104/0x348
>>    _raw_spin_lock+0x48/0x84
>>    imx_uart_int+0x14/0x4dc
>>    __handle_irq_event_percpu+0xac/0x2fc
>>    handle_irq_event_percpu+0xc/0x40
>>    handle_irq_event+0x38/0x8c
>>    handle_fasteoi_irq+0xb4/0x1b8
>>    handle_irq_desc+0x1c/0x2c
>>    gic_handle_irq+0x6c/0xa0
>>    generic_handle_arch_irq+0x2c/0x64
>>    call_with_stack+0x18/0x20
>>    __irq_svc+0x9c/0xbc
>>    _raw_spin_unlock_irqrestore+0x2c/0x48
>>    uart_write+0xd8/0x3a0
>>    do_output_char+0x1a8/0x1e4
>>    n_tty_write+0x224/0x440
>>    file_tty_write.constprop.0+0x124/0x250
>>    do_iter_readv_writev+0x100/0x1e0
>>    vfs_writev+0xc4/0x448
>>    do_writev+0x68/0xf8
>>    ret_fast_syscall+0x0/0x1c
>> irq event stamp: 31593
>> hardirqs last  enabled at (31593): [<c1150e48>] _raw_spin_unlock_irqrestore+0x44/0x48
>> hardirqs last disabled at (31592): [<c07f32f0>] clk_enable_lock+0x60/0x120
>> softirqs last  enabled at (30334): [<c012d1d4>] handle_softirqs+0x2cc/0x478
>> softirqs last disabled at (30325): [<c012d510>] __irq_exit_rcu+0x120/0x15c
>>
>> other info that might help us debug this:
>>   Possible unsafe locking scenario:
>>
>>         CPU0
>>         ----
>>    lock(&port_lock_key);
>>    <Interrupt>
>>      lock(&port_lock_key);
>>
>> Fixes: 3c199ed5bd64 ("serial: imx: Grab port lock in imx_uart_enable_wakeup()")
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   drivers/tty/serial/imx.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 17f70e4bee43..4470bcb3ef19 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -2691,8 +2691,9 @@ static void imx_uart_save_context(struct imx_port *sport)
>>   static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>>   {
>>        u32 ucr3;
>> +     unsigned long flags;
>>
>> -     uart_port_lock(&sport->port);
>> +     uart_port_lock_irqsave(&sport->port, &flags);
> This function is only called from process context, so it should be fine
> to use uart_port_lock_irq().
Thank you for your reminder, Lucas, I will send a new version

thanks
xiaolei
>
> Regards,
> Lucas
>
>>        ucr3 = imx_uart_readl(sport, UCR3);
>>        if (on) {
>> @@ -2714,7 +2715,7 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>>                imx_uart_writel(sport, ucr1, UCR1);
>>        }
>>
>> -     uart_port_unlock(&sport->port);
>> +     uart_port_unlock_irqrestore(&sport->port, flags);
>>   }
>>
>>   static int imx_uart_suspend_noirq(struct device *dev)

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.