* [PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs @ 2025-08-07 20:14 Avadhut Naik 2025-08-07 20:14 ` [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models Avadhut Naik 2025-08-07 20:14 ` [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 Avadhut Naik 0 siblings, 2 replies; 9+ messages in thread From: Avadhut Naik @ 2025-08-07 20:14 UTC (permalink / raw) To: linux-edac; +Cc: bp, yazen.ghannam, linux-kernel, avadhut.naik This patchset adds support for newer AMD's Family 1Ah-based SOCs. The first patch adds support for these new SOCs in the amd64_edac module. The second patch modifies the legacy EDAC sysfs interface so that the 16 UMCs supported by some of these new SOCs are appropriately visible through the interface. Changes in v2: - Remove extra tabs. - Update the legacy EDAC sysfs interface. Links: v1: https://lore.kernel.org/all/20250722192855.3108575-1-avadhut.naik@amd.com/ Avadhut Naik (2): EDAC/amd64: Add support for AMD family 1Ah-based newer models EDAC/mc_sysfs: Increase legacy channel support to 16 drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++ drivers/edac/amd64_edac.h | 2 +- drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) base-commit: 1fb0ddddf5d139089675b86702933cbca992b4d4 -- 2.43.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models 2025-08-07 20:14 [PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs Avadhut Naik @ 2025-08-07 20:14 ` Avadhut Naik 2025-08-18 21:22 ` Borislav Petkov 2025-08-07 20:14 ` [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 Avadhut Naik 1 sibling, 1 reply; 9+ messages in thread From: Avadhut Naik @ 2025-08-07 20:14 UTC (permalink / raw) To: linux-edac; +Cc: bp, yazen.ghannam, linux-kernel, avadhut.naik Add support for family 1Ah-based models 50h-57h, 90h-9Fh, A0h-AFh, and C0h-C7h. Signed-off-by: Avadhut Naik <avadhut.naik@amd.com> --- drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++ drivers/edac/amd64_edac.h | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 07f1e9dc1ca7..2f6ab783bf20 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -3923,6 +3923,26 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ctl_name = "F1Ah_M40h"; pvt->flags.zn_regs_v2 = 1; break; + case 0x50 ... 0x57: + pvt->ctl_name = "F1Ah_M50h"; + pvt->max_mcs = 16; + pvt->flags.zn_regs_v2 = 1; + break; + case 0x90 ... 0x9f: + pvt->ctl_name = "F1Ah_M90h"; + pvt->max_mcs = 8; + pvt->flags.zn_regs_v2 = 1; + break; + case 0xa0 ... 0xaf: + pvt->ctl_name = "F1Ah_MA0h"; + pvt->max_mcs = 8; + pvt->flags.zn_regs_v2 = 1; + break; + case 0xc0 ... 0xc7: + pvt->ctl_name = "F1Ah_MC0h"; + pvt->max_mcs = 16; + pvt->flags.zn_regs_v2 = 1; + break; } break; diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h index 17228d07de4c..d70b8a8d0b09 100644 --- a/drivers/edac/amd64_edac.h +++ b/drivers/edac/amd64_edac.h @@ -96,7 +96,7 @@ /* Hardware limit on ChipSelect rows per MC and processors per system */ #define NUM_CHIPSELECTS 8 #define DRAM_RANGES 8 -#define NUM_CONTROLLERS 12 +#define NUM_CONTROLLERS 16 #define ON true #define OFF false -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models 2025-08-07 20:14 ` [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models Avadhut Naik @ 2025-08-18 21:22 ` Borislav Petkov 2025-08-25 14:30 ` Naik, Avadhut 2025-08-25 15:23 ` Naik, Avadhut 0 siblings, 2 replies; 9+ messages in thread From: Borislav Petkov @ 2025-08-18 21:22 UTC (permalink / raw) To: Avadhut Naik; +Cc: linux-edac, yazen.ghannam, linux-kernel On Thu, Aug 07, 2025 at 08:14:53PM +0000, Avadhut Naik wrote: > Add support for family 1Ah-based models 50h-57h, 90h-9Fh, A0h-AFh, and > C0h-C7h. > > Signed-off-by: Avadhut Naik <avadhut.naik@amd.com> > --- > drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++ > drivers/edac/amd64_edac.h | 2 +- > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index 07f1e9dc1ca7..2f6ab783bf20 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -3923,6 +3923,26 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ctl_name = "F1Ah_M40h"; > pvt->flags.zn_regs_v2 = 1; > break; > + case 0x50 ... 0x57: > + pvt->ctl_name = "F1Ah_M50h"; > + pvt->max_mcs = 16; > + pvt->flags.zn_regs_v2 = 1; > + break; > + case 0x90 ... 0x9f: > + pvt->ctl_name = "F1Ah_M90h"; > + pvt->max_mcs = 8; > + pvt->flags.zn_regs_v2 = 1; > + break; > + case 0xa0 ... 0xaf: > + pvt->ctl_name = "F1Ah_MA0h"; > + pvt->max_mcs = 8; > + pvt->flags.zn_regs_v2 = 1; > + break; > + case 0xc0 ... 0xc7: > + pvt->ctl_name = "F1Ah_MC0h"; > + pvt->max_mcs = 16; > + pvt->flags.zn_regs_v2 = 1; > + break; > } Oh boy, this pvt->ctl_name is ridiculous: we're collecting a zoo of string objects where this thing can simply be scnprintf()-ed once at driver init and then the pointer handed in to mci->ctl_name. Something like static char *ctl_name[MAX_CTL_NAMELEN]; or so. And then scnprintf(...) into it based on family and model. Can you pls do that as a cleanup upfront? This is slowly getting out of hand. And then you can group them together: case 0x50 ... 0x57: case 0xc0 ... 0xc7: pvt->max_mcs = 16; pvt->flags.zn_regs_v2 = 1; break; ... and so on. So that this function turns back into something saner again. Thx. > break; > > diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h > index 17228d07de4c..d70b8a8d0b09 100644 > --- a/drivers/edac/amd64_edac.h > +++ b/drivers/edac/amd64_edac.h > @@ -96,7 +96,7 @@ > /* Hardware limit on ChipSelect rows per MC and processors per system */ > #define NUM_CHIPSELECTS 8 > #define DRAM_RANGES 8 > -#define NUM_CONTROLLERS 12 > +#define NUM_CONTROLLERS 16 I've asked this before but can we read out the number of controllers from the hw somewhere instead of diddling with this silly define all the time? Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models 2025-08-18 21:22 ` Borislav Petkov @ 2025-08-25 14:30 ` Naik, Avadhut 2025-08-25 15:23 ` Naik, Avadhut 1 sibling, 0 replies; 9+ messages in thread From: Naik, Avadhut @ 2025-08-25 14:30 UTC (permalink / raw) To: Borislav Petkov, Avadhut Naik; +Cc: linux-edac, yazen.ghannam, linux-kernel On 8/18/2025 16:22, Borislav Petkov wrote: > On Thu, Aug 07, 2025 at 08:14:53PM +0000, Avadhut Naik wrote: >> Add support for family 1Ah-based models 50h-57h, 90h-9Fh, A0h-AFh, and >> C0h-C7h. >> >> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com> >> --- >> drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++ >> drivers/edac/amd64_edac.h | 2 +- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c >> index 07f1e9dc1ca7..2f6ab783bf20 100644 >> --- a/drivers/edac/amd64_edac.c >> +++ b/drivers/edac/amd64_edac.c >> @@ -3923,6 +3923,26 @@ static int per_family_init(struct amd64_pvt *pvt) >> pvt->ctl_name = "F1Ah_M40h"; >> pvt->flags.zn_regs_v2 = 1; >> break; >> + case 0x50 ... 0x57: >> + pvt->ctl_name = "F1Ah_M50h"; >> + pvt->max_mcs = 16; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0x90 ... 0x9f: >> + pvt->ctl_name = "F1Ah_M90h"; >> + pvt->max_mcs = 8; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0xa0 ... 0xaf: >> + pvt->ctl_name = "F1Ah_MA0h"; >> + pvt->max_mcs = 8; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0xc0 ... 0xc7: >> + pvt->ctl_name = "F1Ah_MC0h"; >> + pvt->max_mcs = 16; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> } > > Oh boy, this pvt->ctl_name is ridiculous: we're collecting a zoo of string > objects where this thing can simply be scnprintf()-ed once at driver init and > then the pointer handed in to mci->ctl_name. > > Something like > > static char *ctl_name[MAX_CTL_NAMELEN]; > > or so. And then > > scnprintf(...) > > into it based on family and model. > > Can you pls do that as a cleanup upfront? > > This is slowly getting out of hand. > > And then you can group them together: > > case 0x50 ... 0x57: > case 0xc0 ... 0xc7: > pvt->max_mcs = 16; > pvt->flags.zn_regs_v2 = 1; > break; > > ... > > and so on. > > So that this function turns back into something saner again. > > Thx. > > > > > >> break; >> >> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h >> index 17228d07de4c..d70b8a8d0b09 100644 >> --- a/drivers/edac/amd64_edac.h >> +++ b/drivers/edac/amd64_edac.h >> @@ -96,7 +96,7 @@ >> /* Hardware limit on ChipSelect rows per MC and processors per system */ >> #define NUM_CHIPSELECTS 8 >> #define DRAM_RANGES 8 >> -#define NUM_CONTROLLERS 12 >> +#define NUM_CONTROLLERS 16 > > I've asked this before but can we read out the number of controllers from the > hw somewhere instead of diddling with this silly define all the time? > > Thx. > Will take a look at this! -- Thanks, Avadhut Naik ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models 2025-08-18 21:22 ` Borislav Petkov 2025-08-25 14:30 ` Naik, Avadhut @ 2025-08-25 15:23 ` Naik, Avadhut 1 sibling, 0 replies; 9+ messages in thread From: Naik, Avadhut @ 2025-08-25 15:23 UTC (permalink / raw) To: Borislav Petkov, Avadhut Naik; +Cc: linux-edac, yazen.ghannam, linux-kernel Apologies! Mistakenly hit send early in my previous response. On 8/18/2025 16:22, Borislav Petkov wrote: > On Thu, Aug 07, 2025 at 08:14:53PM +0000, Avadhut Naik wrote: >> Add support for family 1Ah-based models 50h-57h, 90h-9Fh, A0h-AFh, and >> C0h-C7h. >> >> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com> >> --- >> drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++ >> drivers/edac/amd64_edac.h | 2 +- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c >> index 07f1e9dc1ca7..2f6ab783bf20 100644 >> --- a/drivers/edac/amd64_edac.c >> +++ b/drivers/edac/amd64_edac.c >> @@ -3923,6 +3923,26 @@ static int per_family_init(struct amd64_pvt *pvt) >> pvt->ctl_name = "F1Ah_M40h"; >> pvt->flags.zn_regs_v2 = 1; >> break; >> + case 0x50 ... 0x57: >> + pvt->ctl_name = "F1Ah_M50h"; >> + pvt->max_mcs = 16; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0x90 ... 0x9f: >> + pvt->ctl_name = "F1Ah_M90h"; >> + pvt->max_mcs = 8; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0xa0 ... 0xaf: >> + pvt->ctl_name = "F1Ah_MA0h"; >> + pvt->max_mcs = 8; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> + case 0xc0 ... 0xc7: >> + pvt->ctl_name = "F1Ah_MC0h"; >> + pvt->max_mcs = 16; >> + pvt->flags.zn_regs_v2 = 1; >> + break; >> } > > Oh boy, this pvt->ctl_name is ridiculous: we're collecting a zoo of string > objects where this thing can simply be scnprintf()-ed once at driver init and > then the pointer handed in to mci->ctl_name. > > Something like > > static char *ctl_name[MAX_CTL_NAMELEN]; > > or so. And then > > scnprintf(...) > > into it based on family and model. > > Can you pls do that as a cleanup upfront? > > This is slowly getting out of hand. > > And then you can group them together: > > case 0x50 ... 0x57: > case 0xc0 ... 0xc7: > pvt->max_mcs = 16; > pvt->flags.zn_regs_v2 = 1; > break; > > ... > > and so on. > > So that this function turns back into something saner again. > > Thx. > Will try to put this in place and make it the fist patch of this set. > > > > >> break; >> >> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h >> index 17228d07de4c..d70b8a8d0b09 100644 >> --- a/drivers/edac/amd64_edac.h >> +++ b/drivers/edac/amd64_edac.h >> @@ -96,7 +96,7 @@ >> /* Hardware limit on ChipSelect rows per MC and processors per system */ >> #define NUM_CHIPSELECTS 8 >> #define DRAM_RANGES 8 >> -#define NUM_CONTROLLERS 12 >> +#define NUM_CONTROLLERS 16 > > I've asked this before but can we read out the number of controllers from the > hw somewhere instead of diddling with this silly define all the time? > > Thx. > Had considered this sometime back. Didn't pursue further as it maybe somewhat tricky. Some new CPUIDs were introduced since Zen4 trough which we MAYBE able to accomplish this i.e get rid of the macro. But am not not sure as to what we should do for older SOCs in that case. Zen3, for example. Will look more if I can find something else. -- Thanks, Avadhut Naik ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 2025-08-07 20:14 [PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs Avadhut Naik 2025-08-07 20:14 ` [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models Avadhut Naik @ 2025-08-07 20:14 ` Avadhut Naik 2025-08-18 21:36 ` Borislav Petkov 1 sibling, 1 reply; 9+ messages in thread From: Avadhut Naik @ 2025-08-07 20:14 UTC (permalink / raw) To: linux-edac; +Cc: bp, yazen.ghannam, linux-kernel, avadhut.naik 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 2025-08-07 20:14 ` [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 Avadhut Naik @ 2025-08-18 21:36 ` Borislav Petkov 2025-08-19 13:30 ` Yazen Ghannam 0 siblings, 1 reply; 9+ messages in thread From: Borislav Petkov @ 2025-08-18 21:36 UTC (permalink / raw) To: Avadhut Naik; +Cc: linux-edac, yazen.ghannam, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 2025-08-18 21:36 ` Borislav Petkov @ 2025-08-19 13:30 ` Yazen Ghannam 2025-08-19 13:48 ` Borislav Petkov 0 siblings, 1 reply; 9+ messages in thread From: Yazen Ghannam @ 2025-08-19 13:30 UTC (permalink / raw) To: Borislav Petkov; +Cc: Avadhut Naik, linux-edac, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 2025-08-19 13:30 ` Yazen Ghannam @ 2025-08-19 13:48 ` Borislav Petkov 0 siblings, 0 replies; 9+ messages in thread From: Borislav Petkov @ 2025-08-19 13:48 UTC (permalink / raw) To: Yazen Ghannam; +Cc: Avadhut Naik, linux-edac, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-08-25 15:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-07 20:14 [PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs Avadhut Naik 2025-08-07 20:14 ` [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models Avadhut Naik 2025-08-18 21:22 ` Borislav Petkov 2025-08-25 14:30 ` Naik, Avadhut 2025-08-25 15:23 ` Naik, Avadhut 2025-08-07 20:14 ` [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16 Avadhut Naik 2025-08-18 21:36 ` Borislav Petkov 2025-08-19 13:30 ` Yazen Ghannam 2025-08-19 13:48 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).