[PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.

However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
is fine to acquire a sleeping lock.

Change DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) to use LD_WAIT_CONFIG.

(We can also make printk_legacy_allow_spinlock_enter/exit() depend on
 !PREEMPT_RT && CONFIG_PROVE_RAW_LOCK_NESTING)

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/printk/printk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5aee9ffb16b9..f11b2f31999b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3007,7 +3007,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
  * false positive. For PREEMPT_RT the false positive condition does not
  * occur.
  *
- * This map is used to temporarily establish LD_WAIT_SLEEP context for the
+ * This map is used to temporarily establish LD_WAIT_CONFIG context for the
  * console write() callback when legacy printing to avoid false positive
  * lockdep complaints, thus allowing lockdep to continue to function for
  * real issues.
@@ -3016,7 +3016,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 static inline void printk_legacy_allow_spinlock_enter(void) { }
 static inline void printk_legacy_allow_spinlock_exit(void) { }
 #else
-static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
+static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
 
 static inline void printk_legacy_allow_spinlock_enter(void)
 {
-- 
2.25.1.362.g51ebf55

Re: [PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Petr Mladek]

On Wed 2025-10-22 17:41:15, Oleg Nesterov wrote:
> printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.
> 
> However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
> is fine to acquire a sleeping lock.
> 
> Change DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) to use LD_WAIT_CONFIG.
> 
> (We can also make printk_legacy_allow_spinlock_enter/exit() depend on
>  !PREEMPT_RT && CONFIG_PROVE_RAW_LOCK_NESTING)

I do not have strong opinion about adding (&& CONFIG_PROVE_RAW_LOCK_NESTING).
This dependency is already handled in LD_WAIT_CONFIG definition.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Anyway, the change makes sense to me. It seems that this better fits
the purpose.

Reviewed-by: Petr Mladek <pmladek@suse.com>

See a note below.

> ---
>  kernel/printk/printk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5aee9ffb16b9..f11b2f31999b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3007,7 +3007,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>   * false positive. For PREEMPT_RT the false positive condition does not
>   * occur.

From the comment, it was not obvious to me why the condition does not
occur for PREEMPT_RT. I had to check the commit message from the
commit daeed1595b4ddf314b ("printk: Avoid false positive lockdep
report for legacy printing").

<paste>
    However, on PREEMPT_RT the printing path from atomic context is
    always avoided and the console driver is always invoked from a
    dedicated thread. Thus the lockdep splat on !PREEMPT_RT is a
    false positive.
 </paste>

This is much more clear. It might make sense to improve the comment,
for example:

<proposal>
/*
 * Legacy console printing from printk() caller context does not respect
 * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
 * path from atomic context is always avoided and the console driver
 * is always invoked from a dedicated thread. Thus the lockdep splat
 * on !PREEMPT_RT is a false positive.
 *
 * This map is used to temporarily establish LD_WAIT_CONFIG context for the
 * console write() callback when legacy printing to avoid false positive
 * lockdep complaints, thus allowing lockdep to continue to function for
 * real issues.
 */
</proposal>

But it can be done in a separate patch...

>   *
> - * This map is used to temporarily establish LD_WAIT_SLEEP context for the
> + * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>   * console write() callback when legacy printing to avoid false positive
>   * lockdep complaints, thus allowing lockdep to continue to function for
>   * real issues.
> @@ -3016,7 +3016,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  static inline void printk_legacy_allow_spinlock_enter(void) { }
>  static inline void printk_legacy_allow_spinlock_exit(void) { }
>  #else
> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
>  
>  static inline void printk_legacy_allow_spinlock_enter(void)
>  {

Best Regards,
Petr

PS: I would take this patch via the printk tree. But I am going to wait
    for feedback from others (John, Sebastian, ...).

Re: [PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[John Ogness]

On 2025-10-23, Petr Mladek <pmladek@suse.com> wrote:
>> printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
>> CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.
>> 
>> However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
>> is fine to acquire a sleeping lock.
>> 
>> Change DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) to use LD_WAIT_CONFIG.
>> 
>> (We can also make printk_legacy_allow_spinlock_enter/exit() depend on
>>  !PREEMPT_RT && CONFIG_PROVE_RAW_LOCK_NESTING)
>
> I do not have strong opinion about adding (&& CONFIG_PROVE_RAW_LOCK_NESTING).
> This dependency is already handled in LD_WAIT_CONFIG definition.

I prefer avoiding CONFIG_PROVE_RAW_LOCK_NESTING since it is not necessary.

>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Anyway, the change makes sense to me. It seems that this better fits
> the purpose.
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> See a note below.
>
>> ---
>>  kernel/printk/printk.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 5aee9ffb16b9..f11b2f31999b 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3007,7 +3007,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>>   * false positive. For PREEMPT_RT the false positive condition does not
>>   * occur.
>
> From the comment, it was not obvious to me why the condition does not
> occur for PREEMPT_RT. I had to check the commit message from the
> commit daeed1595b4ddf314b ("printk: Avoid false positive lockdep
> report for legacy printing").
>
> <paste>
>     However, on PREEMPT_RT the printing path from atomic context is
>     always avoided and the console driver is always invoked from a
>     dedicated thread. Thus the lockdep splat on !PREEMPT_RT is a
>     false positive.
>  </paste>
>
> This is much more clear. It might make sense to improve the comment,
> for example:
>
> <proposal>
> /*
>  * Legacy console printing from printk() caller context does not respect
>  * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
>  * path from atomic context is always avoided and the console driver
>  * is always invoked from a dedicated thread. Thus the lockdep splat
>  * on !PREEMPT_RT is a false positive.
>  *
>  * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>  * console write() callback when legacy printing to avoid false positive
>  * lockdep complaints, thus allowing lockdep to continue to function for
>  * real issues.
>  */
> </proposal>

I am OK with this proposal.

> But it can be done in a separate patch...
>
>>   *
>> - * This map is used to temporarily establish LD_WAIT_SLEEP context for the
>> + * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>>   * console write() callback when legacy printing to avoid false positive
>>   * lockdep complaints, thus allowing lockdep to continue to function for
>>   * real issues.
>> @@ -3016,7 +3016,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>>  static inline void printk_legacy_allow_spinlock_enter(void) { }
>>  static inline void printk_legacy_allow_spinlock_exit(void) { }
>>  #else
>> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
>> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
>>  
>>  static inline void printk_legacy_allow_spinlock_enter(void)
>>  {
>
> Best Regards,
> Petr
>
> PS: I would take this patch via the printk tree. But I am going to wait
>     for feedback from others (John, Sebastian, ...).

AFAICT LD_WAIT_CONFIG would be an improvement by allowing detection of
non-spinlock-sleeping.

Reviewed-by: John Ogness <john.ogness@linutronix.de>

I would like to see an official ACK from Sebastian as well.

Re: [PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

On 10/23, Petr Mladek wrote:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks!

> This is much more clear. It might make sense to improve the comment,
> for example:
>
> <proposal>
> /*
>  * Legacy console printing from printk() caller context does not respect
>  * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
>  * path from atomic context is always avoided and the console driver
>  * is always invoked from a dedicated thread. Thus the lockdep splat
>  * on !PREEMPT_RT is a false positive.
>  *
>  * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>  * console write() callback when legacy printing to avoid false positive
>  * lockdep complaints, thus allowing lockdep to continue to function for
>  * real issues.
>  */
> </proposal>

Agreed, will do and send V2.

Oleg.

Re: [PATCH] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

On 10/23, John Ogness wrote:
>
> I prefer avoiding CONFIG_PROVE_RAW_LOCK_NESTING since it is not necessary.

OK,

> Reviewed-by: John Ogness <john.ogness@linutronix.de>

Thanks! I'll send v2 in a minute with yours and Petr's acks preserved.

> I would like to see an official ACK from Sebastian as well.

Yes, me too, even if we already dicsussed this in another thread
https://lore.kernel.org/all/20250811105948.OafBprND@linutronix.de/

Oleg.

[PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.

However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
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 make it more clear as suggested by Petr.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5aee9ffb16b9..80f0bedf5cb7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3003,11 +3003,12 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 
 /*
  * Legacy console printing from printk() caller context does not respect
- * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
- * false positive. For PREEMPT_RT the false positive condition does not
- * occur.
+ * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
+ * path from atomic context is always avoided and the console driver
+ * is always invoked from a dedicated thread. Thus the lockdep splat
+ * on !PREEMPT_RT is a false positive.
  *
- * This map is used to temporarily establish LD_WAIT_SLEEP context for the
+ * This map is used to temporarily establish LD_WAIT_CONFIG context for the
  * console write() callback when legacy printing to avoid false positive
  * lockdep complaints, thus allowing lockdep to continue to function for
  * real issues.
@@ -3016,7 +3017,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 static inline void printk_legacy_allow_spinlock_enter(void) { }
 static inline void printk_legacy_allow_spinlock_exit(void) { }
 #else
-static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
+static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
 
 static inline void printk_legacy_allow_spinlock_enter(void)
 {
-- 
2.25.1.362.g51ebf55

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

On 2025-10-23 12:32:34 [+0200], Oleg Nesterov wrote:
> printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.

Could we please get rid of CONFIG_PROVE_RAW_LOCK_NESTING here? This is
lockdep internal implementation and has nothing to do with printk or
anything. The nesting is documented in
	Documentation/locking/locktypes.rst

and this counts regardless of PREEMPT_RT. The only reason why this is
still around is that non-RT architectures tend to do it wrong (sparc)
and I didn't get around to fix it all. And people started to give me
lockdep splats and after some time I gave up and it is enforced only on
RT architectures.


  printk_legacy_map is using the wrong override type. LD_WAIT_SLEEP is for
  always sleeping lock types like mutex_t. LD_WAIT_CONFIG is for sleeping
  on PREEMPT_RT lock types like spinlock_t.

> However, LD_WAIT_SLEEP is not exactly right; it fools lockdep as if it
> 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 make it more clear as suggested by Petr.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/printk.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5aee9ffb16b9..80f0bedf5cb7 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3003,11 +3003,12 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  
>  /*
>   * Legacy console printing from printk() caller context does not respect
> - * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
> - * false positive. For PREEMPT_RT the false positive condition does not
> - * occur.
> + * raw_spinlock/spinlock nesting. However, on PREEMPT_RT the printing
> + * path from atomic context is always avoided and the console driver
> + * is always invoked from a dedicated thread. Thus the lockdep splat
> + * on !PREEMPT_RT is a false positive.

  The legacy console always acquires a spinlock_t from its printing
  callback. This violates lock nesting if the caller acquired a
  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
  spinlock_t is okay here.

> - * This map is used to temporarily establish LD_WAIT_SLEEP context for the
> + * This map is used to temporarily establish LD_WAIT_CONFIG context for the
>   * console write() callback when legacy printing to avoid false positive
>   * lockdep complaints, thus allowing lockdep to continue to function for
>   * real issues.
> @@ -3016,7 +3017,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  static inline void printk_legacy_allow_spinlock_enter(void) { }
>  static inline void printk_legacy_allow_spinlock_exit(void) { }
>  #else
> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);

We could use this lock_map_acquire_try() directly but okay having it in
one spot with a comment might have its benefit. But _why_ is here a
CONFIG_PREEMPT_RT? This is supposed to work in both cases. Should a
legacy driver be invoked on RT then the comment is not accurate, lockdep
won't yell but we still have CONFIG_DEBUG_ATOMIC_SLEEP making its own
judgement.

>  static inline void printk_legacy_allow_spinlock_enter(void)
>  {

Sebastian

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[John Ogness]

On 2025-10-23, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>> @@ -3016,7 +3017,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>>  static inline void printk_legacy_allow_spinlock_enter(void) { }
>>  static inline void printk_legacy_allow_spinlock_exit(void) { }
>>  #else
>> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
>> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
>
> We could use this lock_map_acquire_try() directly but okay having it in
> one spot with a comment might have its benefit. But _why_ is here a
> CONFIG_PREEMPT_RT? This is supposed to work in both cases. Should a
> legacy driver be invoked on RT then the comment is not accurate, lockdep
> won't yell but we still have CONFIG_DEBUG_ATOMIC_SLEEP making its own
> judgement.

With PREEMPT_RT, legacy console printing is forced into its own
dedicated kthread. At runtime this is checked via
force_legacy_kthread(). So with PREEMPT_RT there is no need for special
lockdep trickery. Once we can get all the consoles switched over to
nbcon, !PREEMPT_RT will also not need this lockdep trickery.

John

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

On 2025-10-23 17:12:49 [+0206], John Ogness wrote:
> On 2025-10-23, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >> @@ -3016,7 +3017,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> >>  static inline void printk_legacy_allow_spinlock_enter(void) { }
> >>  static inline void printk_legacy_allow_spinlock_exit(void) { }
> >>  #else
> >> -static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> >> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
> >
> > We could use this lock_map_acquire_try() directly but okay having it in
> > one spot with a comment might have its benefit. But _why_ is here a
> > CONFIG_PREEMPT_RT? This is supposed to work in both cases. Should a
> > legacy driver be invoked on RT then the comment is not accurate, lockdep
> > won't yell but we still have CONFIG_DEBUG_ATOMIC_SLEEP making its own
> > judgement.
> 
> With PREEMPT_RT, legacy console printing is forced into its own
> dedicated kthread. At runtime this is checked via
> force_legacy_kthread(). So with PREEMPT_RT there is no need for special
> lockdep trickery. Once we can get all the consoles switched over to
> nbcon, !PREEMPT_RT will also not need this lockdep trickery.

This does not matter. My point is that there no need for this ifdefery.
You say: This might get the nesting wrong but it is fine because the
outer lock should be this. This does not hurt if it is also applied on
RT. _BUT_ on RT you don't even trigger this path so this ifdef is not
needed.

> John

Sebastian

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[John Ogness]

On 2025-10-23, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>> With PREEMPT_RT, legacy console printing is forced into its own
>> dedicated kthread. At runtime this is checked via
>> force_legacy_kthread(). So with PREEMPT_RT there is no need for special
>> lockdep trickery. Once we can get all the consoles switched over to
>> nbcon, !PREEMPT_RT will also not need this lockdep trickery.
>
> This does not matter. My point is that there no need for this ifdefery.
> You say: This might get the nesting wrong but it is fine because the
> outer lock should be this. This does not hurt if it is also applied on
> RT. _BUT_ on RT you don't even trigger this path so this ifdef is not
> needed.

I am more concerned if we implement a bug and the outer lock is _not_
what it should be and RT _does_ call this path. IMHO avoiding
lockdep-forcing when it is not needed is more important than avoiding
the trivial PREEMPT_RT ifdefery. But I will not fight it.

John

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

Again, quite possibly I am wrong. please see my reply in another thread,
https://lore.kernel.org/all/20251023152942.GC26461@redhat.com/

On 10/23, Sebastian Andrzej Siewior wrote:
>
> This does not matter. My point is that there no need for this ifdefery.

I disagree. Rightly or not. To me this ifdefery in printk.c make the intent
more clear.

I am still thinking about the possible cleanup of the current usage of
DEFINE_WAIT_OVERRIDE_MAP(), but I think that, whatever we do, this cleanup
should take CONFIG_RT into account.

Oleg.

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

On 2025-10-23 17:46:58 [+0200], Oleg Nesterov wrote:
> Again, quite possibly I am wrong. please see my reply in another thread,
> https://lore.kernel.org/all/20251023152942.GC26461@redhat.com/
> 
> On 10/23, Sebastian Andrzej Siewior wrote:
> >
> > This does not matter. My point is that there no need for this ifdefery.
> 
> I disagree. Rightly or not. To me this ifdefery in printk.c make the intent
> more clear.
> 
> I am still thinking about the possible cleanup of the current usage of
> DEFINE_WAIT_OVERRIDE_MAP(), but I think that, whatever we do, this cleanup
> should take CONFIG_RT into account.

Right. Please just do s/SLEEP/CONFIG as discussed. Please leave
PROVE_RAW_LOCK_NESTING out of it while arguing why this change is
correct.

> Oleg.

Sebastian

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Petr Mladek]

On Thu 2025-10-23 21:14:42, Sebastian Andrzej Siewior wrote:
> On 2025-10-23 17:46:58 [+0200], Oleg Nesterov wrote:
> > Again, quite possibly I am wrong. please see my reply in another thread,
> > https://lore.kernel.org/all/20251023152942.GC26461@redhat.com/
> > 
> > On 10/23, Sebastian Andrzej Siewior wrote:
> > >
> > > This does not matter. My point is that there no need for this ifdefery.
> > 
> > I disagree. Rightly or not. To me this ifdefery in printk.c make the intent
> > more clear.
> > 
> > I am still thinking about the possible cleanup of the current usage of
> > DEFINE_WAIT_OVERRIDE_MAP(), but I think that, whatever we do, this cleanup
> > should take CONFIG_RT into account.
> 
> Right. Please just do s/SLEEP/CONFIG as discussed. Please leave
> PROVE_RAW_LOCK_NESTING out of it while arguing why this change is
> correct.

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.
 *
 * 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
the problematic code path should never be called in PREEMPT_RT mode.

However, LD_WAIT_SLEEP is not exactly right. It fools lockdep as if it
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>


Is this better and acceptable, please?
If not then please provide alternatives ;-)

Best Regards,
Petr

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

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.
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.


>  *
>  * 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? :) 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.

> 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.

> Best Regards,
> Petr

Sebastian

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

On 10/23, Sebastian Andrzej Siewior wrote:
>
> On 2025-10-23 12:32:34 [+0200], Oleg Nesterov wrote:
> > printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> > CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.
>
> Could we please get rid of CONFIG_PROVE_RAW_LOCK_NESTING here? This is
> lockdep internal implementation and has nothing to do with printk or
> anything.

OK, but let me ensure I didn't miss something (again ;).

I mentioned CONFIG_PROVE_RAW_LOCK_NESTING in the changelog because if
CONFIG_PROVE_RAW_LOCK_NESTING=n, then LD_WAIT_CONFIG == LD_WAIT_SPIN
and lockdep will not complain if spinlock_t nests inside raw_spinlock_t.

IOW, without CONFIG_PROVE_RAW_LOCK_NESTING printk_legacy_map is not
really needed.

Right?

Oleg.

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

On 2025-10-24 12:40:56 [+0200], Oleg Nesterov wrote:
> On 10/23, Sebastian Andrzej Siewior wrote:
> >
> > On 2025-10-23 12:32:34 [+0200], Oleg Nesterov wrote:
> > > printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> > > CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.
> >
> > Could we please get rid of CONFIG_PROVE_RAW_LOCK_NESTING here? This is
> > lockdep internal implementation and has nothing to do with printk or
> > anything.
> 
> OK, but let me ensure I didn't miss something (again ;).
> 
> I mentioned CONFIG_PROVE_RAW_LOCK_NESTING in the changelog because if
> CONFIG_PROVE_RAW_LOCK_NESTING=n, then LD_WAIT_CONFIG == LD_WAIT_SPIN
> and lockdep will not complain if spinlock_t nests inside raw_spinlock_t.
> 
> IOW, without CONFIG_PROVE_RAW_LOCK_NESTING printk_legacy_map is not
> really needed.

This is correct but CONFIG_PROVE_RAW_LOCK_NESTING is kind of a must and
should not be an option. This just verifies what we have in
	Documentation/locking/locktypes.rst
and this is not an option.
As I explained it earlier in this (or other) thread I removed it and
brought it back because on non-RT architectures spilled warnings and
people did not want to apply a larger pile of patches because "it did
work" and "I don't use RT" and I have so much time available for
arguing.

It is not an option architectures which support RT. It is one on
architectures which do not support RT so they don't have warning and may
debug & fix them if they want to.

> Oleg.

Sebastian

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

On 10/24, Sebastian Andrzej Siewior wrote:
>
> On 2025-10-24 12:40:56 [+0200], Oleg Nesterov wrote:
> > On 10/23, Sebastian Andrzej Siewior wrote:
> > >
> > > On 2025-10-23 12:32:34 [+0200], Oleg Nesterov wrote:
> > > > printk_legacy_map is used on !PREEMPT_RT to avoid false positives from
> > > > CONFIG_PROVE_RAW_LOCK_NESTING about raw_spinlock/spinlock nesting.
> > >
> > > Could we please get rid of CONFIG_PROVE_RAW_LOCK_NESTING here? This is
> > > lockdep internal implementation and has nothing to do with printk or
> > > anything.
> >
> > OK, but let me ensure I didn't miss something (again ;).
> >
> > I mentioned CONFIG_PROVE_RAW_LOCK_NESTING in the changelog because if
> > CONFIG_PROVE_RAW_LOCK_NESTING=n, then LD_WAIT_CONFIG == LD_WAIT_SPIN
> > and lockdep will not complain if spinlock_t nests inside raw_spinlock_t.
> >
> > IOW, without CONFIG_PROVE_RAW_LOCK_NESTING printk_legacy_map is not
> > really needed.
>
> This is correct but CONFIG_PROVE_RAW_LOCK_NESTING is kind of a must and
> should not be an option.

Yes, I see your point. Just wanted to ensure I fully understand it.

Thanks,

Oleg.

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Petr Mladek]

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

Re: [PATCH v2] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

So it seems that you guys finally have a consensus on this patch ;)

Should I send v3 or will you do it? I don't care about the "From:" tag.
If you want me to do it, let me ask:

On 10/24, Petr Mladek wrote:
>
> On Fri 2025-10-24 12:38:08, Sebastian Andrzej Siewior wrote:
> >   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.

Same here. Should I replace the whole comment block above
DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map) with the text above?

> >   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.

Same here. Should I add anything else into the changelog?

> JFYI, I do not mind which version is used.

Same here ;)

Oleg.

[PATCH v3] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/printk/printk.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5aee9ffb16b9..e9e65228f1ac 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3002,21 +3002,18 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 }
 
 /*
- * Legacy console printing from printk() caller context does not respect
- * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
- * false positive. For PREEMPT_RT the false positive condition does not
- * occur.
- *
- * This map is used to temporarily establish LD_WAIT_SLEEP context for the
- * console write() callback when legacy printing to avoid false positive
- * lockdep complaints, thus allowing lockdep to continue to function for
- * real issues.
- */
+ * 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.
+*/
 #ifdef CONFIG_PREEMPT_RT
 static inline void printk_legacy_allow_spinlock_enter(void) { }
 static inline void printk_legacy_allow_spinlock_exit(void) { }
 #else
-static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
+static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_CONFIG);
 
 static inline void printk_legacy_allow_spinlock_enter(void)
 {
-- 
2.25.1.362.g51ebf55

Re: [PATCH v3] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[Sebastian Andrzej Siewior]

On 2025-10-26 16:07:26 [+0100], Oleg Nesterov wrote:
> 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.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH v3] printk_legacy_map: use LD_WAIT_CONFIG instead of LD_WAIT_SLEEP

[John Ogness]

On 2025-10-26, Oleg Nesterov <oleg@redhat.com> wrote:
> 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.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: John Ogness <john.ogness@linutronix.de>