[PATCH 0/2] lockdep: disable_irq_nosync_lockdep.*() for PREEMPT_RT.

[PATCH 0/2] lockdep: disable_irq_nosync_lockdep.*() for PREEMPT_RT.

[Sebastian Andrzej Siewior]

Guenter reported a sleeping-while-atomic warning on PREEMPT_RT with the
NE2K card. Turns out it is a questionable construct which requires
lockdep tweak to not get reported. It was added long time ago to deal
with the NE2K and has today two users.

The way I see, we could convert NE2K to NAPI and avoid the construct. It
would do the slow-part/ spin (in the reset case) with "just" BH disabled
which is what it is doing now since all callers disable BH before
calling. There are however many parts using the driver which have to be
touched (CS, PCI, …) and I have just qemu here so I am not too
comfortable doing it. Also I haven't looked what is wrong forcedeth yet.

I have no idea why nobody converted NE2K to NAPI in the last 20 years.
The annotation was added in 2007 while NAPI was added in 2002 via
   abb85ec3ab0ba ("Integrate NAPI work done by Jamal Hadi Salim, Robert Olsson, and Alexey Kuznetsov.…)

The easy part is to just avoid disabling interrupt on PREEMPT_RT since
there is no difference.

Sebastian Andrzej Siewior (2):
  lockdep: Don't disable interrupts on RT in
    disable_irq_nosync_lockdep.*()
  lockdep: Remove disable_irq_lockdep().

 include/linux/interrupt.h | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

-- 
2.47.2

[PATCH 1/2] lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

[Sebastian Andrzej Siewior]

disable_irq_nosync_lockdep() disables interrupts with lockdep enabled to
avoid false positive reports by lockdep that a certain lock has not been
acquired with disabled interrupts. The user of this macros expects that
a lock can be acquried without disabling interrupts because the IRQ line
triggering the interrupt is disabled.

This triggers a warning on PREEMPT_RT because after
disable_irq_nosync_lockdep.*() the following spinlock_t now is acquired
with disabled interrupts.

On PREEMPT_RT there is no difference between spin_lock() and
spin_lock_irq() so avoiding disabling interrupts in this case works for
the two remaining callers as of today.

Don't disable interrupts on PREEMPT_RT in disable_irq_nosync_lockdep.*().

Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/760e34f9-6034-40e0-82a5-ee9becd24438@roeck-us.net
Fixes: e8106b941ceab ("[PATCH] lockdep: core, add enable/disable_irq_irqsave/irqrestore() APIs")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/interrupt.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 8cd9327e4e78d..a1b1be9bf73b2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -448,7 +448,7 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
 static inline void disable_irq_nosync_lockdep(unsigned int irq)
 {
 	disable_irq_nosync(irq);
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_disable();
 #endif
 }
@@ -456,7 +456,7 @@ static inline void disable_irq_nosync_lockdep(unsigned int irq)
 static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned long *flags)
 {
 	disable_irq_nosync(irq);
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_save(*flags);
 #endif
 }
@@ -471,7 +471,7 @@ static inline void disable_irq_lockdep(unsigned int irq)
 
 static inline void enable_irq_lockdep(unsigned int irq)
 {
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_enable();
 #endif
 	enable_irq(irq);
@@ -479,7 +479,7 @@ static inline void enable_irq_lockdep(unsigned int irq)
 
 static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long *flags)
 {
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_restore(*flags);
 #endif
 	enable_irq(irq);
-- 
2.47.2

Re: [PATCH 1/2] lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

[Guenter Roeck]

On Wed, Feb 12, 2025 at 11:36:18AM +0100, Sebastian Andrzej Siewior wrote:
> disable_irq_nosync_lockdep() disables interrupts with lockdep enabled to
> avoid false positive reports by lockdep that a certain lock has not been
> acquired with disabled interrupts. The user of this macros expects that
> a lock can be acquried without disabling interrupts because the IRQ line
> triggering the interrupt is disabled.
> 
> This triggers a warning on PREEMPT_RT because after
> disable_irq_nosync_lockdep.*() the following spinlock_t now is acquired
> with disabled interrupts.
> 
> On PREEMPT_RT there is no difference between spin_lock() and
> spin_lock_irq() so avoiding disabling interrupts in this case works for
> the two remaining callers as of today.
> 
> Don't disable interrupts on PREEMPT_RT in disable_irq_nosync_lockdep.*().
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/760e34f9-6034-40e0-82a5-ee9becd24438@roeck-us.net
> Fixes: e8106b941ceab ("[PATCH] lockdep: core, add enable/disable_irq_irqsave/irqrestore() APIs")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 1/2] lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

[Steven Rostedt]

On Wed, 12 Feb 2025 11:36:18 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> disable_irq_nosync_lockdep() disables interrupts with lockdep enabled to
> avoid false positive reports by lockdep that a certain lock has not been
> acquired with disabled interrupts. The user of this macros expects that
> a lock can be acquried without disabling interrupts because the IRQ line
> triggering the interrupt is disabled.
> 
> This triggers a warning on PREEMPT_RT because after
> disable_irq_nosync_lockdep.*() the following spinlock_t now is acquired
> with disabled interrupts.
> 
> On PREEMPT_RT there is no difference between spin_lock() and
> spin_lock_irq() so avoiding disabling interrupts in this case works for
> the two remaining callers as of today.
> 
> Don't disable interrupts on PREEMPT_RT in disable_irq_nosync_lockdep.*().
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/760e34f9-6034-40e0-82a5-ee9becd24438@roeck-us.net
> Fixes: e8106b941ceab ("[PATCH] lockdep: core, add enable/disable_irq_irqsave/irqrestore() APIs")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/interrupt.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 8cd9327e4e78d..a1b1be9bf73b2 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -448,7 +448,7 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  static inline void disable_irq_nosync_lockdep(unsigned int irq)
>  {
>  	disable_irq_nosync(irq);
> -#ifdef CONFIG_LOCKDEP
> +#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)

Hmm, should you also add a:

Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>

?

  https://lore.kernel.org/all/20250211182113.4875751a@gandalf.local.home/

    ;-)

-- Steve

>  	local_irq_disable();
>  #endif
>  }

Re: [PATCH 1/2] lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

[Boqun Feng]

On Wed, Feb 12, 2025 at 11:36:18AM +0100, Sebastian Andrzej Siewior wrote:
> disable_irq_nosync_lockdep() disables interrupts with lockdep enabled to
> avoid false positive reports by lockdep that a certain lock has not been
> acquired with disabled interrupts. The user of this macros expects that
> a lock can be acquried without disabling interrupts because the IRQ line
> triggering the interrupt is disabled.
> 
> This triggers a warning on PREEMPT_RT because after
> disable_irq_nosync_lockdep.*() the following spinlock_t now is acquired
> with disabled interrupts.
> 
> On PREEMPT_RT there is no difference between spin_lock() and
> spin_lock_irq() so avoiding disabling interrupts in this case works for
> the two remaining callers as of today.
> 
> Don't disable interrupts on PREEMPT_RT in disable_irq_nosync_lockdep.*().
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/760e34f9-6034-40e0-82a5-ee9becd24438@roeck-us.net
> Fixes: e8106b941ceab ("[PATCH] lockdep: core, add enable/disable_irq_irqsave/irqrestore() APIs")

Is this "Fixes" tag accurate? IIUC, the issue only exists after
CONFIG_PREEMPT_RT is introduced from mainline POV. Or we want this to be
backported in older stable kernels predating CONFIG_PREEMPT_RT?

Regards,
Boqun

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/interrupt.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 8cd9327e4e78d..a1b1be9bf73b2 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -448,7 +448,7 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  static inline void disable_irq_nosync_lockdep(unsigned int irq)
>  {
>  	disable_irq_nosync(irq);
> -#ifdef CONFIG_LOCKDEP
> +#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
>  	local_irq_disable();
>  #endif
>  }
> @@ -456,7 +456,7 @@ static inline void disable_irq_nosync_lockdep(unsigned int irq)
>  static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned long *flags)
>  {
>  	disable_irq_nosync(irq);
> -#ifdef CONFIG_LOCKDEP
> +#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
>  	local_irq_save(*flags);
>  #endif
>  }
> @@ -471,7 +471,7 @@ static inline void disable_irq_lockdep(unsigned int irq)
>  
>  static inline void enable_irq_lockdep(unsigned int irq)
>  {
> -#ifdef CONFIG_LOCKDEP
> +#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
>  	local_irq_enable();
>  #endif
>  	enable_irq(irq);
> @@ -479,7 +479,7 @@ static inline void enable_irq_lockdep(unsigned int irq)
>  
>  static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long *flags)
>  {
> -#ifdef CONFIG_LOCKDEP
> +#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
>  	local_irq_restore(*flags);
>  #endif
>  	enable_irq(irq);
> -- 
> 2.47.2
> 

[tip: locking/core] lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

[tip-bot2 for Sebastian Andrzej Siewior]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     87886b32d669abc11c7be95ef44099215e4f5788
Gitweb:        https://git.kernel.org/tip/87886b32d669abc11c7be95ef44099215e4f5788
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Wed, 12 Feb 2025 11:36:18 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 14 Mar 2025 21:13:20 +01:00

lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

disable_irq_nosync_lockdep() disables interrupts with lockdep enabled to
avoid false positive reports by lockdep that a certain lock has not been
acquired with disabled interrupts. The user of this macros expects that
a lock can be acquried without disabling interrupts because the IRQ line
triggering the interrupt is disabled.

This triggers a warning on PREEMPT_RT because after
disable_irq_nosync_lockdep.*() the following spinlock_t now is acquired
with disabled interrupts.

On PREEMPT_RT there is no difference between spin_lock() and
spin_lock_irq() so avoiding disabling interrupts in this case works for
the two remaining callers as of today.

Don't disable interrupts on PREEMPT_RT in disable_irq_nosync_lockdep.*().

Closes: https://lore.kernel.org/760e34f9-6034-40e0-82a5-ee9becd24438@roeck-us.net
Fixes: e8106b941ceab ("[PATCH] lockdep: core, add enable/disable_irq_irqsave/irqrestore() APIs")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Suggested-by: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20250212103619.2560503-2-bigeasy@linutronix.de
---
 include/linux/interrupt.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 8cd9327..a1b1be9 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -448,7 +448,7 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
 static inline void disable_irq_nosync_lockdep(unsigned int irq)
 {
 	disable_irq_nosync(irq);
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_disable();
 #endif
 }
@@ -456,7 +456,7 @@ static inline void disable_irq_nosync_lockdep(unsigned int irq)
 static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned long *flags)
 {
 	disable_irq_nosync(irq);
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_save(*flags);
 #endif
 }
@@ -471,7 +471,7 @@ static inline void disable_irq_lockdep(unsigned int irq)
 
 static inline void enable_irq_lockdep(unsigned int irq)
 {
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_enable();
 #endif
 	enable_irq(irq);
@@ -479,7 +479,7 @@ static inline void enable_irq_lockdep(unsigned int irq)
 
 static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long *flags)
 {
-#ifdef CONFIG_LOCKDEP
+#if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
 	local_irq_restore(*flags);
 #endif
 	enable_irq(irq);

[PATCH 2/2] lockdep: Remove disable_irq_lockdep().

[Sebastian Andrzej Siewior]

disable_irq_lockdep() has no users, last one was probabaly removed in
   0b7c874348ea1 ("forcedeth: fix unilateral interrupt disabling in netpoll path")

Remove disable_irq_lockdep().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/interrupt.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a1b1be9bf73b2..c782a74d2a304 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -461,14 +461,6 @@ static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned
 #endif
 }
 
-static inline void disable_irq_lockdep(unsigned int irq)
-{
-	disable_irq(irq);
-#ifdef CONFIG_LOCKDEP
-	local_irq_disable();
-#endif
-}
-
 static inline void enable_irq_lockdep(unsigned int irq)
 {
 #if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)
-- 
2.47.2

Re: [PATCH 2/2] lockdep: Remove disable_irq_lockdep().

[Guenter Roeck]

On Wed, Feb 12, 2025 at 11:36:19AM +0100, Sebastian Andrzej Siewior wrote:
> disable_irq_lockdep() has no users, last one was probabaly removed in
>    0b7c874348ea1 ("forcedeth: fix unilateral interrupt disabling in netpoll path")
> 
> Remove disable_irq_lockdep().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[tip: locking/core] lockdep: Remove disable_irq_lockdep()

[tip-bot2 for Sebastian Andrzej Siewior]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     35e6b537af85d97e0aafd8f2829dfa884a22df20
Gitweb:        https://git.kernel.org/tip/35e6b537af85d97e0aafd8f2829dfa884a22df20
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Wed, 12 Feb 2025 11:36:19 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 14 Mar 2025 21:13:20 +01:00

lockdep: Remove disable_irq_lockdep()

disable_irq_lockdep() has no users, last one was probabaly removed in
   0b7c874348ea1 ("forcedeth: fix unilateral interrupt disabling in netpoll path")

Remove disable_irq_lockdep().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20250212103619.2560503-3-bigeasy@linutronix.de
---
 include/linux/interrupt.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a1b1be9..c782a74 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -461,14 +461,6 @@ static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned
 #endif
 }
 
-static inline void disable_irq_lockdep(unsigned int irq)
-{
-	disable_irq(irq);
-#ifdef CONFIG_LOCKDEP
-	local_irq_disable();
-#endif
-}
-
 static inline void enable_irq_lockdep(unsigned int irq)
 {
 #if defined(CONFIG_LOCKDEP) && !defined(CONFIG_PREEMPT_RT)

Re: [PATCH 0/2] lockdep: disable_irq_nosync_lockdep.*() for PREEMPT_RT.

[Sebastian Andrzej Siewior]

On 2025-02-12 11:36:17 [+0100], To linux-rt-devel@lists.linux.dev wrote:
> Guenter reported a sleeping-while-atomic warning on PREEMPT_RT with the
> NE2K card. Turns out it is a questionable construct which requires
> lockdep tweak to not get reported. It was added long time ago to deal
> with the NE2K and has today two users.

ping.

Sebastian