| 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: <20250910145108.GD11602@yaz-khff2.amd.com>
Date: Wed, 10 Sep 2025 10:51:08 -0400
From: Yazen Ghannam <yazen.ghannam@....com>
To: 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 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.
> + 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);
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 17228d07de4c..56999ed3ae56 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -101,6 +101,8 @@
> #define ON true
> #define OFF false
>
> +#define MAX_CTL_NAMELEN 20
> +
> /*
> * PCI-defined configuration space registers
> */
> @@ -362,7 +364,7 @@ struct amd64_pvt {
> /* x4, x8, or x16 syndromes in use */
> u8 ecc_sym_sz;
>
> - const char *ctl_name;
> + char ctl_name[MAX_CTL_NAMELEN];
> u16 f1_id, f2_id;
> /* Maximum number of memory controllers per die/node. */
> u8 max_mcs;
> --
Thanks,
Yazen
Powered by blists - more mailing lists