Hi Aaron,

> Sashiko [1] has correctly highlighted additional pre-existing race
> conditions in this area. Should you prefer, I would be more than happy to
> incorporate the fixes for these into the current series?
>
> [1]: https://sashiko.dev/#/patchset/20260903194130.186096-1-atomlin%40atomlin.com

I'm not sure about the first Sashiko issue ... there is a wrong statement:
 
   Since the hardware threshold for a stormy bank is set to
   CMCI_STORM_THRESHOLD and no longer generates interrupts,

Setting the storm threshold doesn't disable interrupts. It just prevents
generation of a new interrupt from a bank until enough errors are logged
to meet the threshold. So the user won't see logs for a while. But should
another storm occur, then things will fix themselves without a reboot.

If you see an elegant solution to this race, then go ahead with a patch. But
I wouldn't stress if this one isn't fixed.

The second report regarding firmware first banks does look easy to solve.
Just change cmci_skip_banks() to clear the bit in mce_poll_banks?

-Tony

On Thu, Sep 03, 2026 at 10:02:56PM +0000, Luck, Tony wrote:
> Hi Aaron,
> 
> > Sashiko [1] has correctly highlighted additional pre-existing race
> > conditions in this area. Should you prefer, I would be more than happy to
> > incorporate the fixes for these into the current series?
> >
> > [1]: https://sashiko.dev/#/patchset/20260903194130.186096-1-atomlin%40atomlin.com
> 
> I'm not sure about the first Sashiko issue ... there is a wrong statement:
>  
>    Since the hardware threshold for a stormy bank is set to
>    CMCI_STORM_THRESHOLD and no longer generates interrupts,
> 
> Setting the storm threshold doesn't disable interrupts. It just prevents
> generation of a new interrupt from a bank until enough errors are logged
> to meet the threshold. So the user won't see logs for a while. But should
> another storm occur, then things will fix themselves without a reboot.
> 
> If you see an elegant solution to this race, then go ahead with a patch. But
> I wouldn't stress if this one isn't fixed.
> 
> The second report regarding firmware first banks does look easy to solve.
> Just change cmci_skip_banks() to clear the bit in mce_poll_banks?
> 
> -Tony

Hi Tony,

Yes, you are entirely right regarding the first report. Sashiko's assertion
that error telemetry is permanently lost until reboot is incorrect.
In arch/x86/kernel/cpu/mce/intel.c, I see:

        #define CMCI_STORM_THRESHOLD    32749

Setting the hardware threshold to 32749 merely defers further interrupts
until that count is reached; once enough errors accumulate, hardware
triggers a CMCI interrupt, and cmci_storm_begin() restores active storm
polling.

However, the preemption window I believe is real. Because cmci_storm_end()
runs in timer softirq context (via mce_timer_fn()) with local interrupts
enabled:

        void cmci_storm_end(unsigned int bank)
        {
            ...

            /* If no banks left in storm mode, stop polling. */
            if (!--storm->stormy_bank_count)
                mce_timer_kick(false);
        }

If a CMCI hardirq preempts the CPU after stormy_bank_count is decremented
to zero, but before mce_timer_kick(false) is called, the hardirq's
invocation of cmci_storm_begin() will increment stormy_bank_count to 1 and
call mce_timer_kick(true). When the softirq resumes, its delayed
mce_timer_kick(false) will erroneously override storm mode, leaving the CPU
with stormy_bank_count == 1 while the timer reverts to the 5-minute
interval.

An elegant solution is to protect the storm transitions in both
cmci_storm_begin() and cmci_storm_end() using local_irq_save() and
local_irq_restore(). This serialises the counter updates and timer kicks
against local hardirq preemption:

--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -85,29 +85,37 @@ static void mce_handle_storm(unsigned int bank, bool on)
 void cmci_storm_begin(unsigned int bank)
 {
     struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
+    unsigned long flags;

+    local_irq_save(flags);
     set_bit(bank, this_cpu_ptr(mce_poll_banks));
     storm->banks[bank].in_storm_mode = true;

     /*
      * If this is the first bank on this CPU to enter storm mode
      * start polling.
      */
     if (++storm->stormy_bank_count == 1)
         mce_timer_kick(true);
+    local_irq_restore(flags);
 }

 void cmci_storm_end(unsigned int bank)
 {
     struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
+    unsigned long flags;

+    local_irq_save(flags);
     if (!mce_flags.amd_threshold)
         clear_bit(bank, this_cpu_ptr(mce_poll_banks));
     storm->banks[bank].history = 0;
     storm->banks[bank].in_storm_mode = false;

     /* If no banks left in storm mode, stop polling. */
     if (!--storm->stormy_bank_count)
         mce_timer_kick(false);
+    local_irq_restore(flags);
 }

Now, regarding the second report, Sashiko appears to be correct. During
early boot, acpi_hest_init() -> mce_disable_bank() broadcasts via
on_each_cpu() to clear Firmware First banks from mce_poll_banks, but CPUs
that are brought online late or physically hotplugged miss this broadcast.

When those CPUs come online, cmci_skip_bank() currently bails out early
without clearing mce_poll_banks:

        /* Skip banks in firmware first mode */
        if (test_bit(bank, mce_banks_ce_disabled))
            return true;

Because mce_poll_banks is statically initialised to ~0UL, the bit remains
set, defeating bitmap_empty() on hotplugged CPUs and causing mce_timer_fn()
to periodically poll and clear Firmware First status registers.

Clearing the bit in cmci_skip_bank() resolves this cleanly:

--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -181,8 +181,10 @@ static bool cmci_skip_bank(int bank, u64 *val)
     if (test_bit(bank, owned))
         return true;

     /* Skip banks in firmware first mode */
-    if (test_bit(bank, mce_banks_ce_disabled))
+    if (test_bit(bank, mce_banks_ce_disabled)) {
+        clear_bit(bank, this_cpu_ptr(mce_poll_banks));
         return true;
+    }

     rdmsrq(MSR_IA32_MCx_CTL2(bank), *val);

If you are happy with these two changes, I will fold the local_irq_save()
fix into Patch 2/3 and the cmci_skip_bank() fix into Patch 3/3 for v6.


Kind regards,
-- 
Aaron Tomlin

On Fri, Sep 04, 2026 at 09:54:18AM -0400, Aaron Tomlin wrote:
> An elegant solution is to protect the storm transitions in both
> cmci_storm_begin() and cmci_storm_end() using local_irq_save() and
> local_irq_restore(). This serialises the counter updates and timer kicks
> against local hardirq preemption:
> 
> --- a/arch/x86/kernel/cpu/mce/threshold.c
> +++ b/arch/x86/kernel/cpu/mce/threshold.c
> @@ -85,29 +85,37 @@ static void mce_handle_storm(unsigned int bank, bool on)
>  void cmci_storm_begin(unsigned int bank)
>  {
>      struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +    unsigned long flags;
> 
> +    local_irq_save(flags);
>      set_bit(bank, this_cpu_ptr(mce_poll_banks));
>      storm->banks[bank].in_storm_mode = true;
> 
>      /*
>       * If this is the first bank on this CPU to enter storm mode
>       * start polling.
>       */
>      if (++storm->stormy_bank_count == 1)
>          mce_timer_kick(true);
> +    local_irq_restore(flags);
>  }
> 
>  void cmci_storm_end(unsigned int bank)
>  {
>      struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +    unsigned long flags;
> 
> +    local_irq_save(flags);
>      if (!mce_flags.amd_threshold)
>          clear_bit(bank, this_cpu_ptr(mce_poll_banks));
>      storm->banks[bank].history = 0;
>      storm->banks[bank].in_storm_mode = false;
> 
>      /* If no banks left in storm mode, stop polling. */
>      if (!--storm->stormy_bank_count)
>          mce_timer_kick(false);
> +    local_irq_restore(flags);
>  }

I ran this past an internal AI, and it said there was still a race in
mce_track_storm(). This test:

	if (storm->banks[mce->bank].in_storm_mode) {

is made with interrupts enabled, so another CMCI immediately after
picking which of the if/else paths to take could change the value of
in_storm_mode which then leads to corruption of the storm state machine.

> Now, regarding the second report, Sashiko appears to be correct. During
> early boot, acpi_hest_init() -> mce_disable_bank() broadcasts via
> on_each_cpu() to clear Firmware First banks from mce_poll_banks, but CPUs
> that are brought online late or physically hotplugged miss this broadcast.
> 
> When those CPUs come online, cmci_skip_bank() currently bails out early
> without clearing mce_poll_banks:
> 
>         /* Skip banks in firmware first mode */
>         if (test_bit(bank, mce_banks_ce_disabled))
>             return true;
> 
> Because mce_poll_banks is statically initialised to ~0UL, the bit remains
> set, defeating bitmap_empty() on hotplugged CPUs and causing mce_timer_fn()
> to periodically poll and clear Firmware First status registers.
> 
> Clearing the bit in cmci_skip_bank() resolves this cleanly:
> 
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -181,8 +181,10 @@ static bool cmci_skip_bank(int bank, u64 *val)
>      if (test_bit(bank, owned))
>          return true;
> 
>      /* Skip banks in firmware first mode */
> -    if (test_bit(bank, mce_banks_ce_disabled))
> +    if (test_bit(bank, mce_banks_ce_disabled)) {
> +        clear_bit(bank, this_cpu_ptr(mce_poll_banks));
>          return true;
> +    }

This looks right.

>      rdmsrq(MSR_IA32_MCx_CTL2(bank), *val);
> 
> If you are happy with these two changes, I will fold the local_irq_save()
> fix into Patch 2/3 and the cmci_skip_bank() fix into Patch 3/3 for v6.
> 

I don't think these fixes should be folded into existing patches in this
series. They are distinct changes fixing specific long standing issues.
They deserve their own patches under the "one change per patch" doctrine.

Also we still have:

static void __mce_disable_bank(void *arg)
{
        int bank = *((int *)arg);
        __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
        cmci_disable_bank(bank);
}

That should switch over to the atomic clear_bank() or there should
be a comment on why non-atomic is OK here and bad everywhere else.

-Tony

On Fri, Sep 04, 2026 at 02:51:42PM -0700, Luck, Tony wrote:
> On Fri, Sep 04, 2026 at 09:54:18AM -0400, Aaron Tomlin wrote:
> > An elegant solution is to protect the storm transitions in both
> > cmci_storm_begin() and cmci_storm_end() using local_irq_save() and
> > local_irq_restore(). This serialises the counter updates and timer kicks
> > against local hardirq preemption:
> > 
> > --- a/arch/x86/kernel/cpu/mce/threshold.c
> > +++ b/arch/x86/kernel/cpu/mce/threshold.c
> > @@ -85,29 +85,37 @@ static void mce_handle_storm(unsigned int bank, bool on)
> >  void cmci_storm_begin(unsigned int bank)
> >  {
> >      struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> > +    unsigned long flags;
> > 
> > +    local_irq_save(flags);
> >      set_bit(bank, this_cpu_ptr(mce_poll_banks));
> >      storm->banks[bank].in_storm_mode = true;
> > 
> >      /*
> >       * If this is the first bank on this CPU to enter storm mode
> >       * start polling.
> >       */
> >      if (++storm->stormy_bank_count == 1)
> >          mce_timer_kick(true);
> > +    local_irq_restore(flags);
> >  }
> > 
> >  void cmci_storm_end(unsigned int bank)
> >  {
> >      struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> > +    unsigned long flags;
> > 
> > +    local_irq_save(flags);
> >      if (!mce_flags.amd_threshold)
> >          clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> >      storm->banks[bank].history = 0;
> >      storm->banks[bank].in_storm_mode = false;
> > 
> >      /* If no banks left in storm mode, stop polling. */
> >      if (!--storm->stormy_bank_count)
> >          mce_timer_kick(false);
> > +    local_irq_restore(flags);
> >  }
> 
> I ran this past an internal AI, and it said there was still a race in
> mce_track_storm(). This test:
> 
> 	if (storm->banks[mce->bank].in_storm_mode) {
> 
> is made with interrupts enabled, so another CMCI immediately after
> picking which of the if/else paths to take could change the value of
> in_storm_mode which then leads to corruption of the storm state machine.
> 
> > Now, regarding the second report, Sashiko appears to be correct. During
> > early boot, acpi_hest_init() -> mce_disable_bank() broadcasts via
> > on_each_cpu() to clear Firmware First banks from mce_poll_banks, but CPUs
> > that are brought online late or physically hotplugged miss this broadcast.
> > 
> > When those CPUs come online, cmci_skip_bank() currently bails out early
> > without clearing mce_poll_banks:
> > 
> >         /* Skip banks in firmware first mode */
> >         if (test_bit(bank, mce_banks_ce_disabled))
> >             return true;
> > 
> > Because mce_poll_banks is statically initialised to ~0UL, the bit remains
> > set, defeating bitmap_empty() on hotplugged CPUs and causing mce_timer_fn()
> > to periodically poll and clear Firmware First status registers.
> > 
> > Clearing the bit in cmci_skip_bank() resolves this cleanly:
> > 
> > --- a/arch/x86/kernel/cpu/mce/intel.c
> > +++ b/arch/x86/kernel/cpu/mce/intel.c
> > @@ -181,8 +181,10 @@ static bool cmci_skip_bank(int bank, u64 *val)
> >      if (test_bit(bank, owned))
> >          return true;
> > 
> >      /* Skip banks in firmware first mode */
> > -    if (test_bit(bank, mce_banks_ce_disabled))
> > +    if (test_bit(bank, mce_banks_ce_disabled)) {
> > +        clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> >          return true;
> > +    }
> 
> This looks right.
> 
> >      rdmsrq(MSR_IA32_MCx_CTL2(bank), *val);
> > 
> > If you are happy with these two changes, I will fold the local_irq_save()
> > fix into Patch 2/3 and the cmci_skip_bank() fix into Patch 3/3 for v6.
> > 
> 
> I don't think these fixes should be folded into existing patches in this
> series. They are distinct changes fixing specific long standing issues.
> They deserve their own patches under the "one change per patch" doctrine.
> 
> Also we still have:
> 
> static void __mce_disable_bank(void *arg)
> {
>         int bank = *((int *)arg);
>         __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
>         cmci_disable_bank(bank);
> }
> 
> That should switch over to the atomic clear_bank() or there should
> be a comment on why non-atomic is OK here and bad everywhere else.
> 
> -Tony

Hi Tony,

Thank you and apologies for the delay.

Yes, you are correct about mce_track_storm(). Since mce_track_storm() can
be called from timer softirq context with interrupts enabled, a CMCI
hardirq on the same CPU can re-entrantly execute mce_track_storm(), racing
on in_storm_mode and corrupting the bank's history and storm state.

Rather than only protecting the helper functions cmci_storm_begin() and
cmci_storm_end(), the proper solution is to enclose mce_track_storm() with
local_irq_save() and local_irq_restore(). This serialises the entire storm
state evaluation, counter transitions, and timer kicks against hardirq
preemption.

I also agree with the "one change per patch" doctrine. As such:
    1.  I will split the firmware-first cmci_skip_bank() fix into its own
        separate patch.

    2.  I will convert __mce_disable_bank() to atomic clear_bit() as part
        of the atomic bitops/concurrency fix so that mce_poll_banks updates
        are uniformly atomic throughout the subsystem.

    3.  The final patch will remain solely focused on the core polling
        optimisation (non-CMCI banks, bitmap_empty, and isolation checks).

I will prepare and send version 6 structured as a 4-patch series
accordingly.

-- 
Aaron Tomlin