Re: [PATCH v2 06/10] x86/mce: Convert multiple if () statements into a switch() statement
From: Tony Luck
Date: Fri Oct 18 2024 - 16:15:10 EST
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