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

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: <20250819133040.GA359442@yaz-khff2.amd.com>
Date: Tue, 19 Aug 2025 09:30:40 -0400
From: Yazen Ghannam <yazen.ghannam@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: Avadhut Naik <avadhut.naik@....com>, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to
 16

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@....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