Messages in this thread Patch in this message | | | Date | Wed, 18 Sep 2019 16:52:52 +0900 | From | Sergey Senozhatsky <> | Subject | Re: Regression in dbdda842fe96 ("printk: Add console owner and waiter logic to load balance console writes") [Was: Regression in fd5f7cde1b85 ("...")] |
| |
On (09/18/19 09:11), Uwe Kleine-König wrote: > I rechecked and indeed fd5f7cde1b85's parent has the problem, too, so I > did a mistake during my bisection :-| > > Redoing the bisection (a bit quicker this time) points to > > dbdda842fe96 ("printk: Add console owner and waiter logic to load balance console writes") > > Sorry for the confusion.
No worries!
[..] > > So I'd say that lockdep is correct, but there are several hacks which > > prevent actual deadlock. > > Just to make sure, I got you right: With the way lockdep works it is > right to assume there is a problem, but in fact there isn't?
I'd probably say so... Unless I'm missing something.
sysrq-over-serial is handled from the serial driver's IRQ handler, under serial driver's port->lock. sysrq handling calls printk(), which takes console_sem/owner and re-enters the serial driver via ->write() callback.
So lockdep sees a reverse locking pattern: port->lock goes before console_sem/owner, which is not the usual order.
> This is IMHO unfortunate because such false positives reduces the > usefulness of lockdep considerably. :-|
I agree.
port->sysrq state is global to uart port. IOW, if CPUA sets port->sysrq then all printk->write() paths (from any other CPU) become lockless.
This makes me wonder is we really need to hold port->lock for uart_handle_sysrq_char(). I sort of doubt it...
Can you try the following patch? It's against linux-next, I guess you can backport to your kernel.
The basic idea is to handle sysrq out of port->lock.
I didn't test it all (not even sure if it compiles).
--- drivers/tty/serial/imx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 87c58f9f6390..f0dd807b52df 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -731,9 +731,9 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id) struct imx_port *sport = dev_id; unsigned int rx, flg, ignored = 0; struct tty_port *port = &sport->port.state->port; + unsigned long flags; - spin_lock(&sport->port.lock); - + uart_port_lock_irqsave(&sport->port, flags); while (imx_uart_readl(sport, USR2) & USR2_RDR) { u32 usr2; @@ -749,8 +749,8 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id) continue; } - if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) - continue; + if (uart_prepare_sysrq_char(&sport->port, (unsigned char)rx)) + break; if (unlikely(rx & URXD_ERR)) { if (rx & URXD_BRK) @@ -792,7 +792,7 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id) } out: - spin_unlock(&sport->port.lock); + uart_unlock_and_check_sysrq(&sport->port, flags); tty_flip_buffer_push(port); return IRQ_HANDLED; }
| |