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: <8e44d499-ad55-d90a-d4e6-2a97aa6d0c41@akamai.com>
Date:   Tue, 29 Mar 2022 10:58:50 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     joshuahant@...il.com
Cc:     linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] EDAC/ie31200: Add Skylake-S support



On 3/22/22 14:47, joshuahant@...il.com wrote:
> From: Josh Hant <joshuahant@...il.com>
> 
> Add device IDs for Skylake-S CPUs according to datasheet.
> 
> Signed-off-by: Josh Hant <joshuahant@...il.com>
> ---
> Dear all,
> 
> I found that edac-util -v shows no memory controllers when using an
> Intel i5-6100T with a Supermicro X11SAE motherboard. With this patch,
> the ECC memory is detected. I tried to follow previous patches
> that added new families of processors to the module.
> 
> This is my first submission to the kernel so please let me know if I
> missed something in the process.
> 
> Thanks,
> Josh Hant
> 


Hi Josh,

Thanks for your submission! Some comments below.

>  drivers/edac/ie31200_edac.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
> index 9a9ff5ad611a..96a3f70d06e6 100644
> --- a/drivers/edac/ie31200_edac.c
> +++ b/drivers/edac/ie31200_edac.c
> @@ -20,11 +20,14 @@
>   * 0c08: Xeon E3-1200 v3 Processor DRAM Controller
>   * 1918: Xeon E3-1200 v5 Skylake Host Bridge/DRAM Registers
>   * 5918: Xeon E3-1200 Xeon E3-1200 v6/7th Gen Core Processor Host Bridge/DRAM Registers
> + * 190f: 6th Gen Core Dual-Core Processor Host Bridge/DRAM Registers
> + * 191f: 6th Gen Core Quad-Core Processor Host Bridge/DRAM Registers
>   * 3e..: 8th/9th Gen Core Processor Host Bridge/DRAM Registers
>   *
>   * Based on Intel specification:
>   * https://urldefense.com/v3/__https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e3-1200v3-vol-2-datasheet.pdf__;!!GjvTz_vk!AMVZ8h7gNGrptmwFk4tdnr0n6gUXJhZWRRjaZ3_usWEhORCh3fhbT-HVOitdUQ$ 
>   * https://urldefense.com/v3/__http://www.intel.com/content/www/us/en/processors/xeon/xeon-e3-1200-family-vol-2-datasheet.html__;!!GjvTz_vk!AMVZ8h7gNGrptmwFk4tdnr0n6gUXJhZWRRjaZ3_usWEhORCh3fhbT-HdDfO4eA$ 
> + * https://urldefense.com/v3/__https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/desktop-6th-gen-core-family-datasheet-vol-2.pdf__;!!GjvTz_vk!AMVZ8h7gNGrptmwFk4tdnr0n6gUXJhZWRRjaZ3_usWEhORCh3fhbT-Ef_03qbw$ 


I didn't find register #s in the above doc. But this one seems to have them:

https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e3-1200v6-vol-2-datasheet.pdf


>   * https://urldefense.com/v3/__https://www.intel.com/content/www/us/en/processors/core/7th-gen-core-family-mobile-h-processor-lines-datasheet-vol-2.html__;!!GjvTz_vk!AMVZ8h7gNGrptmwFk4tdnr0n6gUXJhZWRRjaZ3_usWEhORCh3fhbT-EznD3gEQ$ 
>   * https://urldefense.com/v3/__https://www.intel.com/content/www/us/en/products/docs/processors/core/8th-gen-core-family-datasheet-vol-2.html__;!!GjvTz_vk!AMVZ8h7gNGrptmwFk4tdnr0n6gUXJhZWRRjaZ3_usWEhORCh3fhbT-FmC7_jRg$ 
>   *
> @@ -53,15 +56,17 @@
>  #define ie31200_printk(level, fmt, arg...) \
>  	edac_printk(level, "ie31200", fmt, ##arg)
> 
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_1 0x0108
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_2 0x010c
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_3 0x0150
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_4 0x0158
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_5 0x015c
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_6 0x0c04
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_7 0x0c08
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_8 0x1918
> -#define PCI_DEVICE_ID_INTEL_IE31200_HB_9 0x5918
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_1  0x0108
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_2  0x010c
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_3  0x0150
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_4  0x0158
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_5  0x015c
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_6  0x0c04
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_7  0x0c08
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_8  0x190F
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_9  0x1918
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_10 0x191F
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_11 0x5918
> 
>  /* Coffee Lake-S */
>  #define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK 0x3e00
> @@ -80,6 +85,7 @@
>  #define DEVICE_ID_SKYLAKE_OR_LATER(did)                                        \
>  	(((did) == PCI_DEVICE_ID_INTEL_IE31200_HB_8) ||                        \
>  	 ((did) == PCI_DEVICE_ID_INTEL_IE31200_HB_9) ||                        \
> +	 ((did) == PCI_DEVICE_ID_INTEL_IE31200_HB_10) ||                       \
>  	 (((did) & PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK) ==                 \
>  	  PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK))

Looks like PCI_DEVICE_ID_INTEL_IE31200_HB_11 is missing from this check? IE you added
two new device ids, but added only 1 new check?

> 
> @@ -577,6 +583,8 @@ static const struct pci_device_id ie31200_pci_tbl[] = {
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_7),      PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_8),      PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_9),      PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_10),     PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_11),     PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_1),  PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_2),  PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
>  	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_3),  PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
> --
> 2.34.1
> 


Also, just to confirm that after this patch the edac memory info/size is consistent with
what's in /proc/meminfo, dmidecode -t memory?

Thanks,

-Jason

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ