Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 18 Sep 2019 16:52:52 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Steven Rostedt <rostedt@goodmis.org>,
        Petr Mladek <pmladek@suse.com>, linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: Regression in dbdda842fe96 ("printk: Add console owner and
 waiter logic to load balance console writes") [Was: Regression in
 fd5f7cde1b85 ("...")]
Message-ID: <20190918075252.GA30808@jagdpanzerIV>
References: <20190917141034.gvjg7bgylqbbxyv7@pengutronix.de>
 <20190918013032.GA2895@jagdpanzerIV>
 <20190918071158.rtw45jch2roa2wum@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190918071158.rtw45jch2roa2wum@pengutronix.de>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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