Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}()

[Boqun Feng <boqun@kernel.org>]

On Tue, Sep 01, 2026 at 03:43:47PM +0200, Thomas Gleixner wrote:
> > On Mon, Aug 31, 2026 at 12:02:50PM +0200, Thomas Gleixner wrote:
> > Right, that's why I thought fully revert on commit e901c1510e24 might
> > not be needed.
> 
> It's gone already and as I told you before the reason is that the issues
> were not restricted to the ordering parts v.s. count/hardware and the
> fallout in the softirq code. The whole issue with nested unlock/lock
> inside a guard are not solved by reordering local_interrupt_disable().
> Not to talk about the lack of proper debug features for it.
> 
> > And if PREEMPT_COUNT_IRQFLAGS=y is the future (i.e. it'll be always y),
> > then we will likely have local_interrupt_{en,dis}able() (or a different
> > name) as a general API for everyone. So the API (and its semantics) is
> > not Rust-specific considering the future direction. Hence previously I
> > said that we can move them to Rust only but seems a bit unnecessary to
> > me.
> 
> No. We need a proper strategy to pull that off and not exposing the
> functionality and the name right now outside of Rust makes that way
> simpler. Changing Rust is one thing, chasing down a pile of random use
> cases which crept in _before_ the design and strategy is settled is a
> completely different story.
> 

Fair enough.

Not trying to keep interrupt_{en,dis}able() from moving, but after some
thoughts, I think I figured out a few debugs we can add for
PREEMPT_COUNT_IRQFLAGS=n case, things we want to avoid:

*	interrupt_disable(); irq_disable(); irq_enable(); interrupt_enable();

*	interrupt_disable(); irq_enable(); irq_disable(); interrupt_enable();

*	irq_save(flags); interrupt_disable(); irq_restore(flags); interrupt_enable();

on top of your current work, we can do the following when
PREEMPT_COUNT_IRQFLAGS=n. It'll help find a few random use cases that
break. (even when interrupt_disable() are Rust-only, Rust code can still
call a function which does irq_enable(); irq_disable(); while in a
interrupt_disable() critical section, so it makes sense to catch them).

Thoughts?

Regards,
Boqun

---------------------->8
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index dd55786768d1..a41d188dceef 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -241,6 +241,14 @@ static __always_inline void raw_safe_halt(void)
 
 static __always_inline void raw_local_irq_disable(void)
 {
+	/*
+	 * Assuming local_irq_{en,dis}able() always paired, then
+	 * local_irq_disable() should not be used inside an
+	 * local_interrupt_disable() critical section. Because the paired
+	 * local_irq_enable() would enable the interrupt inside a
+	 * local_interrupt_disable() critical section.
+	 */
+	debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK));
 	arch_local_irq_disable();
 }
 
@@ -251,6 +259,12 @@ static __always_inline void raw_force_local_irq_disable(void)
 
 static __always_inline void raw_local_irq_enable(void)
 {
+	/*
+	 * local_irq_enable() should not be called inside a
+	 * local_interrupt_disable() critical section, but it would enable the
+	 * interrupt unexpectedly.
+	 */
+	debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK));
 	arch_local_irq_enable();
 }
 
@@ -261,6 +275,12 @@ static __always_inline unsigned long __raw_local_irq_save(void)
 
 static __always_inline void __raw_local_irq_restore(unsigned long flags)
 {
+	/*
+	 * local_irq_restore() should not enable interrupt unexpectedly inside
+	 * a local_interrupt_disable() critical section
+	 */
+	debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK) ||
+		     arch_irqs_disabled_flags(flags));
 	arch_local_irq_restore(flags);
 }