[PATCH 2/4] EDAC/amd64: Remove NUM_CONTROLLERS macro

Avadhut Naik posted 4 patches 2 months ago
[PATCH 2/4] EDAC/amd64: Remove NUM_CONTROLLERS macro
Posted by Avadhut Naik 2 months ago
Currently, the NUM_CONTROLLERS macro is only used to statically allocate
the csels array of struct chip_select in struct amd64_pvt.

The size of this array, however, will never exceed the number of UMCs on
the SOC. Since, max_mcs variable in struct amd64_pvt already stores the
number of UMCs on the SOC, the macro can be removed and the static array
can be dynamically allocated instead.

The max_mcs variable and the csels array are used for legacy systems too.
These systems have a max of 2 controllers (DCTs). Since the default value
of max_mcs, set in per_family_init(), is 2, these legacy system are also
covered by this change.

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
---
 drivers/edac/amd64_edac.c | 5 +++++
 drivers/edac/amd64_edac.h | 5 ++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 886ad075d222..2391f3469961 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3732,6 +3732,7 @@ static void hw_info_put(struct amd64_pvt *pvt)
 	pci_dev_put(pvt->F1);
 	pci_dev_put(pvt->F2);
 	kfree(pvt->umc);
+	kfree(pvt->csels);
 }
 
 static struct low_ops umc_ops = {
@@ -3915,6 +3916,10 @@ static int per_family_init(struct amd64_pvt *pvt)
 		scnprintf(pvt->ctl_name, sizeof(pvt->ctl_name), "F%02Xh_M%02Xh",
 			  pvt->fam, pvt->model);
 
+	pvt->csels = kcalloc(pvt->max_mcs, sizeof(*pvt->csels), GFP_KERNEL);
+	if (!pvt->csels)
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index bb6cf4b1ab77..5f61631c8a7d 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -96,7 +96,6 @@
 /* Hardware limit on ChipSelect rows per MC and processors per system */
 #define NUM_CHIPSELECTS			8
 #define DRAM_RANGES			8
-#define NUM_CONTROLLERS			16
 
 #define ON true
 #define OFF false
@@ -347,8 +346,8 @@ struct amd64_pvt {
 	u32 dbam0;		/* DRAM Base Address Mapping reg for DCT0 */
 	u32 dbam1;		/* DRAM Base Address Mapping reg for DCT1 */
 
-	/* one for each DCT/UMC */
-	struct chip_select csels[NUM_CONTROLLERS];
+	/* Allocate one for each DCT/UMC */
+	struct chip_select *csels;
 
 	/* DRAM base and limit pairs F1x[78,70,68,60,58,50,48,40] */
 	struct dram_range ranges[DRAM_RANGES];
-- 
2.43.0
Re: [PATCH 2/4] EDAC/amd64: Remove NUM_CONTROLLERS macro
Posted by Borislav Petkov 1 month, 4 weeks ago
On Mon, Oct 13, 2025 at 05:30:41PM +0000, Avadhut Naik wrote:
> Currently, the NUM_CONTROLLERS macro is only used to statically allocate
> the csels array of struct chip_select in struct amd64_pvt.

"... is used to limit the amount of memory controllers available per node."

You don't need to explain the code - think big picture.

> The size of this array, however, will never exceed the number of UMCs on
> the SOC.

Not on the SOC - the thing is per node instance.

> Since, max_mcs variable in struct amd64_pvt already stores the
> number of UMCs on the SOC, the macro can be removed and the static array

Please describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

> can be dynamically allocated instead.
> 
> The max_mcs variable and the csels array are used for legacy systems too.
> These systems have a max of 2 controllers (DCTs). Since the default value

DCTs are DRAM controllers. Do not confuse the reader.

> of max_mcs, set in per_family_init(), is 2, these legacy system are also
> covered by this change.

...

> @@ -347,8 +346,8 @@ struct amd64_pvt {
>  	u32 dbam0;		/* DRAM Base Address Mapping reg for DCT0 */
>  	u32 dbam1;		/* DRAM Base Address Mapping reg for DCT1 */
>  
> -	/* one for each DCT/UMC */
> -	struct chip_select csels[NUM_CONTROLLERS];
> +	/* Allocate one for each DCT/UMC */

You're not allocating here anything. Just explain what this variable
represents - IOW, the comment was fine.

> +	struct chip_select *csels;
>  
>  	/* DRAM base and limit pairs F1x[78,70,68,60,58,50,48,40] */
>  	struct dram_range ranges[DRAM_RANGES];
> -- 
> 2.43.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 2/4] EDAC/amd64: Remove NUM_CONTROLLERS macro
Posted by Naik, Avadhut 1 month, 4 weeks ago

On 10/21/2025 05:44, Borislav Petkov wrote:
> On Mon, Oct 13, 2025 at 05:30:41PM +0000, Avadhut Naik wrote:
>> Currently, the NUM_CONTROLLERS macro is only used to statically allocate
>> the csels array of struct chip_select in struct amd64_pvt.
> 
> "... is used to limit the amount of memory controllers available per node."
> 
> You don't need to explain the code - think big picture.
> 
Okay!

>> The size of this array, however, will never exceed the number of UMCs on
>> the SOC.
> 
> Not on the SOC - the thing is per node instance.
>
Will change this!
 
>> Since, max_mcs variable in struct amd64_pvt already stores the
>> number of UMCs on the SOC, the macro can be removed and the static array
> 
> Please describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
Will do!

>> can be dynamically allocated instead.
>>
>> The max_mcs variable and the csels array are used for legacy systems too.
>> These systems have a max of 2 controllers (DCTs). Since the default value
> 
> DCTs are DRAM controllers. Do not confuse the reader.
> 
Will remove the *DCTs* word altogether.

>> of max_mcs, set in per_family_init(), is 2, these legacy system are also
>> covered by this change.
> 
> ...
> 
>> @@ -347,8 +346,8 @@ struct amd64_pvt {
>>  	u32 dbam0;		/* DRAM Base Address Mapping reg for DCT0 */
>>  	u32 dbam1;		/* DRAM Base Address Mapping reg for DCT1 */
>>  
>> -	/* one for each DCT/UMC */
>> -	struct chip_select csels[NUM_CONTROLLERS];
>> +	/* Allocate one for each DCT/UMC */
> 
> You're not allocating here anything. Just explain what this variable
> represents - IOW, the comment was fine.
> 
Will revert this!

>> +	struct chip_select *csels;
>>  
>>  	/* DRAM base and limit pairs F1x[78,70,68,60,58,50,48,40] */
>>  	struct dram_range ranges[DRAM_RANGES];
>> -- 
>> 2.43.0
>>
> 

-- 
Thanks,
Avadhut Naik