Re: [PATCH] EDAC/amd64: Stop allocating ECC settings separately

[Yazen Ghannam <yazen.ghannam@amd.com>]

On 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