Re: [PATCH v5 0/3] x86/mce: Fix timer list corruption and avoid redundant polling

[Aaron Tomlin]

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