Message-ID: <aPt3qje1IQU8i9Md@pathway.suse.cz>
Date: Fri, 24 Oct 2025 14:57:14 +0200
From: Petr Mladek <pmladek@...e.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Oleg Nesterov <oleg@...hat.com>,
	John Ogness <john.ogness@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of
 LD_WAIT_SLEEP

On Fri 2025-10-24 12:38:08, Sebastian Andrzej Siewior wrote:
> On 2025-10-24 11:35:14 [+0200], Petr Mladek wrote:
> > It is clear that the commit message and the comment above the mapping
> > caused some confusion. I thought about better wording.
> > 
> > I wanted to be as clear as possible, But the problem is that everyone
> > has different background and might understand the same term
> > differently. Also I am not a native speaker.
> > 
> > /*
> >  * Some legacy console drivers might violate raw_spinlock/spinlock nesting
> >  * rules when printk() was called under a raw_spinlock and the driver used
> >  * a spinlock. It is not a real problem because the legacy drivers should
> >  * never be called directly from printk() in PREEMPT_RT.
> >  *
> >  * This map is used to pretend that printk() was called under a normal spinlock
> >  * to hide the above described locking violation. It still allows to catch
> >  * other problems, for example, possible ABBA deadlocks or sleeping locks.
> 
> It is not "Some legacy console" but all of them. The only exception
> would if they don't use any locking. Serial driver should use
> uart_port::lock, VT has its printing_lock and so on.

I had raw_spinlock in mind. But you are right. I guess that no console
driver is using raw_spinlock for writing the entire message.
Otherwise, you would need to fix it for RT.


> Don't like the "might violate".
> "should never be called" is misleading because we know how things work
> and they must not be called. But this is minor…
> 
> But why bring ABBA deadlocks into this and sleeping locks? Especially
> since different people assume different things when "sleeping locks" is
> used. And clearly the last was not handled well :)
> 
> I would suggest simple and focus on the change and why:
> The override addresses the nesting problem on !RT which does not occur
> on RT because the code flow is different.
> 
> What about the suggested:
> 
>   The legacy console always acquires a spinlock_t from its printing
>   callback. This violates lock nesting if the caller acquired an always
>   spinning lock (raw_spinlock_t) while invoking printk(). This is not a
>   problem on PREEMPT_RT because legacy consoles print always from a
>   dedicated thread and never from within printk(). Therefore we tell
>   lockdep that a sleeping spin lock (spinlock_t) is valid here.

Looks good to me.

> >  *
> >  * The mapping is not used in PREEMPT_RT which allows to catch bugs when
> >  * the legacy console driver would get called from an atomic context by mistake.
> >  */
> > 
> > 
> > And the commit message might be:
> > 
> > <commit_message>
> > printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP
> > 
> > printk_legacy_map is used to hide possible violations of
> > raw_spinlock/spinlock nesting when printk() calls legacy console
> > drivers directly. It is not a real problem in !PREEMPT_RT mode and
>   s/real//
> 
> > the problematic code path should never be called in PREEMPT_RT mode.
> 
>   because this code path is never called on PREEMPT_RT.
> 
> > However, LD_WAIT_SLEEP is not exactly right. It fools lockdep as if it
> 
> Why is not exactly right? :)

IMHO, it was answered in the following sentence. :)

> Usually you describe _why_ you do things
> and because it wasn't right is okay if it is obvious to everyone.
> 
> > is fine to acquire a sleeping lock.
> > 
> > Change DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) to use LD_WAIT_CONFIG.
> > 
> > Also, update the comment to better describe the purpose of the mapping.
> > </commit_message>
> 
> For my taste it is too verbose and you bring too much context. It is
> *just* the lock nest override. No need to bring other aspects of lockdep
> into the game.
> 
>   printk_legacy_map is used to hide lock nesting violations caused by
>   legacy drivers and is using the wrong override type. LD_WAIT_SLEEP is
>   for always sleeping lock types such as mutex_t. LD_WAIT_CONFIG is for
>   lock type which are sleeping while spinning on PREEMPT_RT such as
>   spinlock_t.

Looks goot to me.

> > Is this better and acceptable, please?
> > If not then please provide alternatives ;-)
> 
> I made some suggestions. However you got rid of the points I complained
> about initially so I fine with it. Thank you.

JFYI, I do not mind which version is used.

Best Regards,
Petr

Powered by blists - more mailing lists