Regression in dbdda842fe96 ("printk: Add console owner and waiter logic to load balance console writes") [Was: Regression in fd5f7cde1b85 ("...")]

[Uwe Kleine-König]

Hello Sergey,

On Wed, Sep 18, 2019 at 10:30:32AM +0900, Sergey Senozhatsky wrote:
> On (09/17/19 16:10), Uwe Kleine-König wrote:
> > Hello,
> >
> > Today it saw sysrq on an UART driven by drivers/tty/serial/imx.c report
> > a lockdep issue. Bisecting pointed to
> >
> > 	fd5f7cde1b85 ("printk: Never set console_may_schedule in console_trylock()")
> 
> Hmmm...
> 
> I don't see how this patch can affect anything. It simply
> disables preemption in printk().

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.

> > When I type <break>t I get:
> > 
> > [   87.940104] sysrq: SysRq : This sysrq operation is disabled.
> > [   87.948752] 
> > [   87.948772] ======================================================
> > [   87.948787] WARNING: possible circular locking dependency detected
> > [   87.948798] 4.14.0-12954-gfd5f7cde1b85 #26 Not tainted
> > [   87.948813] ------------------------------------------------------
> > [   87.948822] swapper/0 is trying to acquire lock:
> > [   87.948829]  (console_owner){-...}, at: [<c015e438>] console_unlock+0x110/0x598
> > [   87.948861] 
> > [   87.948869] but task is already holding lock:
> > [   87.948874]  (&port_lock_key){-.-.}, at: [<c048d5b0>] imx_rxint+0x2c/0x290
> > [   87.948902] 
> > [   87.948911] which lock already depends on the new lock.
> > [   87.948917] 
> > [   87.948923] 
> > [   87.948932] the existing dependency chain (in reverse order) is:
> > [   87.948938] 
> > [   87.948943] -> #1 (&port_lock_key){-.-.}:
> > [   87.948975]        _raw_spin_lock_irqsave+0x5c/0x70
> > [   87.948983]        imx_console_write+0x138/0x15c
> > [   87.948991]        console_unlock+0x204/0x598
> > [   87.949000]        register_console+0x21c/0x3e8
> > [   87.949008]        uart_add_one_port+0x3e4/0x4dc
> > [   87.949019]        platform_drv_probe+0x3c/0x78
> > [   87.949027]        driver_probe_device+0x25c/0x47c
> > [   87.949035]        __driver_attach+0xec/0x114
> > [   87.949044]        bus_for_each_dev+0x80/0xb0
> > [   87.949054]        bus_add_driver+0x1d4/0x264
> > [   87.949062]        driver_register+0x80/0xfc
> > [   87.949069]        imx_serial_init+0x28/0x48
> > [   87.949078]        do_one_initcall+0x44/0x18c
> > [   87.949087]        kernel_init_freeable+0x11c/0x1cc
> > [   87.949095]        kernel_init+0x10/0x114
> > [   87.949103]        ret_from_fork+0x14/0x30
> 
> This is "normal" locking path
> 
> 	console_sem -> port->lock
> 
> 	printk()
> 	 lock console_sem
> 	  imx_console_write()
> 	   lock port->lock
> 
> > [   87.949113] -> #0 (console_owner){-...}:
> > [   87.949145]        lock_acquire+0x100/0x23c
> > [   87.949154]        console_unlock+0x1a4/0x598
> > [   87.949162]        vprintk_emit+0x1a4/0x45c
> > [   87.949171]        vprintk_default+0x28/0x30
> > [   87.949180]        printk+0x28/0x38
> > [   87.949189]        __handle_sysrq+0x1c4/0x244
> > [   87.949196]        imx_rxint+0x258/0x290
> > [   87.949206]        imx_int+0x170/0x178
> > [   87.949216]        __handle_irq_event_percpu+0x78/0x418
> > [   87.949225]        handle_irq_event_percpu+0x24/0x6c
> > [   87.949233]        handle_irq_event+0x40/0x64
> > [   87.949242]        handle_level_irq+0xb4/0x138
> > [   87.949252]        generic_handle_irq+0x28/0x3c
> > [   87.949261]        __handle_domain_irq+0x50/0xb0
> > [   87.949269]        avic_handle_irq+0x3c/0x5c
> > [   87.949277]        __irq_svc+0x6c/0xa4
> > [   87.949287]        arch_cpu_idle+0x30/0x40
> > [   87.949297]        arch_cpu_idle+0x30/0x40
> > [   87.949305]        do_idle+0xa0/0x104
> > [   87.949313]        cpu_startup_entry+0x14/0x18
> > [   87.949323]        start_kernel+0x30c/0x368
> 
> This one is a "reverse" locking path...
> 
> 	port->lock -> console_sem
> 
> There is more to it:
> 
>  imxint()
>   lock port->lock
>    uart_handle_sysrq_char()
>     handle_sysrq()
>      printk()
>       lock conosole_sem
>        imx_console_write()
>         lock port->lock			[boom]
> 
> This path re-enters serial driver. But it doesn't deadlock, because
> uart_handle_sysrq_char() sets a special flag port->sysrq, and serial
> consoles are expected to make sure that they don't lock port->lock
> in this case. Otherwise we will kill the system:
> 
> 	void serial_console_write(...)
> 	{
> 	...
>           if (sport->port.sysrq)
>                   locked = 0;
>           else if (oops_in_progress)
>                   locked = spin_trylock_irqsave(&sport->port.lock, flags);
>           else
>                   spin_lock_irqsave(&sport->port.lock, flags);
>  	...
> 	}
> 
> 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?
This is IMHO unfortunate because such false positives reduces the
usefulness of lockdep considerably. :-|

> No idea why bisection has pointed at fd5f7cde1b85, it really doesn't
> change the locking patterns.

See above. I bent off wrongly during bisection and dbdda842fe96
("printk: Add console owner and waiter logic to load balance console
writes") is the first commit that issues the lockdep splat. I guess that
doesn't change what you said above though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |