Messages in this thread Patch in this message | | | Date | Tue, 20 Aug 2019 10:56:47 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection |
| |
On Mon, Jun 24, 2019 at 03:09:24PM +0000, Robert Richter wrote: > In a later patch we want to 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); > mem_info_prepare_mci(mci); > edac_mc_add_mc(mci); > }; > > This patch introduces mem_info_setup() and mem_info_prepare_mci().
Avoid having "This patch" or "This commit" in the commit message. It is tautologically useless.
> 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.
That sentence is not needed in the commit message.
> Signed-off-by: Robert Richter <rrichter@marvell.com> > --- > drivers/edac/ghes_edac.c | 166 ++++++++++++++++++++++++++++++--------- > 1 file changed, 127 insertions(+), 39 deletions(-) > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > index 8063996a311d..44bfb499b147 100644 > --- a/drivers/edac/ghes_edac.c > +++ b/drivers/edac/ghes_edac.c > @@ -65,17 +65,53 @@ struct memdev_dmi_entry { > u16 conf_mem_clk_speed; > } __attribute__((__packed__)); > > -struct ghes_edac_dimm_fill { > - struct mem_ctl_info *mci; > - unsigned count;
All those "dimm" and "info" words everywhere are making my head spin. Let's make it more readable:
> +struct ghes_dimm_info { > + struct dimm_info dimm_info; > + int idx; > +}; > + > +struct ghes_mem_info { > + int num_dimm; > + struct ghes_dimm_info *dimms; > }; > > +static struct ghes_mem_info mem_info;
/* A DIMM */ struct ghes_dimm { struct dimm_info dimm; int idx; };
/* The memory layout of the system */ struct ghes_memory { struct ghes_dimm *dimms; int num_dimms; };
static struct ghes_memory mem;
> + > +#define for_each_dimm(dimm) \ > + for (dimm = mem_info.dimms; \ > + dimm < mem_info.dimms + mem_info.num_dimm; \ > + dimm++) > + > static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg) > { > - int *num_dimm = arg; > + int *num = arg; > > if (dh->type == DMI_ENTRY_MEM_DEVICE) > - (*num_dimm)++; > + (*num)++; > +} > + > +static int ghes_dimm_info_init(int num)
ghes_dimm_init()
... you get the idea - let's drop the _info crap.
> +{ > + struct ghes_dimm_info *dimm; > + int idx = 0; > + > + memset(&mem_info, 0, sizeof(mem_info)); > + > + if (num <= 0) > + return -EINVAL;
Move that check into the caller mem_info_setup() so that you don't do the memset unnecessarily.
> + > + mem_info.dimms = kcalloc(num, sizeof(*mem_info.dimms), GFP_KERNEL); > + if (!mem_info.dimms) > + return -ENOMEM; > + > + mem_info.num_dimm = num; > + > + for_each_dimm(dimm) { > + dimm->idx = idx; > + idx++; > + }
or simply
for_each_dimm(dimm) dimm->idx = idx++; > + > + return 0; > } > > static int get_dimm_smbios_index(u16 handle)
-- Regards/Gruss, Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
| |