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

Yazen Ghannam
2022-03-28T16:17:09Z

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