Messages in this thread | | | Date | Wed, 18 Jan 2023 18:31:10 +0000 | From | Yazen Ghannam <> | Subject | Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA |
| |
On Tue, Jan 17, 2023 at 10:23:09AM +0100, Ingo Molnar wrote: > > * Avadhut Naik <avadnaik@amd.com> wrote: > > > @@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = { > > { SMCA_CS, HWID_MCATYPE(0x2E, 0x0) }, > > { SMCA_PIE, HWID_MCATYPE(0x2E, 0x1) }, > > { SMCA_CS_V2, HWID_MCATYPE(0x2E, 0x2) }, > > + /* Software defined SMCA bank type to handle erratum 1384*/ > > + { SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1) }, > > > > /* Unified Memory Controller MCA type */ > > { SMCA_UMC, HWID_MCATYPE(0x96, 0x0) }, > > @@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype) > > > > if (c->x86 == 0x19) { > > switch (c->x86_model) { > > + /* > > + * Per Genoa's revision guide, erratum 1384, some SMCA Extended > > + * Error Codes and SMCA Control bits are incorrect for SMCA CS > > + * bank type. > > + */ > > + case 0x10 ... 0x1F: > > + case 0x60 ... 0x7B: > > + case 0xA0 ... 0xAF: > > + if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2)) > > + *hwid_mcatype = HWID_MCATYPE(0x0, 0x1); > > Why are we open-coding these types? > > Why not use smca_hwid_mcatypes[SMCA_CS_V2], etc.? > > > + if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2)) > > + *hwid_mcatype = HWID_MCATYPE(0x0, 0x1); > > Ditto. >
This code runs before matching on a struct with the enums. It seems simplest to fixup the hardware-provided value before setting things up rather than changing things later.
Current code flow: 1) Read HWID/McaType values from hardware. 2) Loop through known types and match on the HWID_MCATYPE() tuple. 3) Continue setup based on matched type and its Linux enum.
New code flow: 1) Read HWID/McaType values from hardware. --> Fixup HWID/McaType values due to any known hardware quirks. 2) Loop through known types and match on the HWID_MCATYPE() tuple. 3) Continue setup based on matched type and its Linux enum.
What do you think?
Also, a further update (maybe you're alluding to?) is get rid of the struct smca_hwid and just define an enum with "NAME = HWID_MCATYPE(XXX, YYY)".
The struct smca_hwid had another field that was removed, so it seems unnecessary at the moment.
Thanks, Yazen
| |