Hi Robert,
On 29/05/2019 09:44, Robert Richter wrote:
> In a later patch we want add more information about the memory
> hierarchy (NUMA topology, DIMM label information). Rework memory
> hierarchy detection to make the code extendable for this.
>
> The general approach is roughly like:
>
> mem_info_setup();
> for_each_node(nid) {
> mci = edac_mc_alloc(nid);
> mci_add_dimm_info(mci);
> edac_mc_add_mc(mci);
> };
>
> This patch introduces mem_info_setup() and mci_add_dimm_info().
>
> All data of the memory hierarchy is collected in a local struct
> ghes_mem_info.
>
> Note: Per (NUMA) node registration will be implemented in a later
> patch.
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index ea4d53043199..50f4ee36b755 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -67,17 +67,38 @@ struct memdev_dmi_entry {
> u16 conf_mem_clk_speed;
> } __attribute__((__packed__));
>
> -struct ghes_edac_dimm_fill {
> - struct mem_ctl_info *mci;
> - unsigned count;
> +struct ghes_dimm_info {
> + struct dimm_info dimm_info;
> + int idx;
> +};
> +struct ghes_mem_info {
> + int num_dimm;
> + struct ghes_dimm_info *dimms;
> };
>
> +struct ghes_mem_info mem_info;
static?
> @@ -94,18 +115,17 @@ static int get_dimm_smbios_index(u16 handle)
>
> static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
> {
> - struct ghes_edac_dimm_fill *dimm_fill = arg;
> - struct mem_ctl_info *mci = dimm_fill->mci;
> -
> if (dh->type == DMI_ENTRY_MEM_DEVICE) {
> + int *idx = arg;
> struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
> - struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count,
> - 0, 0);
> + struct ghes_dimm_info *mi = &mem_info.dimms[*idx];
> + struct dimm_info *dimm = &mi->dimm_info;
> u16 rdr_mask = BIT(7) | BIT(13);
> + mi->phys_handle = entry->phys_mem_array_handle;
Where did this come from, and what is it for?
...
Should this be in a later patch? (did you bisect build this?)
> if (entry->size == 0xffff) {
> - pr_info("Can't get DIMM%i size\n",
> - dimm_fill->count);
> + pr_info("Can't get DIMM%i size\n", mi->idx);
> dimm->nr_pages = MiB_TO_PAGES(32);/* Unknown */
> } else if (entry->size == 0x7fff) {
> dimm->nr_pages = MiB_TO_PAGES(entry->extended_size);
> +static int mem_info_setup(void)
> +{
> + int idx = 0;
> +
> + memset(&mem_info, 0, sizeof(mem_info));
Is this necessary? Isn't mem_info in the BSS, it will zero'd already.
> + /* Get the number of DIMMs */
> + dmi_walk(ghes_edac_count_dimms, NULL);
> + if (!mem_info.num_dimm)
> + return -EINVAL;
> + mem_info.dimms = kcalloc(mem_info.num_dimm,
> + sizeof(*mem_info.dimms), GFP_KERNEL);
> + if (!mem_info.dimms)
> + return -ENOMEM;
> + ghes_dimm_info_init();
Could you move the kcalloc() into ghes_dimm_info_init()? This would save you
having a
unnecessarily-different version in mem_info_setup_fake().
> + dmi_walk(ghes_edac_dmidecode, &idx);
> +
> + return 0;
> +}
> +static int mem_info_setup_fake(void)
> +{
> + struct ghes_dimm_info *ghes_dimm;
> + struct dimm_info *dimm;
> +
> + memset(&mem_info, 0, sizeof(mem_info));
Is this necessary? Its only been touched by mem_info_setup(), and you get in
here because
mem_info.num_dimm == 0...
> + ghes_dimm = kzalloc(sizeof(*mem_info.dimms), GFP_KERNEL);
> + if (!ghes_dimm)
> + return -ENOMEM;
This is common with mem_info_setup(). If ghes_dimm_info_init() read
mem_info.num_dimm and
did the rest, you'd avoid some duplication here.
> + mem_info.num_dimm = 1;
> + mem_info.dimms = ghes_dimm;
> +
> + ghes_dimm_info_init();
> +
> + dimm = &ghes_dimm->dimm_info;
> + dimm->nr_pages = 1;
> + dimm->grain = 128;
> + dimm->mtype = MEM_UNKNOWN;
> + dimm->dtype = DEV_UNKNOWN;
> + dimm->edac_mode = EDAC_SECDED;
> +
> + return 0;
> +}
> +static void mci_add_dimm_info(struct mem_ctl_info *mci)
(From the name I expected this to be in edac_mc.c)
> +{
> + struct dimm_info *mci_dimm, *dmi_dimm;
> + struct ghes_dimm_info *dimm;
> + int index = 0;
> +
> + for_each_dimm(dimm) {
> + dmi_dimm = &dimm->dimm_info;
> + mci_dimm = edac_get_dimm_by_index(mci, index);
> +
> + index++;
> + if (index > mci->tot_dimms)
> + break;
> +
> + mci_dimm->nr_pages = dmi_dimm->nr_pages;
> + mci_dimm->mtype = dmi_dimm->mtype;
> + mci_dimm->edac_mode = dmi_dimm->edac_mode;
> + mci_dimm->dtype = dmi_dimm->dtype;
> + mci_dimm->grain = dmi_dimm->grain;
> + mci_dimm->smbios_handle = dmi_dimm->smbios_handle;
> }
This isn't fun. I guess 'numa' is the reason for generating a shadow copy of
all this, and
then having to copy it into edac. But surely that isn't a problem unique to
ghes_edac.c?
Can't you add the nid, and any other properties to struct dimm_info? It already
has
smbios_handle, which is hardly useful to other drivers!
> + if (index != mci->tot_dimms)
> + pr_warn("Unexpected number of DIMMs: %d (exp. %d)\n",
> + index, mci->tot_dimms);
> }
> @@ -472,22 +566,24 @@ int ghes_edac_register(struct ghes *ghes, struct device
> *dev)
> mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct
> ghes_edac_pvt));
> if (!mci) {
> - pr_info("Can't allocate memory for EDAC data\n");
> + pr_err("Can't allocate memory for EDAC data\n");
Leftover debug?
kfree(mem_info.dimms); ?
> return -ENOMEM;
> }
>
> @@ -513,26 +609,14 @@ int ghes_edac_register(struct ghes *ghes, struct device
> *dev)
> - if (!fake) {
> - dimm_fill.count = 0;
> - dimm_fill.mci = mci;
> - dmi_walk(ghes_edac_dmidecode, &dimm_fill);
> - } else {
> - struct dimm_info *dimm = edac_get_dimm(mci, 0, 0, 0);
> -
> - dimm->nr_pages = 1;
> - dimm->grain = 128;
> - dimm->mtype = MEM_UNKNOWN;
> - dimm->dtype = DEV_UNKNOWN;
> - dimm->edac_mode = EDAC_SECDED;
> - }
> + mci_add_dimm_info(mci);
>
> rc = edac_mc_add_mc(mci);
> if (rc < 0) {
> - pr_info("Can't register at EDAC core\n");
> + pr_err("Can't register at EDAC core\n");
Leftover debug?
> edac_mc_free(mci);
kfree(mem_info.dimms); ?
> return -ENODEV;
> }
Thanks!
James
