[PATCH v3 0/2] EDAC/amd64: Fix UMC csrow decode and consolidate Family 1Ah setup

[PATCH v3 0/2] EDAC/amd64: Fix UMC csrow decode and consolidate Family 1Ah setup

[Vishal Badole]

Two small amd64_edac fixes/cleanups for AMD Family 1Ah.

Patch 1 is the fix: mask the UMC chip select with 0x3, not 0x7. Only
four chip selects exist, so the old mask could decode an out-of-range
csrow and trip an EDAC warning.

Patch 2 sets zn_regs_v2 once at the Family 1Ah level instead of in each
model case, so future models cannot miss the v2 register layout.

Changes since v2:
 - Patch 1 (fix): Simplified the commit message and updated the code
   comment.

Changes since v1:
 - Patch 1 (fix): Added Fixes: and Cc: stable, and move the fix first.
 - Patch 2 (cleanup): Updated the commit message.

v2: https://lore.kernel.org/all/20260928180935.472319-1-Vishal.Badole@amd.com/
v1: https://lore.kernel.org/all/20260925172639.97063-1-Vishal.Badole@amd.com/

Vishal Badole (2):
  EDAC/amd64: Mask UMC chip select to the four implemented selects
  EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

 drivers/edac/amd64_edac.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] EDAC/amd64: Mask UMC chip select to the four implemented selects

[Vishal Badole]

For DRAM ECC errors, the chip select value can be found in the
MCA_SYND[ErrorInformation] field.

ErrorInformation[2:0] was first documented in Zen1. The 3 bit field
implied that up to 8 chip selects may be used.

However, all Zen-based systems have only ever had up to 4 chip selects.
So only bits [1:0] would contain a valid chip select value.

Future systems are redefining the field to ErrorInformation[1:0]. This
matches the existing 'up to 4 chip selects' implementations. And it
frees up the extra bit for other decoding.

Change the current chip select mask from 0x7 to 0x3 to prepare for the
decode change.

Fixes: 713ad54675fd ("EDAC, amd64: Define and register UMC error decode function")
Cc: <stable@vger.kernel.org>
Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/edac/amd64_edac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 475235c402e8..0c0d11c72925 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2796,13 +2796,13 @@ static inline void decode_bus_error(int node_id, struct mce *m)
  * the instance_id. For example, instance_id=0xYXXXXX where Y is the channel
  * number.
  *
- * For DRAM ECC errors, the Chip Select number is given in bits [2:0] of
+ * For DRAM ECC errors, the Chip Select number is given in bits [1:0] of
  * the MCA_SYND[ErrorInformation] field.
  */
 static void umc_get_err_info(struct mce *m, struct err_info *err)
 {
 	err->channel = (m->ipid & GENMASK(31, 0)) >> 20;
-	err->csrow = m->synd & 0x7;
+	err->csrow = m->synd & 0x3;
 }
 
 static void decode_umc_error(int node_id, struct mce *m)
-- 
2.34.1

[PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Vishal Badole]

All Family 1Ah models use the v2 register layout. Today zn_regs_v2 is
set in each model case. This works, but a new model could forget to set
it and read the UMC registers at the wrong offsets.

Set zn_regs_v2 once at the Family 1Ah level so it applies to all models
uniformly, and drop the per-model assignments.

No functional change intended.

Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/edac/amd64_edac.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 0c0d11c72925..708fc1b4a999 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3887,23 +3887,19 @@ static int per_family_init(struct amd64_pvt *pvt)
 		break;
 
 	case 0x1A:
+		pvt->flags.zn_regs_v2   = 1;
+
 		switch (pvt->model) {
 		case 0x00 ... 0x1f:
 			pvt->max_mcs            = 12;
-			pvt->flags.zn_regs_v2   = 1;
-			break;
-		case 0x40 ... 0x4f:
-			pvt->flags.zn_regs_v2   = 1;
 			break;
 		case 0x50 ... 0x57:
 		case 0xc0 ... 0xc7:
 			pvt->max_mcs            = 16;
-			pvt->flags.zn_regs_v2   = 1;
 			break;
 		case 0x90 ... 0x9f:
 		case 0xa0 ... 0xaf:
 			pvt->max_mcs            = 8;
-			pvt->flags.zn_regs_v2   = 1;
 			break;
 		}
 		break;
-- 
2.34.1

Re: [PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Borislav Petkov]

On Fri, Oct 02, 2026 at 05:02:04PM +0530, Vishal Badole wrote:
> All Family 1Ah models use the v2 register layout. Today zn_regs_v2 is
> set in each model case. This works, but a new model could forget to set
> it and read the UMC registers at the wrong offsets.
> 
> Set zn_regs_v2 once at the Family 1Ah level so it applies to all models
> uniformly, and drop the per-model assignments.
> 
> No functional change intended.
> 
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/edac/amd64_edac.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 0c0d11c72925..708fc1b4a999 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3887,23 +3887,19 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		break;
>  
>  	case 0x1A:
> +		pvt->flags.zn_regs_v2   = 1;
> +
>  		switch (pvt->model) {
>  		case 0x00 ... 0x1f:
>  			pvt->max_mcs            = 12;
> -			pvt->flags.zn_regs_v2   = 1;
> -			break;
> -		case 0x40 ... 0x4f:
		^^^^^^^^^^^^^^^^^^^

Why?

> -			pvt->flags.zn_regs_v2   = 1;
>  			break;
>  		case 0x50 ... 0x57:
>  		case 0xc0 ... 0xc7:
>  			pvt->max_mcs            = 16;
> -			pvt->flags.zn_regs_v2   = 1;
>  			break;
>  		case 0x90 ... 0x9f:
>  		case 0xa0 ... 0xaf:
>  			pvt->max_mcs            = 8;
> -			pvt->flags.zn_regs_v2   = 1;
>  			break;
>  		}
>  		break;
> -- 
> 2.34.1
> 
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Yazen Ghannam]

On Fri, Oct 02, 2026 at 06:22:38PM -0700, Borislav Petkov wrote:
> On Fri, Oct 02, 2026 at 05:02:04PM +0530, Vishal Badole wrote:
> > All Family 1Ah models use the v2 register layout. Today zn_regs_v2 is
> > set in each model case. This works, but a new model could forget to set
> > it and read the UMC registers at the wrong offsets.
> > 
> > Set zn_regs_v2 once at the Family 1Ah level so it applies to all models
> > uniformly, and drop the per-model assignments.
> > 
> > No functional change intended.
> > 
> > Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  drivers/edac/amd64_edac.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> > index 0c0d11c72925..708fc1b4a999 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -3887,23 +3887,19 @@ static int per_family_init(struct amd64_pvt *pvt)
> >  		break;
> >  
> >  	case 0x1A:
> > +		pvt->flags.zn_regs_v2   = 1;
> > +
> >  		switch (pvt->model) {
> >  		case 0x00 ... 0x1f:
> >  			pvt->max_mcs            = 12;
> > -			pvt->flags.zn_regs_v2   = 1;
> > -			break;
> > -		case 0x40 ... 0x4f:
> 		^^^^^^^^^^^^^^^^^^^
> 
> Why?
> 
> > -			pvt->flags.zn_regs_v2   = 1;
> >  			break;

Because it becomes an empty case once this flag set is moved up.

Thanks,
Yazen

Re: [PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Borislav Petkov]

On Sun, Oct 04, 2026 at 11:14:47AM -0400, Yazen Ghannam wrote:
> Because it becomes an empty case once this flag set is moved up.

No, models 0x40... are supported. The others which are not explicitly listed
there are not.

And no, the code doesn't enforce it yet but probably it should. And there
should be a default: label for the models too which returns -ENODEV, like it
does for the unknown families.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Yazen Ghannam]

On Sun, Oct 04, 2026 at 09:04:12AM -0700, Borislav Petkov wrote:
> On Sun, Oct 04, 2026 at 11:14:47AM -0400, Yazen Ghannam wrote:
> > Because it becomes an empty case once this flag set is moved up.
> 
> No, models 0x40... are supported. The others which are not explicitly listed
> there are not.
> 
> And no, the code doesn't enforce it yet but probably it should. And there
> should be a default: label for the models too which returns -ENODEV, like it
> does for the unknown families.
> 

I see what you mean, but I think that's a bigger issue with the current
design. And I want to move away from that.

The current design is "opt-in" for each new model/group even if there's
no technical difference. So we keep having to write these minor model
check patches just to load the module.

I'd rather we load unconditionally for all models with the same base
behavior using sane defaults. Then we can have model-specific patches
for variations, if needed.

Essentially, we could avoid a whole class of patches for derivative
(client, embedded, etc.) products. The module would load with the sane
defaults. If the test folks find an issue, then we can have a
model-specific patch.

This is how we've been trending with AMD64 EDAC. The family init has
been shrinking over the years.

Thanks,
Yazen

Re: [PATCH v3 2/2] EDAC/amd64: Set zn_regs_v2 for all Family 1Ah models

[Borislav Petkov]

On Sun, Oct 04, 2026 at 01:05:25PM -0400, Yazen Ghannam wrote:
> I'd rather we load unconditionally for all models with the same base
> behavior using sane defaults. Then we can have model-specific patches
> for variations, if needed.

I'd rather not because I keep getting all those: amd64_edac doesn't load on my
machine reports. Well, after a while it turns out that it should not load
there in the first place.

And then there's the managerial checkbox patch which needs to add support for
their new model just because... does it even make sense to add support? Oh, we
didn't even think of that but it says "Unsupported" so we thought we should
"fix" the error message... 

So I don't want to have that unnecessary waste of everything. And if a f/m/s
would keep my sanity, then I'm perfectly fine with it.

> Essentially, we could avoid a whole class of patches for derivative
> (client, embedded, etc.) products. The module would load with the sane
> defaults. If the test folks find an issue, then we can have a
> model-specific patch.

Only on well-tested and supported configurations. Everything else doesn't
work.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette