Re: [PATCH v3 2/2] x86/mce: Reset MCA_SYND1/2 and kflags between bank scans

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Mon, Sep 21, 2026 at 01:38:12PM +0000, OptoCloud wrote:
> From: Eirik Bøe <git@optocloud.no>
> 
> machine_check_poll() and __mc_scan_banks() reuse a single
> struct mce_hw_err across the whole bank scan. The record is zeroed
> once before the loop. Each iteration then resets only MISC, ADDR and
> SYND. Three more fields are written conditionally inside the loop and
> never cleared again, so a later bank inherits them.
> 
> mce_read_aux() writes err->vendor.amd.synd1/synd2 only on SMCA, and
> then only when MCI_STATUS_SYNDV is set, so a bank without SYNDV is
> printed with the supplemental syndromes of an earlier bank in the
> same scan.
> 
> A stale m->kflags changes what happens to the next bank:
> 
>   - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
>     error was taken from MCA_DESTAT rather than MCA_STATUS. A later
>     bank in the same poll then reads MCx_DEADDR instead of MCA_ADDR
>     if it has ADDRV, and amd_clear_bank() returns before writing 0 to
>     MCA_STATUS, so MCI_STATUS_VAL stays set and the bank is logged a
>     second time on the next poll.
> 
>   - mce_default_notifier() prints a record only if m->kflags is empty
>     or print_all is set, and the record reaches the gen pool as it
>     stands. A later bank that inherits MCE_CHECK_DFR_REGS is logged
>     with a non-zero m->kflags and is not printed by the notifier
>     chain.
> 
> Factor the per-bank clearing into mce_clear_hw_err_fields() and call
> it from both loops.
> 
> Found by code inspection; not reproduced on hardware.
> 
> Fixes: d4fca1358ea9 ("x86/MCE/AMD: Add support for new MCA_SYND{1,2} registers")
> Fixes: 7cb735d7c0cb ("x86/mce: Unify AMD DFR handler with MCA Polling")
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Eirik Bøe <git@optocloud.no>
> ---
> 
> Notes (amlog):
>     Changes since v2:
>     - Squashed the MCA_SYND1/2 reset and the ->kflags reset into one
>       patch, with the ->kflags reset in mce_clear_hw_err_fields() (Yazen)
>     - m->kflags is now cleared in __mc_scan_banks() as well, which it was
>       not in v2
>     - Subject prefix x86/mce/amd: -> x86/mce:
>     
>     Changes since v1:
>     - Dropped Cc: stable (Yazen)
>     - Added mce_clear_hw_err_fields() so the resets are not repeated in
>       machine_check_poll() and __mc_scan_banks() (Yazen)
>     - Reset the whole m->kflags field instead of just MCE_CHECK_DFR_REGS
>       (Yazen)
> 
>  arch/x86/kernel/cpu/mce/core.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 16183fa4ddc7..e906c3f8e888 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -653,6 +653,22 @@ static struct notifier_block mce_default_nb = {
>  	.priority	= MCE_PRIO_LOWEST,
>  };
>  
> +/*
> + * These fields are only filled in conditionally, so clear them before each
> + * bank to stop a bank inheriting the previous bank's values.
> + */
> +static noinstr void mce_clear_hw_err_fields(struct mce_hw_err *err)
> +{
> +	struct mce *m = &err->m;
> +
> +	m->misc = 0;
> +	m->addr = 0;
> +	m->synd = 0;
> +	m->kflags = 0;
> +	err->vendor.amd.synd1 = 0;
> +	err->vendor.amd.synd2 = 0;

It seems that the entire union can be reset in one step:
	err->vendor = (union vendor_info){ };

This would be more future-proof if/when new fields or structs are added
to the union.

Also, please align the lines on the '='.

Thanks,
Yazen