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

[wang xiaolei]

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@xxxxxxxxxxxxx>
> > ---
> >   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)