[v2,03/10] EDAC/ghes: Remove unused members of struct ghes_edac_pvt, rename it to ghes_mci
diff mbox series

Message ID 20200422115814.22205-4-rrichter@marvell.com
State New
Headers show
Series
  • EDAC/mc/ghes: Fixes, cleanup and reworks
Related show

Commit Message

Robert Richter April 22, 2020, 11:58 a.m. UTC
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(-)

Comments

Borislav Petkov April 23, 2020, 5:55 p.m. UTC | #1
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.
Robert Richter May 5, 2020, 7:50 a.m. UTC | #2
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

Patch
diff mbox series

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;