Re: [PATCH v2 06/10] x86/mce: Convert multiple if () statements into a switch() statement

[Tony Luck]

On Fri, Oct 18, 2024 at 12:44:00PM -0700, Sohil Mehta wrote:
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 725c1d6fb1e5..40672fe0991a 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > @@ -1892,7 +1892,8 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> >  	}
> >  
> >  	/* This should be disabled by the BIOS, but isn't always */
> 
> This comment is specific to the AMD and placing it before the switch
> makes it seem generic to the entire switch statement. It should probably
> be moved inside the AMD case just above the disable GART TLB check.
> 
> > -	if (c->x86_vendor == X86_VENDOR_AMD) {
> > +	switch (c->x86_vendor) {
> > +	case X86_VENDOR_AMD:
> >  		if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> >  			/*
> >  			 * disable GART TBL walk error reporting, which
> > @@ -1925,9 +1926,9 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> >  		if (c->x86 >= 0x17 && c->x86 <= 0x1A)
> >  			mce_flags.zen_ifu_quirk = 1;
> >  
> > -	}
> > +		break;
> >  
> 
> 
> Also, why not include the unknown vendor check (right above) inside the
> switch case as well?
> 
> if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
> 	pr_info("unknown CPU type - not enabling MCE support\n");
> 	return -EOPNOTSUPP;
> }
> 
> This seems to follow the same pattern as others and can be the first
> case inside the switch.

The vendor specific bits are large enough to warrant their own
static functions (as we do elsewhere in this file).

How about this (only compile-tested) patch?

-Tony