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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY8PR11MB7134B4D87A55C6680D5CB0BF89412@CY8PR11MB7134.namprd11.prod.outlook.com>
Date: Sat, 19 Oct 2024 04:39:26 +0000
From: "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>, "Mehta, Sohil" <sohil.mehta@...el.com>
CC: "bp@...en8.de" <bp@...en8.de>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"mingo@...hat.com" <mingo@...hat.com>, "hpa@...or.com" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>, "linux-edac@...r.kernel.org"
	<linux-edac@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 06/10] x86/mce: Convert multiple if () statements into
 a switch() statement

> From: Luck, Tony <tony.luck@...el.com>
> [...]
> 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?
> 

LGTM.
This makes the code more structured and readable. 
I'll replace mine with this new patch in my next version after basic testing.
Thanks, Tony & Sohil.

-Qiuxu