[v2] Add support for AMD Family 1Ah-based SOCs

[PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

Posted by Avadhut Naik 1 month, 3 weeks ago

Newer AMD systems can support up to 16 channels per EDAC "mc" device.
These are detected by the EDAC module running on the device, and the
current EDAC interface is appropriately enumerated.

The legacy EDAC sysfs interface however, provides device attributes for
channels 0 through 11 only. Consequently, the last four channels, 12
through 15, will not be enumerated and will not be visible through the
legacy sysfs interface.

Add additional device attributes to ensure that all 16 channels, if
present, are enumerated by and visible through the legacy EDAC sysfs
interface.

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
---
 drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0f338adf7d93..8689631f1905 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
 	channel_dimm_label_show, channel_dimm_label_store, 10);
 DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
 	channel_dimm_label_show, channel_dimm_label_store, 11);
+DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 12);
+DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 13);
+DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 14);
+DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 15);
 
 /* Total possible dynamic DIMM Label attribute file table */
 static struct attribute *dynamic_csrow_dimm_attr[] = {
@@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
 	&dev_attr_legacy_ch9_dimm_label.attr.attr,
 	&dev_attr_legacy_ch10_dimm_label.attr.attr,
 	&dev_attr_legacy_ch11_dimm_label.attr.attr,
+	&dev_attr_legacy_ch12_dimm_label.attr.attr,
+	&dev_attr_legacy_ch13_dimm_label.attr.attr,
+	&dev_attr_legacy_ch14_dimm_label.attr.attr,
+	&dev_attr_legacy_ch15_dimm_label.attr.attr,
 	NULL
 };
 
@@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
 		   channel_ce_count_show, NULL, 10);
 DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
 		   channel_ce_count_show, NULL, 11);
+DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 12);
+DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 13);
+DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 14);
+DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 15);
 
 /* Total possible dynamic ce_count attribute file table */
 static struct attribute *dynamic_csrow_ce_count_attr[] = {
@@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
 	&dev_attr_legacy_ch9_ce_count.attr.attr,
 	&dev_attr_legacy_ch10_ce_count.attr.attr,
 	&dev_attr_legacy_ch11_ce_count.attr.attr,
+	&dev_attr_legacy_ch12_ce_count.attr.attr,
+	&dev_attr_legacy_ch13_ce_count.attr.attr,
+	&dev_attr_legacy_ch14_ce_count.attr.attr,
+	&dev_attr_legacy_ch15_ce_count.attr.attr,
 	NULL
 };
 
-- 
2.43.0

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

Posted by Borislav Petkov 1 month, 2 weeks ago

On Thu, Aug 07, 2025 at 08:14:54PM +0000, Avadhut Naik wrote:
> Newer AMD systems can support up to 16 channels per EDAC "mc" device.
> These are detected by the EDAC module running on the device, and the
> current EDAC interface is appropriately enumerated.
> 
> The legacy EDAC sysfs interface however, provides device attributes for
> channels 0 through 11 only. Consequently, the last four channels, 12
> through 15, will not be enumerated and will not be visible through the
> legacy sysfs interface.
> 
> Add additional device attributes to ensure that all 16 channels, if
> present, are enumerated by and visible through the legacy EDAC sysfs
> interface.
> 
> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> ---
>  drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 0f338adf7d93..8689631f1905 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
>  	channel_dimm_label_show, channel_dimm_label_store, 10);
>  DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
>  	channel_dimm_label_show, channel_dimm_label_store, 11);
> +DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 12);
> +DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 13);
> +DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 14);
> +DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 15);
>  
>  /* Total possible dynamic DIMM Label attribute file table */
>  static struct attribute *dynamic_csrow_dimm_attr[] = {
> @@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
>  	&dev_attr_legacy_ch9_dimm_label.attr.attr,
>  	&dev_attr_legacy_ch10_dimm_label.attr.attr,
>  	&dev_attr_legacy_ch11_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch12_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch13_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch14_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch15_dimm_label.attr.attr,
>  	NULL
>  };
>  
> @@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
>  		   channel_ce_count_show, NULL, 10);
>  DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
>  		   channel_ce_count_show, NULL, 11);
> +DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 12);
> +DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 13);
> +DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 14);
> +DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 15);
>  
>  /* Total possible dynamic ce_count attribute file table */
>  static struct attribute *dynamic_csrow_ce_count_attr[] = {
> @@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
>  	&dev_attr_legacy_ch9_ce_count.attr.attr,
>  	&dev_attr_legacy_ch10_ce_count.attr.attr,
>  	&dev_attr_legacy_ch11_ce_count.attr.attr,
> +	&dev_attr_legacy_ch12_ce_count.attr.attr,
> +	&dev_attr_legacy_ch13_ce_count.attr.attr,
> +	&dev_attr_legacy_ch14_ce_count.attr.attr,
> +	&dev_attr_legacy_ch15_ce_count.attr.attr,
>  	NULL
>  };

This is also slowly getting out of hand. All those should be allocated
and initialized dynamically based on a number of MCs but not keep adding them
here ad absurdum.

Because if we were doing them dynamically, we'd never ever miss any new
channels when the hw grows...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

Posted by Yazen Ghannam 1 month, 1 week ago

On Mon, Aug 18, 2025 at 11:36:51PM +0200, Borislav Petkov wrote:
> On Thu, Aug 07, 2025 at 08:14:54PM +0000, Avadhut Naik wrote:
> > Newer AMD systems can support up to 16 channels per EDAC "mc" device.
> > These are detected by the EDAC module running on the device, and the
> > current EDAC interface is appropriately enumerated.
> > 
> > The legacy EDAC sysfs interface however, provides device attributes for
> > channels 0 through 11 only. Consequently, the last four channels, 12
> > through 15, will not be enumerated and will not be visible through the
> > legacy sysfs interface.
> > 
> > Add additional device attributes to ensure that all 16 channels, if
> > present, are enumerated by and visible through the legacy EDAC sysfs
> > interface.
> > 
> > Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> > ---
> >  drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 0f338adf7d93..8689631f1905 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
> >  	channel_dimm_label_show, channel_dimm_label_store, 10);
> >  DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
> >  	channel_dimm_label_show, channel_dimm_label_store, 11);
> > +DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 12);
> > +DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 13);
> > +DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 14);
> > +DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 15);
> >  
> >  /* Total possible dynamic DIMM Label attribute file table */
> >  static struct attribute *dynamic_csrow_dimm_attr[] = {
> > @@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
> >  	&dev_attr_legacy_ch9_dimm_label.attr.attr,
> >  	&dev_attr_legacy_ch10_dimm_label.attr.attr,
> >  	&dev_attr_legacy_ch11_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch12_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch13_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch14_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch15_dimm_label.attr.attr,
> >  	NULL
> >  };
> >  
> > @@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
> >  		   channel_ce_count_show, NULL, 10);
> >  DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
> >  		   channel_ce_count_show, NULL, 11);
> > +DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 12);
> > +DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 13);
> > +DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 14);
> > +DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 15);
> >  
> >  /* Total possible dynamic ce_count attribute file table */
> >  static struct attribute *dynamic_csrow_ce_count_attr[] = {
> > @@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
> >  	&dev_attr_legacy_ch9_ce_count.attr.attr,
> >  	&dev_attr_legacy_ch10_ce_count.attr.attr,
> >  	&dev_attr_legacy_ch11_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch12_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch13_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch14_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch15_ce_count.attr.attr,
> >  	NULL
> >  };
> 
> This is also slowly getting out of hand. All those should be allocated
> and initialized dynamically based on a number of MCs but not keep adding them
> here ad absurdum.
> 
> Because if we were doing them dynamically, we'd never ever miss any new
> channels when the hw grows...
> 

Maybe it's time to final remove this legacy interface? It's been
obsolete for more than a decade now.

199747106934 ("edac: add a new per-dimm API and make the old per-virtual-rank API obsolete")

Any guidance on the best way to remove this?

Thanks,
Yazen

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

Posted by Borislav Petkov 1 month, 1 week ago

On Tue, Aug 19, 2025 at 09:30:40AM -0400, Yazen Ghannam wrote:
> Maybe it's time to final remove this legacy interface? It's been
> obsolete for more than a decade now.
> 
> 199747106934 ("edac: add a new per-dimm API and make the old per-virtual-rank API obsolete")

Bah, look how time flies... :-\

Good catch - I had completely forgotten about this gunk.

> Any guidance on the best way to remove this?

Yeah, you set it to default n in Kconfig and add a warning when someone reads
the old sysfs nodes. Something along the lines that this interface has been
deprecated for a decade now and that people should switch to the new dimm
interface and that it will be removed in, say, 2-3 kernel releases or so.

I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

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