Re: [PATCH 07/14] EDAC/amd64: Add ecc_enabled() into pvt->ops

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

On Mon, Feb 28, 2022 at 09:43:47PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for ecc_enabled() in pvt->ops and assign
> family specific ecc_enabled() definitions appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 77 ++++++++++++++++++++++++---------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 713ffe763e64..15d775a9ce7e 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3649,49 +3649,60 @@ static void restore_ecc_error_reporting(struct ecc_settings *s, u16 nid,
>  		amd64_warn("Error restoring NB MCGCTL settings!\n");
>  }
>  
> -static bool ecc_enabled(struct amd64_pvt *pvt)
> +static bool f1x_ecc_enabled(struct amd64_pvt *pvt)
>  {
>  	u16 nid = pvt->mc_node_id;
>  	bool nb_mce_en = false;
> -	u8 ecc_en = 0, i;
> +	u8 ecc_en = 0;
>  	u32 value;
>  
> -	if (boot_cpu_data.x86 >= 0x17) {
> -		u8 umc_en_mask = 0, ecc_en_mask = 0;
> -		struct amd64_umc *umc;
> +	amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
>  
> -		for_each_umc(i) {
> -			umc = &pvt->umc[i];
> +	ecc_en = !!(value & NBCFG_ECC_ENABLE);
>  
> -			/* Only check enabled UMCs. */
> -			if (!(umc->sdp_ctrl & UMC_SDP_INIT))
> -				continue;
> +	nb_mce_en = nb_mce_bank_enabled_on_node(nid);
> +	if (!nb_mce_en)
> +		edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
> +			 MSR_IA32_MCG_CTL, nid);
>  
> -			umc_en_mask |= BIT(i);
> +	edac_dbg(3, "Node %d: DRAM ECC %s.\n", nid, (ecc_en ? "enabled" : "disabled"));
>  
> -			if (umc->umc_cap_hi & UMC_ECC_ENABLED)
> -				ecc_en_mask |= BIT(i);
> -		}
> +	if (!ecc_en || !nb_mce_en)
> +		return false;
> +	else
> +		return true;
> +}
>  
> -		/* Check whether at least one UMC is enabled: */
> -		if (umc_en_mask)
> -			ecc_en = umc_en_mask == ecc_en_mask;
> -		else
> -			edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
> +static bool f17_ecc_enabled(struct amd64_pvt *pvt)
> +{
> +	u8 umc_en_mask = 0, ecc_en_mask = 0;
> +	u8 ecc_en = 0, i;

This line should go at the end to keep the longest->shortest line style.

> +	u16 nid = pvt->mc_node_id;
> +	bool nb_mce_en = false;
> +	struct amd64_umc *umc;
>  
> -		/* Assume UMC MCA banks are enabled. */
> -		nb_mce_en = true;
> -	} else {
> -		amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
> +	for_each_umc(i) {
> +		umc = &pvt->umc[i];
> +
> +		/* Only check enabled UMCs. */
> +		if (!(umc->sdp_ctrl & UMC_SDP_INIT))
> +			continue;
>  
> -		ecc_en = !!(value & NBCFG_ECC_ENABLE);
> +		umc_en_mask |= BIT(i);
>  
> -		nb_mce_en = nb_mce_bank_enabled_on_node(nid);
> -		if (!nb_mce_en)
> -			edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
> -				     MSR_IA32_MCG_CTL, nid);
> +		if (umc->umc_cap_hi & UMC_ECC_ENABLED)
> +			ecc_en_mask |= BIT(i);
>  	}
>  
> +	/* Check whether at least one UMC is enabled: */
> +	if (umc_en_mask)
> +		ecc_en = umc_en_mask == ecc_en_mask;
> +	else
> +		edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
> +
> +	/* Assume UMC MCA banks are enabled. */
> +	nb_mce_en = true;
> +

The nb_mce_en variable can be dropped since this is now a separate function.

Thanks,
Yazen