* [PATCH v2] serial: imx: Fix sysrq deadlock
@ 2021-09-29 21:43 Fabio Estevam
2021-09-30 7:02 ` Uwe Kleine-König
2021-09-30 7:54 ` Johan Hovold
0 siblings, 2 replies; 7+ messages in thread
From: Fabio Estevam @ 2021-09-29 21:43 UTC (permalink / raw)
To: gregkh; +Cc: michael, linux-serial, johan, marex, Fabio Estevam
The following sysrq command causes the following deadlock:
# echo t > /proc/sysrq-trigger
....
[ 20.325246] ======================================================
[ 20.325252] WARNING: possible circular locking dependency detected
[ 20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
Not tainted
[ 20.325273] ------------------------------------------------------
[ 20.325279] sh/236 is trying to acquire lock:
[ 20.325293] c1618614 (console_owner){-...}-{0:0}, at:
console_unlock+0x180/0x5bc
[ 20.325361]
[ 20.325361] but task is already holding lock:
[ 20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
show_workqueue_state+0x104/0x3c8
[ 20.325432]
[ 20.325432] which lock already depends on the new lock.
...
[ 20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
[ 20.325690] __queue_work+0x114/0x810
[ 20.325710] queue_work_on+0x54/0x94
[ 20.325727] __imx_uart_rxint.constprop.0+0x1b4/0x2e0
[ 20.325760] imx_uart_int+0x270/0x310
This problem happens because uart_handle_sysrq_char() is called
with the lock held.
Fix this by using the same approach done in commit 5697df7322fe ("serial:
fsl_lpuart: split sysrq handling"), which calls
uart_unlock_and_check_sysrq() to drop the lock prior to
uart_handle_sysrq_char().
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- I noticed that when sending break + t via the terminal, the characters
were sometimes lost. Do the minimal changes to fix the deadlock without
missing the sysrq input.
drivers/tty/serial/imx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b121cd869e9..1c768dd3896d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
unsigned int rx, flg, ignored = 0;
struct tty_port *port = &sport->port.state->port;
+ uart_unlock_and_check_sysrq(&sport->port);
while (imx_uart_readl(sport, USR2) & USR2_RDR) {
u32 usr2;
@@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
out:
tty_flip_buffer_push(port);
+ spin_lock(&sport->port.lock);
return IRQ_HANDLED;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-09-29 21:43 [PATCH v2] serial: imx: Fix sysrq deadlock Fabio Estevam
@ 2021-09-30 7:02 ` Uwe Kleine-König
2021-09-30 7:54 ` Johan Hovold
1 sibling, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2021-09-30 7:02 UTC (permalink / raw)
To: Fabio Estevam; +Cc: gregkh, michael, linux-serial, johan, marex
[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]
Hello Fabio,
On Wed, Sep 29, 2021 at 06:43:24PM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following deadlock:
>
> # echo t > /proc/sysrq-trigger
> ....
> [ 20.325246] ======================================================
> [ 20.325252] WARNING: possible circular locking dependency detected
> [ 20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
> Not tainted
> [ 20.325273] ------------------------------------------------------
> [ 20.325279] sh/236 is trying to acquire lock:
> [ 20.325293] c1618614 (console_owner){-...}-{0:0}, at:
> console_unlock+0x180/0x5bc
> [ 20.325361]
> [ 20.325361] but task is already holding lock:
> [ 20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
> show_workqueue_state+0x104/0x3c8
> [ 20.325432]
> [ 20.325432] which lock already depends on the new lock.
>
> ...
>
> [ 20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
> [ 20.325690] __queue_work+0x114/0x810
> [ 20.325710] queue_work_on+0x54/0x94
> [ 20.325727] __imx_uart_rxint.constprop.0+0x1b4/0x2e0
> [ 20.325760] imx_uart_int+0x270/0x310
>
> This problem happens because uart_handle_sysrq_char() is called
> with the lock held.
>
> Fix this by using the same approach done in commit 5697df7322fe ("serial:
> fsl_lpuart: split sysrq handling"), which calls
> uart_unlock_and_check_sysrq() to drop the lock prior to
> uart_handle_sysrq_char().
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - I noticed that when sending break + t via the terminal, the characters
> were sometimes lost. Do the minimal changes to fix the deadlock without
> missing the sysrq input.
>
> drivers/tty/serial/imx.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..1c768dd3896d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> unsigned int rx, flg, ignored = 0;
> struct tty_port *port = &sport->port.state->port;
>
> + uart_unlock_and_check_sysrq(&sport->port);
> while (imx_uart_readl(sport, USR2) & USR2_RDR) {
> u32 usr2;
>
> @@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> out:
> tty_flip_buffer_push(port);
>
> + spin_lock(&sport->port.lock);
> return IRQ_HANDLED;
Hmm, this releases the port lock. Are you sure it's correct to e.g.
modify sport->port.icount and various registers and call serial core
functions without holding it?
Also consider imx1 where we have a different irq for tx, rx and
handshaking, so unlocking port.lock might result in a call to
imx_uart_txint or imx_uart_rtsint.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-09-29 21:43 [PATCH v2] serial: imx: Fix sysrq deadlock Fabio Estevam
2021-09-30 7:02 ` Uwe Kleine-König
@ 2021-09-30 7:54 ` Johan Hovold
2021-09-30 13:45 ` Fabio Estevam
1 sibling, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2021-09-30 7:54 UTC (permalink / raw)
To: Fabio Estevam; +Cc: gregkh, michael, linux-serial, marex
On Wed, Sep 29, 2021 at 06:43:24PM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following deadlock:
>
> # echo t > /proc/sysrq-trigger
> ....
> [ 20.325246] ======================================================
> [ 20.325252] WARNING: possible circular locking dependency detected
> [ 20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
> Not tainted
> [ 20.325273] ------------------------------------------------------
> [ 20.325279] sh/236 is trying to acquire lock:
> [ 20.325293] c1618614 (console_owner){-...}-{0:0}, at:
> console_unlock+0x180/0x5bc
> [ 20.325361]
> [ 20.325361] but task is already holding lock:
> [ 20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
> show_workqueue_state+0x104/0x3c8
> [ 20.325432]
> [ 20.325432] which lock already depends on the new lock.
>
> ...
>
> [ 20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
> [ 20.325690] __queue_work+0x114/0x810
> [ 20.325710] queue_work_on+0x54/0x94
> [ 20.325727] __imx_uart_rxint.constprop.0+0x1b4/0x2e0
> [ 20.325760] imx_uart_int+0x270/0x310
>
> This problem happens because uart_handle_sysrq_char() is called
> with the lock held.
>
> Fix this by using the same approach done in commit 5697df7322fe ("serial:
> fsl_lpuart: split sysrq handling"), which calls
> uart_unlock_and_check_sysrq() to drop the lock prior to
> uart_handle_sysrq_char().
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - I noticed that when sending break + t via the terminal, the characters
> were sometimes lost. Do the minimal changes to fix the deadlock without
> missing the sysrq input.
>
> drivers/tty/serial/imx.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..1c768dd3896d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> unsigned int rx, flg, ignored = 0;
> struct tty_port *port = &sport->port.state->port;
>
> + uart_unlock_and_check_sysrq(&sport->port);
This is just so broken; you can't just drop the lock. And you clearly
haven't even tried to understand how uart_unlock_and_check_sysrq()
works.
Please take a closer look at the commit you're trying to mimic.
> while (imx_uart_readl(sport, USR2) & USR2_RDR) {
> u32 usr2;
>
> @@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> out:
> tty_flip_buffer_push(port);
>
> + spin_lock(&sport->port.lock);
> return IRQ_HANDLED;
> }
Johan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-09-30 7:54 ` Johan Hovold
@ 2021-09-30 13:45 ` Fabio Estevam
2021-10-01 7:52 ` Johan Hovold
0 siblings, 1 reply; 7+ messages in thread
From: Fabio Estevam @ 2021-09-30 13:45 UTC (permalink / raw)
To: Johan Hovold; +Cc: gregkh, michael, linux-serial, marex, u.kleine-koenig
Hi Johan,
On 30/09/2021 04:54, Johan Hovold wrote:
> This is just so broken; you can't just drop the lock. And you clearly
> haven't even tried to understand how uart_unlock_and_check_sysrq()
> works.
>
> Please take a closer look at the commit you're trying to mimic.
Thanks for the feedback.
I have changed it to:
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b121cd869e9..b7cda50602d5 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void
*dev_id)
continue;
}
- if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
+ if (uart_prepare_sysrq_char(&sport->port, rx))
continue;
if (unlikely(rx & URXD_ERR)) {
@@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void
*dev_id)
}
out:
+ uart_unlock_and_check_sysrq(&sport->port);
tty_flip_buffer_push(port);
return IRQ_HANDLED;
@@ -959,6 +960,7 @@ static irqreturn_t imx_uart_int(int irq, void
*dev_id)
imx_uart_writel(sport, USR1_AGTIM, USR1);
__imx_uart_rxint(irq, dev_id);
+ spin_lock(&sport->port.lock);
ret = IRQ_HANDLED;
}
@@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, const
char *s, unsigned int count)
unsigned int ucr1;
int locked = 1;
- if (sport->port.sysrq)
- locked = 0;
- else if (oops_in_progress)
+ if (oops_in_progress)
locked = spin_trylock_irqsave(&sport->port.lock, flags);
else
spin_lock_irqsave(&sport->port.lock, flags);
This makes the deadlock not happen after running:
echo t > /proc/sysrq-trigger
, but entering <break> + t via the console does not work anymore.
It returns the sysrq help instead:
sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d)
terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i)
thaw-filesystems(j) sak(k) show-backtrace-all-active-cpu
s(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o)
show-registers(p) show-all-timers(q) unraw(r) sync(s)
show-task-states(t) unmount(u) show-blocked-tasks(w)
dump-ftrace-buffer(z)
Thanks
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-09-30 13:45 ` Fabio Estevam
@ 2021-10-01 7:52 ` Johan Hovold
2021-10-01 10:17 ` Fabio Estevam
0 siblings, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2021-10-01 7:52 UTC (permalink / raw)
To: Fabio Estevam; +Cc: gregkh, michael, linux-serial, marex, u.kleine-koenig
On Thu, Sep 30, 2021 at 10:45:31AM -0300, Fabio Estevam wrote:
> Hi Johan,
>
> On 30/09/2021 04:54, Johan Hovold wrote:
>
> > This is just so broken; you can't just drop the lock. And you clearly
> > haven't even tried to understand how uart_unlock_and_check_sysrq()
> > works.
> >
> > Please take a closer look at the commit you're trying to mimic.
>
> Thanks for the feedback.
>
> I have changed it to:
>
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..b7cda50602d5 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void
> *dev_id)
> continue;
> }
>
> - if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> + if (uart_prepare_sysrq_char(&sport->port, rx))
Why did you drop the cast? If there's anything in the high bits you'd
see the help text printed as you report below (even if it seems
unlikely).
> continue;
>
> if (unlikely(rx & URXD_ERR)) {
> @@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void
> *dev_id)
> }
>
> out:
> + uart_unlock_and_check_sysrq(&sport->port);
> tty_flip_buffer_push(port);
>
> return IRQ_HANDLED;
> @@ -959,6 +960,7 @@ static irqreturn_t imx_uart_int(int irq, void
> *dev_id)
> imx_uart_writel(sport, USR1_AGTIM, USR1);
>
> __imx_uart_rxint(irq, dev_id);
> + spin_lock(&sport->port.lock);
> ret = IRQ_HANDLED;
> }
It's a step in the right direction, but you need to restructure the code
so that you don't need to drop and reacquire the lock.
> @@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, const
> char *s, unsigned int count)
> unsigned int ucr1;
> int locked = 1;
>
> - if (sport->port.sysrq)
> - locked = 0;
> - else if (oops_in_progress)
> + if (oops_in_progress)
> locked = spin_trylock_irqsave(&sport->port.lock, flags);
> else
> spin_lock_irqsave(&sport->port.lock, flags);
And you need to fix the commit summary and commit message since you're
actually fixing any deadlock. You're just suppressing a false positive
lockdep warning due to the above sysrq hack.
> This makes the deadlock not happen after running:
> echo t > /proc/sysrq-trigger
>
> , but entering <break> + t via the console does not work anymore.
>
>
> It returns the sysrq help instead:
>
> sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d)
> terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i)
> thaw-filesystems(j) sak(k) show-backtrace-all-active-cpu
> s(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o)
> show-registers(p) show-all-timers(q) unraw(r) sync(s)
> show-task-states(t) unmount(u) show-blocked-tasks(w)
> dump-ftrace-buffer(z)
So either you're just pushing garbage to the sysrq handler due to the
dropped cast above or you may, for example, have a NUL char in the
receiver due to the break that you don't discard.
I'd start with logging the key that gets passed to the sysrq handler.
Johan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-10-01 7:52 ` Johan Hovold
@ 2021-10-01 10:17 ` Fabio Estevam
2021-10-01 13:48 ` Johan Hovold
0 siblings, 1 reply; 7+ messages in thread
From: Fabio Estevam @ 2021-10-01 10:17 UTC (permalink / raw)
To: Johan Hovold
Cc: Fabio Estevam, Greg Kroah-Hartman, Michael Walle, linux-serial,
Marek Vasut, Uwe Kleine-König
Hi Johan,
On Fri, Oct 1, 2021 at 4:53 AM Johan Hovold <johan@kernel.org> wrote:
> Why did you drop the cast? If there's anything in the high bits you'd
> see the help text printed as you report below (even if it seems
> unlikely).
That was it, thanks!
I have taken your feedback into consideration and sent a v3.
The only one that I didn't do was to reorganize the code to avoid the
unlock/lock as
this would require a significant rework.
Thanks,
Fabio Estevam
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serial: imx: Fix sysrq deadlock
2021-10-01 10:17 ` Fabio Estevam
@ 2021-10-01 13:48 ` Johan Hovold
0 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2021-10-01 13:48 UTC (permalink / raw)
To: Fabio Estevam
Cc: Fabio Estevam, Greg Kroah-Hartman, Michael Walle, linux-serial,
Marek Vasut, Uwe Kleine-König
On Fri, Oct 01, 2021 at 07:17:53AM -0300, Fabio Estevam wrote:
> Hi Johan,
>
> On Fri, Oct 1, 2021 at 4:53 AM Johan Hovold <johan@kernel.org> wrote:
>
> > Why did you drop the cast? If there's anything in the high bits you'd
> > see the help text printed as you report below (even if it seems
> > unlikely).
>
> That was it, thanks!
>
> I have taken your feedback into consideration and sent a v3.
>
> The only one that I didn't do was to reorganize the code to avoid the
> unlock/lock as
> this would require a significant rework.
Judging from a quick look at the code is very straight-forward, and we
don't want to add interrupt latency just to shut up lockdep.
Johan
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-01 13:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29 21:43 [PATCH v2] serial: imx: Fix sysrq deadlock Fabio Estevam
2021-09-30 7:02 ` Uwe Kleine-König
2021-09-30 7:54 ` Johan Hovold
2021-09-30 13:45 ` Fabio Estevam
2021-10-01 7:52 ` Johan Hovold
2021-10-01 10:17 ` Fabio Estevam
2021-10-01 13:48 ` Johan Hovold
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.