EDAC/amd64: Stop allocating ECC settings separately
Subsystems (2)
PATCH 1/1 EDAC/amd64: Stop allocating ECC settings separately
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.
---
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/[email protected]/
drivers/edac/amd64_edac.c | 40 ++++++--------------------------------- drivers/edac/amd64_edac.h | 27 ++++++++++++++------------ 2 files changed, 21 insertions(+), 46 deletions(-)
drivers/edac/amd64_edac.c +6 -34
@@ -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 ; }
drivers/edac/amd64_edac.h +15 -12
@@ -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
Signature · 5 lines
2.55.0.1082.g2b9226bbc0-goog -- Dmitry
Quoted 107 lines · Dmitry Torokhov
There is no reason to allocate and manage ECC settings separately when struct
ecc_settingscan be embedded directly into the driver-private structamd64_pvt.Signed-off-by Dmitry Torokhov <[email protected]>---
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_edachas long moved away frompci_register_driver()to probing nodes directly inamd64_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/[email protected]/
drivers/edac/amd64_edac.c | 40 ++++++--------------------------------- drivers/edac/amd64_edac.h | 27 ++++++++++++++------------ 2 files changed, 21 insertions(+), 46 deletions(-)drivers/edac/amd64_edac.c +4
-22@@ -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 ();
drivers/edac/amd64_edac.c static int __init amd64_edac_init(void) · line 4137
Quoted 10 lines
drivers/edac/amd64_edac.c +1
-4err = -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;'.
Quoted 16 lines
drivers/edac/amd64_edac.c +1
-4for (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; }
drivers/edac/amd64_edac.c static void __exit amd64_edac_exit(void) · line 4225
Quoted 8 lines
drivers/edac/amd64_edac.c +0
-4@@ -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.
Quoted 8 lines
drivers/edac/amd64_edac.c +0
-0msrs_free (msrs); msrs =NULL ; }drivers/edac/amd64_edac.h +0
-0
drivers/edac/amd64_edac.h struct amd64_family_flags { · line 327
Quoted 6 lines
drivers/edac/amd64_edac.h +3
-0@@ -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.
Quoted 32 lines
drivers/edac/amd64_edac.h +12
-5+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;
drivers/edac/amd64_edac.h static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt) · line 461
Quoted 10 lines
drivers/edac/amd64_edac.h +0
-7-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.
Thanks,
Yazen