| 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
| ||
|
Message-ID: <8186d0e4-14a1-4198-9a9f-8082f0b289b8@amd.com>
Date: Wed, 10 Sep 2025 10:53:38 -0500
From: "Naik, Avadhut" <avadnaik@....com>
To: Yazen Ghannam <yazen.ghannam@....com>, Avadhut Naik <avadhut.naik@....com>
Cc: linux-edac@...r.kernel.org, bp@...en8.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] EDAC/amd64: Generate ctl_name string at runtime
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@....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
Powered by blists - more mailing lists