drivers/edac/amd64_edac.c | 40 ++++++--------------------------------- drivers/edac/amd64_edac.h | 27 ++++++++++++++------------ 2 files changed, 21 insertions(+), 46 deletions(-)
There is no reason to allocate and manage ECC settings separately when
struct ecc_settings can be embedded directly into the driver-private
struct amd64_pvt.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Resending this standalone cleanup that was originally posted back in
2015 as part of an async-probe RFC series [1] and got lost in the
broader driver model discussion. Since amd64_edac has long moved away
from pci_register_driver() to probing nodes directly in
amd64_edac_init(), patches 2 and 3 from that series are obsolete, and
this patch has been rebased onto the current code.
[1] https://lore.kernel.org/all/1426726150-983-2-git-send-email-dmitry.torokhov@gmail.com/
drivers/edac/amd64_edac.c | 40 ++++++---------------------------------
drivers/edac/amd64_edac.h | 27 ++++++++++++++------------
2 files changed, 21 insertions(+), 46 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 475235c402e8..6931b7e61652 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -30,9 +30,6 @@ static inline u32 get_umc_reg(struct amd64_pvt *pvt, u32 reg)
return 0;
}
-/* Per-node stuff */
-static struct ecc_settings **ecc_stngs;
-
/* Device for the PCI component */
static struct device *pci_ctl_dev;
@@ -3999,19 +3996,12 @@ static int probe_one_instance(unsigned int nid)
{
struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
struct amd64_pvt *pvt = NULL;
- struct ecc_settings *s;
int ret;
ret = -ENOMEM;
- s = kzalloc_obj(struct ecc_settings);
- if (!s)
- goto err_out;
-
- ecc_stngs[nid] = s;
-
pvt = kzalloc_obj(struct amd64_pvt);
if (!pvt)
- goto err_settings;
+ goto err_out;
pvt->mc_node_id = nid;
pvt->F3 = F3;
@@ -4042,7 +4032,7 @@ static int probe_one_instance(unsigned int nid)
} else
amd64_warn("Forcing ECC on!\n");
- if (!enable_ecc_error_reporting(s, nid, F3))
+ if (!enable_ecc_error_reporting(&pvt->ecc, nid, F3))
goto err_enable;
}
@@ -4051,7 +4041,7 @@ static int probe_one_instance(unsigned int nid)
amd64_err("Error probing instance: %d\n", nid);
if (boot_cpu_data.x86 < 0x17)
- restore_ecc_error_reporting(s, nid, F3);
+ restore_ecc_error_reporting(&pvt->ecc, nid, F3);
goto err_enable;
}
@@ -4067,10 +4057,6 @@ static int probe_one_instance(unsigned int nid)
hw_info_put(pvt);
kfree(pvt);
-err_settings:
- kfree(s);
- ecc_stngs[nid] = NULL;
-
err_out:
return ret;
}
@@ -4078,7 +4064,6 @@ static int probe_one_instance(unsigned int nid)
static void remove_one_instance(unsigned int nid)
{
struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
- struct ecc_settings *s = ecc_stngs[nid];
struct mem_ctl_info *mci;
struct amd64_pvt *pvt;
@@ -4089,10 +4074,7 @@ static void remove_one_instance(unsigned int nid)
pvt = mci->pvt_info;
- restore_ecc_error_reporting(s, nid, F3);
-
- kfree(ecc_stngs[nid]);
- ecc_stngs[nid] = NULL;
+ restore_ecc_error_reporting(&pvt->ecc, nid, F3);
/* Free the EDAC CORE resources */
mci->pvt_info = NULL;
@@ -4149,13 +4131,10 @@ static int __init amd64_edac_init(void)
opstate_init();
err = -ENOMEM;
- ecc_stngs = kzalloc_objs(ecc_stngs[0], amd_nb_num());
- if (!ecc_stngs)
- goto err_free;
msrs = msrs_alloc();
if (!msrs)
- goto err_free;
+ goto err_ret;
for (i = 0; i < amd_nb_num(); i++) {
err = probe_one_instance(i);
@@ -4195,10 +4174,7 @@ static int __init amd64_edac_init(void)
msrs_free(msrs);
msrs = NULL;
-err_free:
- kfree(ecc_stngs);
- ecc_stngs = NULL;
-
+err_ret:
return err;
}
@@ -4218,11 +4194,7 @@ static void __exit amd64_edac_exit(void)
for (i = 0; i < amd_nb_num(); i++)
remove_one_instance(i);
- kfree(ecc_stngs);
- ecc_stngs = NULL;
-
pci_ctl_dev = NULL;
-
msrs_free(msrs);
msrs = NULL;
}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 1757c1b99fc8..a93e0214143c 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -322,6 +322,19 @@ struct amd64_family_flags {
__reserved : 63;
};
+/*
+ * per-node ECC settings descriptor
+ */
+struct ecc_settings {
+ u32 old_nbctl;
+ bool nbctl_valid;
+
+ struct flags {
+ unsigned long nb_mce_enable:1;
+ unsigned long nb_ecc_prev:1;
+ } flags;
+};
+
struct amd64_pvt {
struct low_ops *ops;
@@ -372,6 +385,8 @@ struct amd64_pvt {
/* place to store error injection parameters prior to issue */
struct error_injection injection;
+ struct ecc_settings ecc;
+
/*
* cache the dram_type
*
@@ -441,18 +456,6 @@ static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
return ((pvt)->dct_sel_lo >> 6) & 0x3;
}
-/*
- * per-node ECC settings descriptor
- */
-struct ecc_settings {
- u32 old_nbctl;
- bool nbctl_valid;
-
- struct flags {
- unsigned long nb_mce_enable:1;
- unsigned long nb_ecc_prev:1;
- } flags;
-};
/*
* Each of the PCI Device IDs types have their own set of hardware accessor
--
2.55.0.1082.g2b9226bbc0-goog
--
DmitryOn Sun, Sep 20, 2026 at 07:10:13PM -0700, Dmitry Torokhov wrote:
> There is no reason to allocate and manage ECC settings separately when
> struct ecc_settings can be embedded directly into the driver-private
> struct amd64_pvt.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> Resending this standalone cleanup that was originally posted back in
> 2015 as part of an async-probe RFC series [1] and got lost in the
> broader driver model discussion. Since amd64_edac has long moved away
> from pci_register_driver() to probing nodes directly in
> amd64_edac_init(), patches 2 and 3 from that series are obsolete, and
> this patch has been rebased onto the current code.
>
> [1] https://lore.kernel.org/all/1426726150-983-2-git-send-email-dmitry.torokhov@gmail.com/
>
>
> drivers/edac/amd64_edac.c | 40 ++++++---------------------------------
> drivers/edac/amd64_edac.h | 27 ++++++++++++++------------
> 2 files changed, 21 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 475235c402e8..6931b7e61652 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -30,9 +30,6 @@ static inline u32 get_umc_reg(struct amd64_pvt *pvt, u32 reg)
> return 0;
> }
>
> -/* Per-node stuff */
> -static struct ecc_settings **ecc_stngs;
> -
> /* Device for the PCI component */
> static struct device *pci_ctl_dev;
>
> @@ -3999,19 +3996,12 @@ static int probe_one_instance(unsigned int nid)
> {
> struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
> struct amd64_pvt *pvt = NULL;
> - struct ecc_settings *s;
> int ret;
>
> ret = -ENOMEM;
> - s = kzalloc_obj(struct ecc_settings);
> - if (!s)
> - goto err_out;
> -
> - ecc_stngs[nid] = s;
> -
> pvt = kzalloc_obj(struct amd64_pvt);
> if (!pvt)
> - goto err_settings;
> + goto err_out;
>
> pvt->mc_node_id = nid;
> pvt->F3 = F3;
> @@ -4042,7 +4032,7 @@ static int probe_one_instance(unsigned int nid)
> } else
> amd64_warn("Forcing ECC on!\n");
>
> - if (!enable_ecc_error_reporting(s, nid, F3))
> + if (!enable_ecc_error_reporting(&pvt->ecc, nid, F3))
> goto err_enable;
> }
>
> @@ -4051,7 +4041,7 @@ static int probe_one_instance(unsigned int nid)
> amd64_err("Error probing instance: %d\n", nid);
>
> if (boot_cpu_data.x86 < 0x17)
> - restore_ecc_error_reporting(s, nid, F3);
> + restore_ecc_error_reporting(&pvt->ecc, nid, F3);
>
> goto err_enable;
> }
> @@ -4067,10 +4057,6 @@ static int probe_one_instance(unsigned int nid)
> hw_info_put(pvt);
> kfree(pvt);
>
> -err_settings:
> - kfree(s);
> - ecc_stngs[nid] = NULL;
> -
> err_out:
> return ret;
> }
> @@ -4078,7 +4064,6 @@ static int probe_one_instance(unsigned int nid)
> static void remove_one_instance(unsigned int nid)
> {
> struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
> - struct ecc_settings *s = ecc_stngs[nid];
> struct mem_ctl_info *mci;
> struct amd64_pvt *pvt;
>
> @@ -4089,10 +4074,7 @@ static void remove_one_instance(unsigned int nid)
>
> pvt = mci->pvt_info;
>
> - restore_ecc_error_reporting(s, nid, F3);
> -
> - kfree(ecc_stngs[nid]);
> - ecc_stngs[nid] = NULL;
> + restore_ecc_error_reporting(&pvt->ecc, nid, F3);
>
> /* Free the EDAC CORE resources */
> mci->pvt_info = NULL;
> @@ -4149,13 +4131,10 @@ static int __init amd64_edac_init(void)
> opstate_init();
>
> err = -ENOMEM;
> - ecc_stngs = kzalloc_objs(ecc_stngs[0], amd_nb_num());
> - if (!ecc_stngs)
> - goto err_free;
>
> msrs = msrs_alloc();
> if (!msrs)
> - goto err_free;
> + goto err_ret;
This can just 'return -ENOMEM;'.
>
> for (i = 0; i < amd_nb_num(); i++) {
> err = probe_one_instance(i);
> @@ -4195,10 +4174,7 @@ static int __init amd64_edac_init(void)
> msrs_free(msrs);
> msrs = NULL;
>
> -err_free:
> - kfree(ecc_stngs);
> - ecc_stngs = NULL;
> -
> +err_ret:
> return err;
> }
>
> @@ -4218,11 +4194,7 @@ static void __exit amd64_edac_exit(void)
> for (i = 0; i < amd_nb_num(); i++)
> remove_one_instance(i);
>
> - kfree(ecc_stngs);
> - ecc_stngs = NULL;
> -
> pci_ctl_dev = NULL;
> -
This removal isn't needed.
> msrs_free(msrs);
> msrs = NULL;
> }
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 1757c1b99fc8..a93e0214143c 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -322,6 +322,19 @@ struct amd64_family_flags {
> __reserved : 63;
> };
>
> +/*
> + * per-node ECC settings descriptor
> + */
I understand this was a direct move, but it can just be a single-line
comment.
> +struct ecc_settings {
> + u32 old_nbctl;
> + bool nbctl_valid;
> +
> + struct flags {
> + unsigned long nb_mce_enable:1;
> + unsigned long nb_ecc_prev:1;
> + } flags;
> +};
> +
> struct amd64_pvt {
> struct low_ops *ops;
>
> @@ -372,6 +385,8 @@ struct amd64_pvt {
> /* place to store error injection parameters prior to issue */
> struct error_injection injection;
>
> + struct ecc_settings ecc;
> +
> /*
> * cache the dram_type
> *
> @@ -441,18 +456,6 @@ static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
>
> return ((pvt)->dct_sel_lo >> 6) & 0x3;
> }
> -/*
> - * per-node ECC settings descriptor
> - */
> -struct ecc_settings {
> - u32 old_nbctl;
> - bool nbctl_valid;
> -
> - struct flags {
> - unsigned long nb_mce_enable:1;
> - unsigned long nb_ecc_prev:1;
> - } flags;
> -};
>
> /*
> * Each of the PCI Device IDs types have their own set of hardware accessor
> --
Besides the minor issues, this looks good to me.
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Thanks,
Yazen
© 2016 - 2026 Red Hat, Inc.