Hi Tony,

On Mon, 12 Mar 2018 11:24:30 -0700, Tony Luck wrote:
> This just covers the topology function of the EDAC driver.
> We locate which DIMM slots are populated with NVDIMMs and
> query the NFIT and SMBIOS tables to get the size.
> 
> Signed-off-by: Tony Luck <tony.l...@intel.com>
> ---
>  drivers/edac/Kconfig    |  5 +++-
>  drivers/edac/skx_edac.c | 66 
> ++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 64 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 3c4017007647..c12e34564557 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -232,9 +232,12 @@ config EDAC_SBRIDGE
>  config EDAC_SKX
>       tristate "Intel Skylake server Integrated MC"
>       depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
> +     select DMI
>       help
>         Support for error detection and correction the Intel
> -       Skylake server Integrated Memory Controllers.
> +       Skylake server Integrated Memory Controllers. If your
> +       system has non-volatile DIMMs you should also manually
> +       select CONFIG_ACPI_NFIT

Nitpicking: trailing dot is missing.

>  
>  config EDAC_PND2
>       tristate "Intel Pondicherry2"
> diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
> index 912c4930c9ef..84c18bb1e0cd 100644
> --- a/drivers/edac/skx_edac.c
> +++ b/drivers/edac/skx_edac.c
> (...)
> +static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
> +                        int chan, int dimmno)
> +{
> +     int smbios_handle;
> +     u32 dev_handle;
> +     u16 flags;
> +     u64 size = 0;
> +
> +     dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
> +                                                imc->src_id, 0);
> +
> +     smbios_handle = nfit_get_smbios_id(dev_handle, &flags);
> +     if (smbios_handle == -EOPNOTSUPP) {
> +             pr_warn_once("skx_edac: can't find size of NVDIMM. Try enabling 
> CONFIG_ACPI_NFIT\n");
> +             goto unknown_size;
> +     }
> +     if (smbios_handle < 0) {
> +             skx_printk(KERN_ERR, "Can't find handle for NVDIMM ADR=%x\n", 
> dev_handle);
> +             goto unknown_size;
> +     }
> +
> +     if (flags & ACPI_NFIT_MEM_MAP_FAILED) {
> +             skx_printk(KERN_ERR, "NVDIMM ADR=%x is not mapped\n", 
> dev_handle);
> +             goto unknown_size;
> +     }
> +
> +     size = dmi_memdev_size(smbios_handle);
> +     if (size == ~0ull)
> +             skx_printk(KERN_ERR, "Can't find size for NVDIMM 
> ADR=%x/SMBIOS=%x\n",
> +                        dev_handle, smbios_handle);
> +
> +unknown_size:
> +     dimm->nr_pages = size >> PAGE_SHIFT;
> +     dimm->grain = 32;
> +     dimm->dtype = DEV_UNKNOWN;
> +     dimm->mtype = MEM_NVDIMM;
> +     dimm->edac_mode = EDAC_SECDED; /* likely better than this */
> +
> +     edac_dbg(0, "mc#%d: channel %d, dimm %d, %llu Mb (%u pages)\n",

I did not notice on previous review, but I think "b" in general means
bit not byte, so "MB" would be better.

> +              imc->mc, chan, dimmno, size >> 20, dimm->nr_pages);
> +
> +     snprintf(dimm->label, sizeof(dimm->label), 
> "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u",
> +              imc->src_id, imc->lmc, chan, dimmno);
> +
> +     return 1;
> +}

Now this function always return 1, that doesn't make a lot of sense?

Other than these details, the dmi-related code looks good to me now.

Reviewed-by: Jean Delvare <jdelv...@suse.de> # for DMI

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Reply via email to