| Message ID | 20200422115814.22205-4-rrichter@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/mc/ghes: Fixes, cleanup and reworks | expand |
On Wed, Apr 22, 2020 at 01:58:07PM +0200, Robert Richter wrote: > The struct members list and ghes of struct ghes_edac_pvt are unused, > remove them. On that occasion, rename it to struct ghes_mci. This is > shorter and aligns better with the current naming scheme. > > Signed-off-by: Robert Richter <rrichter@marvell.com> > --- > drivers/edac/ghes_edac.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > index cb3dab56a875..39efce0df881 100644 > --- a/drivers/edac/ghes_edac.c > +++ b/drivers/edac/ghes_edac.c > @@ -15,9 +15,7 @@ > #include "edac_module.h" > #include <ras/ras_event.h> > > -struct ghes_edac_pvt { > - struct list_head list; > - struct ghes *ghes; > +struct ghes_mci { No, that should be "ghes_pvt" because it *is* ghes_edac's private structure and there's also an mci pointer in it. > struct mem_ctl_info *mci; > > /* Buffers for the error handling routine */ > @@ -32,7 +30,7 @@ static refcount_t ghes_refcount = REFCOUNT_INIT(0); > * also provides the necessary (implicit) memory barrier for the SMP > * case to make the pointer visible on another CPU. > */ > -static struct ghes_edac_pvt *ghes_pvt; > +static struct ghes_mci *ghes_pvt; > > /* GHES registration mutex */ > static DEFINE_MUTEX(ghes_reg_mutex); > @@ -203,7 +201,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err) > { > struct edac_raw_error_desc *e; > struct mem_ctl_info *mci; > - struct ghes_edac_pvt *pvt; > + struct ghes_mci *pvt; > unsigned long flags; > char *p; > > @@ -457,7 +455,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) > bool fake = false; > int rc = 0, num_dimm = 0; > struct mem_ctl_info *mci; > - struct ghes_edac_pvt *pvt; > + struct ghes_mci *pvt; > struct edac_mc_layer layers[1]; > struct ghes_edac_dimm_fill dimm_fill; > unsigned long flags; > @@ -494,7 +492,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) > layers[0].size = num_dimm; > layers[0].is_virt_csrow = true; > > - mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt)); > + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*pvt)); The sizeof() change doesn't make it better because now I have to go look up what pvt is. sizeof(struct ghes_pvt) tells you what size you're getting here. Thx.
On 23.04.20 19:55:17, Borislav Petkov wrote: > On Wed, Apr 22, 2020 at 01:58:07PM +0200, Robert Richter wrote: > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > > index cb3dab56a875..39efce0df881 100644 > > --- a/drivers/edac/ghes_edac.c > > +++ b/drivers/edac/ghes_edac.c > > @@ -15,9 +15,7 @@ > > #include "edac_module.h" > > #include <ras/ras_event.h> > > > > -struct ghes_edac_pvt { > > - struct list_head list; > > - struct ghes *ghes; > > +struct ghes_mci { > > No, that should be "ghes_pvt" because it *is* ghes_edac's private > structure and there's also an mci pointer in it. The ghes driver will use private data for both structs, mci and dimm_info. Thus I named it ghes_mci and ghes_dimm (see next patch) as they are counterparts. I could name it "ghes_pvt", but the meaning would be less obvious. Same for your suggestion in the next patch, struct dimm is too general and could cause namespace conflicts with other code (think of cscope etc.). -Robert
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index cb3dab56a875..39efce0df881 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -15,9 +15,7 @@ #include "edac_module.h" #include <ras/ras_event.h> -struct ghes_edac_pvt { - struct list_head list; - struct ghes *ghes; +struct ghes_mci { struct mem_ctl_info *mci; /* Buffers for the error handling routine */ @@ -32,7 +30,7 @@ static refcount_t ghes_refcount = REFCOUNT_INIT(0); * also provides the necessary (implicit) memory barrier for the SMP * case to make the pointer visible on another CPU. */ -static struct ghes_edac_pvt *ghes_pvt; +static struct ghes_mci *ghes_pvt; /* GHES registration mutex */ static DEFINE_MUTEX(ghes_reg_mutex); @@ -203,7 +201,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err) { struct edac_raw_error_desc *e; struct mem_ctl_info *mci; - struct ghes_edac_pvt *pvt; + struct ghes_mci *pvt; unsigned long flags; char *p; @@ -457,7 +455,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) bool fake = false; int rc = 0, num_dimm = 0; struct mem_ctl_info *mci; - struct ghes_edac_pvt *pvt; + struct ghes_mci *pvt; struct edac_mc_layer layers[1]; struct ghes_edac_dimm_fill dimm_fill; unsigned long flags; @@ -494,7 +492,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) layers[0].size = num_dimm; layers[0].is_virt_csrow = true; - mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt)); + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*pvt)); if (!mci) { pr_info("Can't allocate memory for EDAC data\n"); rc = -ENOMEM; @@ -502,7 +500,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) } pvt = mci->pvt_info; - pvt->ghes = ghes; pvt->mci = mci; mci->pdev = dev;
The struct members list and ghes of struct ghes_edac_pvt are unused, remove them. On that occasion, rename it to struct ghes_mci. This is shorter and aligns better with the current naming scheme. Signed-off-by: Robert Richter <rrichter@marvell.com> --- drivers/edac/ghes_edac.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)