Re: [PATCH v3 1/5] EDAC/amd64: Generate ctl_name string at runtime

["Naik, Avadhut" <avadnaik@amd.com>]

On 9/10/2025 09:51, Yazen Ghannam wrote:
> On Tue, Sep 09, 2025 at 06:53:10PM +0000, Avadhut Naik wrote:
>> Currently, the ctl_name string is statically assigned based on the family
>> and model of the SOC when the amd64_edac module is loaded.
>>
>> The same, however, is not exactly needed as the string can be generated
>> and assigned at runtime through scnprintf().
>>
>> Remove all static assignments and generate the string at runtime. Also,
>> cleanup the switch cases which now become defunct.
>>
>> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
>> ---
>> Changes in v3:
>> Patch introduced.
>> ---
>>  drivers/edac/amd64_edac.c | 44 +++++++--------------------------------
>>  drivers/edac/amd64_edac.h |  4 +++-
>>  2 files changed, 11 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 07f1e9dc1ca7..3989794e4f29 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -3767,6 +3767,8 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  	pvt->model	= boot_cpu_data.x86_model;
>>  	pvt->fam	= boot_cpu_data.x86;
>>  	pvt->max_mcs	= 2;
> 
> Newline here, please.
> 
Okay.

>> +	scnprintf(pvt->ctl_name, sizeof(pvt->ctl_name), "F%02Xh_M%02Xh",
>> +		  pvt->fam, pvt->model);
> 
> There are a couple of special cases below.
> 
> So I think it may be better to move this part to the end...
> 
>>  
>>  	/*
>>  	 * Decide on which ops group to use here and do any family/model
>> @@ -3779,8 +3781,10 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  
>>  	switch (pvt->fam) {
>>  	case 0xf:
>> -		pvt->ctl_name				= (pvt->ext_model >= K8_REV_F) ?
>> -							  "K8 revF or later" : "K8 revE or earlier";
>> +		if (pvt->ext_model >= K8_REV_F)
>> +			strscpy(pvt->ctl_name, "K8 revF or later", sizeof(pvt->ctl_name));
>> +		else
>> +			strscpy(pvt->ctl_name, "K8 revE or earlier", sizeof(pvt->ctl_name));
> 
> Maybe save this special case to a temporary "char *name".
> 
>>  		pvt->f1_id				= PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP;
>>  		pvt->f2_id				= PCI_DEVICE_ID_AMD_K8_NB_MEMCTL;
>>  		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
>> @@ -3788,7 +3792,6 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  		break;
>>  
>>  	case 0x10:
>> -		pvt->ctl_name				= "F10h";
>>  		pvt->f1_id				= PCI_DEVICE_ID_AMD_10H_NB_MAP;
>>  		pvt->f2_id				= PCI_DEVICE_ID_AMD_10H_NB_DRAM;
>>  		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
>> @@ -3797,12 +3800,10 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  	case 0x15:
>>  		switch (pvt->model) {
>>  		case 0x30:
>> -			pvt->ctl_name			= "F15h_M30h";
>>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
>>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2;
>>  			break;
>>  		case 0x60:
>> -			pvt->ctl_name			= "F15h_M60h";
>>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F1;
>>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F2;
>>  			pvt->ops->dbam_to_cs		= f15_m60h_dbam_to_chip_select;
>> @@ -3811,7 +3812,6 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  			/* Richland is only client */
>>  			return -ENODEV;
>>  		default:
>> -			pvt->ctl_name			= "F15h";
>>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_NB_F1;
>>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_NB_F2;
>>  			pvt->ops->dbam_to_cs		= f15_dbam_to_chip_select;
>> @@ -3822,12 +3822,10 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  	case 0x16:
>>  		switch (pvt->model) {
>>  		case 0x30:
>> -			pvt->ctl_name			= "F16h_M30h";
>>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_16H_M30H_NB_F1;
>>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_16H_M30H_NB_F2;
>>  			break;
>>  		default:
>> -			pvt->ctl_name			= "F16h";
>>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_16H_NB_F1;
>>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_16H_NB_F2;
>>  			break;
>> @@ -3836,76 +3834,52 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  
>>  	case 0x17:
>>  		switch (pvt->model) {
>> -		case 0x10 ... 0x2f:
>> -			pvt->ctl_name			= "F17h_M10h";
>> -			break;
>>  		case 0x30 ... 0x3f:
>> -			pvt->ctl_name			= "F17h_M30h";
>>  			pvt->max_mcs			= 8;
>>  			break;
>> -		case 0x60 ... 0x6f:
>> -			pvt->ctl_name			= "F17h_M60h";
>> -			break;
>> -		case 0x70 ... 0x7f:
>> -			pvt->ctl_name			= "F17h_M70h";
>> -			break;
>>  		default:
>> -			pvt->ctl_name			= "F17h";
>>  			break;
>>  		}
>>  		break;
>>  
>>  	case 0x18:
>> -		pvt->ctl_name				= "F18h";
>>  		break;
>>  
>>  	case 0x19:
>>  		switch (pvt->model) {
>>  		case 0x00 ... 0x0f:
>> -			pvt->ctl_name			= "F19h";
>>  			pvt->max_mcs			= 8;
>>  			break;
>>  		case 0x10 ... 0x1f:
>> -			pvt->ctl_name			= "F19h_M10h";
>>  			pvt->max_mcs			= 12;
>>  			pvt->flags.zn_regs_v2		= 1;
>>  			break;
>> -		case 0x20 ... 0x2f:
>> -			pvt->ctl_name			= "F19h_M20h";
>> -			break;
>>  		case 0x30 ... 0x3f:
>>  			if (pvt->F3->device == PCI_DEVICE_ID_AMD_MI200_DF_F3) {
>> -				pvt->ctl_name		= "MI200";
>> +				memset(pvt->ctl_name, 0, sizeof(pvt->ctl_name));
>> +				strscpy(pvt->ctl_name, "MI200", sizeof(pvt->ctl_name));
> 
> And this to a temp name too.
> 
>>  				pvt->max_mcs		= 4;
>>  				pvt->dram_type		= MEM_HBM2;
>>  				pvt->gpu_umc_base	= 0x50000;
>>  				pvt->ops		= &gpu_ops;
>>  			} else {
>> -				pvt->ctl_name		= "F19h_M30h";
>>  				pvt->max_mcs		= 8;
>>  			}
>>  			break;
>> -		case 0x50 ... 0x5f:
>> -			pvt->ctl_name			= "F19h_M50h";
>> -			break;
>>  		case 0x60 ... 0x6f:
>> -			pvt->ctl_name			= "F19h_M60h";
>>  			pvt->flags.zn_regs_v2		= 1;
>>  			break;
>>  		case 0x70 ... 0x7f:
>> -			pvt->ctl_name			= "F19h_M70h";
>>  			pvt->max_mcs			= 4;
>>  			pvt->flags.zn_regs_v2		= 1;
>>  			break;
>>  		case 0x90 ... 0x9f:
>> -			pvt->ctl_name			= "F19h_M90h";
>>  			pvt->max_mcs			= 4;
>>  			pvt->dram_type			= MEM_HBM3;
>>  			pvt->gpu_umc_base		= 0x90000;
>>  			pvt->ops			= &gpu_ops;
>>  			break;
>>  		case 0xa0 ... 0xaf:
>> -			pvt->ctl_name			= "F19h_MA0h";
>>  			pvt->max_mcs			= 12;
>>  			pvt->flags.zn_regs_v2		= 1;
>>  			break;
>> @@ -3915,12 +3889,10 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  	case 0x1A:
>>  		switch (pvt->model) {
>>  		case 0x00 ... 0x1f:
>> -			pvt->ctl_name           = "F1Ah";
>>  			pvt->max_mcs            = 12;
>>  			pvt->flags.zn_regs_v2   = 1;
>>  			break;
>>  		case 0x40 ... 0x4f:
>> -			pvt->ctl_name           = "F1Ah_M40h";
>>  			pvt->flags.zn_regs_v2   = 1;
>>  			break;
>>  		}
> 
> ...here.
> 
> Then check if the name was set already (by the special cases). If not,
> then set the generic family/model name.
> 
> For example:
> 	char *tmp_name = NULL;
> 
> 	if K8:
> 		if F:
> 			tmp_name = "K8 F";
> 		else:
> 			tmp_name = "K8 E";
> 	if MI200:
> 		tmp_name = "MI200";
> 
> 	if (tmp_name)
> 		scnprintf(pvt->ctl_name, sizeof(pvt->ctl_name), tmp_name);
> 	else
> 		scnprintf(pvt->ctl_name, sizeof(pvt->ctl_name), "F%02Xh_M%02Xh",
> 			  pvt->fam, pvt->model);
> 
Okay! Will try this out!

-- 
Thanks,
Avadhut Naik