linux-kernel - Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a42a7696-0150-6d63-953e-adec19e25d02@oracle.com>
Date:   Thu, 3 Feb 2022 14:19:19 +0100
From:   William Roche <william.roche@...cle.com>
To:     Yazen Ghannam <yazen.ghannam@....com>, linux-edac@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, bp@...en8.de, mchehab@...nel.org,
        tony.luck@...el.com, james.morse@....com, rric@...nel.org,
        Smita.KoralahalliChannabasappa@....com
Subject: Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM

As we are moving the dram_type cached date from pvt to umc for family >= 
0x17, should we also add a small comment for the dram_type field in the 
amd64_pvt structure to indicate that ?

Something like that for example:

@@ -385,7 +385,7 @@
      /* place to store error injection parameters prior to issue */
      struct error_injection injection;

-    /* cache the dram_type */
+    /* cache the dram_type for family<0x17 */
      enum mem_type dram_type;

      struct amd64_umc *umc;    /* UMC registers */


Just a suggestion.
The code looks good to me.

Reviewed-by: William Roche <william.roche@...cle.com>

W.

On 02/02/2022 15:43, Yazen Ghannam wrote:
> Current AMD systems allow mixing of DIMM types within a system. However,
> DIMMs within a channel, i.e. managed by a single Unified Memory
> Controller (UMC), must be of the same type.
>
> Handle this possible configuration by checking and setting the memory
> type for each individual "UMC" structure.
>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> ---
> Link:
> https://lore.kernel.org/r/20211228200615.412999-2-yazen.ghannam@amd.com
>
> v3->v4:
> * Cache dram_type in struct umc.
>
> v2->v3:
> * Change patch to properly handle systems with different DIMM types.
>
> v1->v2:
> * Was patch 3 in v1.
> * Update commit message.
>
>   drivers/edac/amd64_edac.c | 47 +++++++++++++++++++++++++++++----------
>   drivers/edac/amd64_edac.h |  3 +++
>   2 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fba609ada0e6..49e384207ce0 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1429,7 +1429,7 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
>   		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
>   				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
>   
> -		if (pvt->dram_type == MEM_LRDDR4) {
> +		if (umc->dram_type == MEM_LRDDR4) {
>   			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
>   			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
>   					i, 1 << ((tmp >> 4) & 0x3));
> @@ -1616,19 +1616,40 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>   	}
>   }
>   
> +static void _determine_memory_type_df(struct amd64_umc *umc)
> +{
> +	if (!(umc->sdp_ctrl & UMC_SDP_INIT)) {
> +		umc->dram_type = MEM_EMPTY;
> +		return;
> +	}
> +
> +	if (umc->dimm_cfg & BIT(5))
> +		umc->dram_type = MEM_LRDDR4;
> +	else if (umc->dimm_cfg & BIT(4))
> +		umc->dram_type = MEM_RDDR4;
> +	else
> +		umc->dram_type = MEM_DDR4;
> +}
> +
> +static void determine_memory_type_df(struct amd64_pvt *pvt)
> +{
> +	struct amd64_umc *umc;
> +	u32 i;
> +
> +	for_each_umc(i) {
> +		umc = &pvt->umc[i];
> +
> +		_determine_memory_type_df(umc);
> +		edac_dbg(1, "  UMC%d DIMM type: %s\n", i, edac_mem_types[umc->dram_type]);
> +	}
> +}
> +
>   static void determine_memory_type(struct amd64_pvt *pvt)
>   {
>   	u32 dram_ctrl, dcsm;
>   
> -	if (pvt->umc) {
> -		if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
> -			pvt->dram_type = MEM_LRDDR4;
> -		else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
> -			pvt->dram_type = MEM_RDDR4;
> -		else
> -			pvt->dram_type = MEM_DDR4;
> -		return;
> -	}
> +	if (pvt->umc)
> +		return determine_memory_type_df(pvt);
>   
>   	switch (pvt->fam) {
>   	case 0xf:
> @@ -3452,7 +3473,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>   	read_dct_base_mask(pvt);
>   
>   	determine_memory_type(pvt);
> -	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> +
> +	if (!pvt->umc)
> +		edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
>   
>   	determine_ecc_sym_sz(pvt);
>   }
> @@ -3548,7 +3571,7 @@ static int init_csrows_df(struct mem_ctl_info *mci)
>   					pvt->mc_node_id, cs);
>   
>   			dimm->nr_pages = get_csrow_nr_pages(pvt, umc, cs);
> -			dimm->mtype = pvt->dram_type;
> +			dimm->mtype = pvt->umc[umc].dram_type;
>   			dimm->edac_mode = edac_mode;
>   			dimm->dtype = dev_type;
>   			dimm->grain = 64;
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 352bda9803f6..09ad28299c57 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -344,6 +344,9 @@ struct amd64_umc {
>   	u32 sdp_ctrl;		/* SDP Control reg */
>   	u32 ecc_ctrl;		/* DRAM ECC Control reg */
>   	u32 umc_cap_hi;		/* Capabilities High reg */
> +
> +	/* cache the dram_type */
> +	enum mem_type dram_type;
>   };
>   
>   struct amd64_pvt {