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

[Luck, Tony]

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