diff mbox series

[05/19] EDAC, mc: Reduce indentation level in edac_mc_handle_error()

Message ID 20191010202418.25098-6-rrichter@marvell.com
State Superseded
Headers show
Series EDAC: Rework edac_mc and ghes drivers | expand

Commit Message

Robert Richter Oct. 10, 2019, 8:25 p.m. UTC 
Reduce the indentation level in edac_mc_handle_error() a bit by using
continue. No functional changes.

Signed-off-by: Robert Richter <rrichter@marvell.com>

---
 drivers/edac/edac_mc.c | 59 +++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

-- 
2.20.1

Comments

Joe Perches Oct. 10, 2019, 10:10 p.m. UTC | #1 
On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:
> Reduce the indentation level in edac_mc_handle_error() a bit by using

> continue. No functional changes.


Seems fine, but trivially below:

> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c

[]
> @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,

[]
> +		strcpy(p, dimm->label);

> +		p += strlen(p);

> +		*p = '\0';


This *p = '\0' is unnecessary as the strcpy already did that.
Robert Richter Oct. 11, 2019, 6:50 a.m. UTC | #2 
On 10.10.19 15:10:53, Joe Perches wrote:
> On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:

> > Reduce the indentation level in edac_mc_handle_error() a bit by using

> > continue. No functional changes.

> 

> Seems fine, but trivially below:

> 

> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c

> []

> > @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,

> []

> > +		strcpy(p, dimm->label);

> > +		p += strlen(p);

> > +		*p = '\0';

> 

> This *p = '\0' is unnecessary as the strcpy already did that.


Other changes than the indentation are not the aim of this patch.
However, on the occasion and if there won't be any objections I could
include this trivial change in the patch in my next version of the
series.

Thanks for review.

-Robert
Mauro Carvalho Chehab Oct. 11, 2019, 10:17 a.m. UTC | #3 
Em Thu, 10 Oct 2019 20:25:14 +0000
Robert Richter <rrichter@marvell.com> escreveu:

> Reduce the indentation level in edac_mc_handle_error() a bit by using

> continue. No functional changes.

> 

> Signed-off-by: Robert Richter <rrichter@marvell.com>


Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>


> ---

>  drivers/edac/edac_mc.c | 59 +++++++++++++++++++++---------------------

>  1 file changed, 30 insertions(+), 29 deletions(-)

> 

> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c

> index f2cbca77bc50..45b02bb31964 100644

> --- a/drivers/edac/edac_mc.c

> +++ b/drivers/edac/edac_mc.c

> @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,

>  		 * channel/memory controller/...  may be affected.

>  		 * Also, don't show errors for empty DIMM slots.

>  		 */

> -		if (e->enable_per_layer_report && dimm->nr_pages) {

> -			if (n_labels >= EDAC_MAX_LABELS) {

> -				e->enable_per_layer_report = false;

> -				break;

> -			}

> -			n_labels++;

> -			if (p != e->label) {

> -				strcpy(p, OTHER_LABEL);

> -				p += strlen(OTHER_LABEL);

> -			}

> -			strcpy(p, dimm->label);

> -			p += strlen(p);

> -			*p = '\0';

> +		if (!e->enable_per_layer_report || !dimm->nr_pages)

> +			continue;

>  

> -			/*

> -			 * get csrow/channel of the DIMM, in order to allow

> -			 * incrementing the compat API counters

> -			 */

> -			edac_dbg(4, "%s csrows map: (%d,%d)\n",

> -				 mci->csbased ? "rank" : "dimm",

> -				 dimm->csrow, dimm->cschannel);

> -			if (row == -1)

> -				row = dimm->csrow;

> -			else if (row >= 0 && row != dimm->csrow)

> -				row = -2;

> -

> -			if (chan == -1)

> -				chan = dimm->cschannel;

> -			else if (chan >= 0 && chan != dimm->cschannel)

> -				chan = -2;

> +		if (n_labels >= EDAC_MAX_LABELS) {

> +			e->enable_per_layer_report = false;

> +			break;

> +		}

> +		n_labels++;

> +		if (p != e->label) {

> +			strcpy(p, OTHER_LABEL);

> +			p += strlen(OTHER_LABEL);

>  		}

> +		strcpy(p, dimm->label);

> +		p += strlen(p);

> +		*p = '\0';

> +

> +		/*

> +		 * get csrow/channel of the DIMM, in order to allow

> +		 * incrementing the compat API counters

> +		 */

> +		edac_dbg(4, "%s csrows map: (%d,%d)\n",

> +			mci->csbased ? "rank" : "dimm",

> +			dimm->csrow, dimm->cschannel);

> +		if (row == -1)

> +			row = dimm->csrow;

> +		else if (row >= 0 && row != dimm->csrow)

> +			row = -2;

> +

> +		if (chan == -1)

> +			chan = dimm->cschannel;

> +		else if (chan >= 0 && chan != dimm->cschannel)

> +			chan = -2;

>  	}

>  

>  	if (!e->enable_per_layer_report) {




Thanks,
Mauro
Mauro Carvalho Chehab Oct. 11, 2019, 10:20 a.m. UTC | #4 
Em Thu, 10 Oct 2019 15:10:53 -0700
Joe Perches <joe@perches.com> escreveu:

> On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:

> > Reduce the indentation level in edac_mc_handle_error() a bit by using

> > continue. No functional changes.  

> 

> Seems fine, but trivially below:

> 

> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c  

> []

> > @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,  

> []

> > +		strcpy(p, dimm->label);

> > +		p += strlen(p);

> > +		*p = '\0';  

> 

> This *p = '\0' is unnecessary as the strcpy already did that.


True, but better to put it on a separate patch, as it makes
easier to review if you don't mix code de-indent with changes.

Also, maybe there are other occurrences of this pattern.

Thanks,
Mauro
Joe Perches Oct. 11, 2019, 10:50 a.m. UTC | #5 
On Fri, 2019-10-11 at 07:20 -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 10 Oct 2019 15:10:53 -0700

> Joe Perches <joe@perches.com> escreveu:

> 

> > On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:

> > > Reduce the indentation level in edac_mc_handle_error() a bit by using

> > > continue. No functional changes.  

> > 

> > Seems fine, but trivially below:

> > 

> > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c  

> > []

> > > @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,  

> > []

> > > +		strcpy(p, dimm->label);

> > > +		p += strlen(p);

> > > +		*p = '\0';  

> > 

> > This *p = '\0' is unnecessary as the strcpy already did that.

> 

> True, but better to put it on a separate patch, as it makes

> easier to review if you don't mix code de-indent with changes.

> 

> Also, maybe there are other occurrences of this pattern.


Maybe 80 or so uses of paired

	strcpy(foo, bar);
	strlen(foo)

where another function like stpcpy, which doesn't exist
in the kernel, could have been used.
Robert Richter Oct. 11, 2019, 12:08 p.m. UTC | #6 
On 11.10.19 03:50:23, Joe Perches wrote:
> On Fri, 2019-10-11 at 07:20 -0300, Mauro Carvalho Chehab wrote:

> > Em Thu, 10 Oct 2019 15:10:53 -0700

> > Joe Perches <joe@perches.com> escreveu:

> > 

> > > On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:

> > > > Reduce the indentation level in edac_mc_handle_error() a bit by using

> > > > continue. No functional changes.  

> > > 

> > > Seems fine, but trivially below:

> > > 

> > > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c  

> > > []

> > > > @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,  

> > > []

> > > > +		strcpy(p, dimm->label);

> > > > +		p += strlen(p);

> > > > +		*p = '\0';  

> > > 

> > > This *p = '\0' is unnecessary as the strcpy already did that.

> > 

> > True, but better to put it on a separate patch, as it makes

> > easier to review if you don't mix code de-indent with changes.

> > 

> > Also, maybe there are other occurrences of this pattern.

> 

> Maybe 80 or so uses of paired

> 

> 	strcpy(foo, bar);

> 	strlen(foo)

> 

> where another function like stpcpy, which doesn't exist

> in the kernel, could have been used.


Under drivers/edac/ I found this one place only.

So I could create a separate patch as a oneliner with that (trivial)
change?

Thanks,

-Robert
Joe Perches Oct. 11, 2019, 2:49 p.m. UTC | #7 
On Fri, 2019-10-11 at 12:08 +0000, Robert Richter wrote:
> On 11.10.19 03:50:23, Joe Perches wrote:

> > On Fri, 2019-10-11 at 07:20 -0300, Mauro Carvalho Chehab wrote:

> > > Em Thu, 10 Oct 2019 15:10:53 -0700 Joe Perches <joe@perches.com> escreveu:

> > > > On Thu, 2019-10-10 at 20:25 +0000, Robert Richter wrote:

> > > > > Reduce the indentation level in edac_mc_handle_error() a bit by using

> > > > > continue. No functional changes.  

> > > > 

> > > > Seems fine, but trivially below:

> > > > 

> > > > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c  

> > > > []

> > > > > @@ -1171,37 +1171,38 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,  

> > > > []

> > > > > +		strcpy(p, dimm->label);

> > > > > +		p += strlen(p);

> > > > > +		*p = '\0';  

> > > > 

> > > > This *p = '\0' is unnecessary as the strcpy already did that.

> > > 

> > > True, but better to put it on a separate patch, as it makes

> > > easier to review if you don't mix code de-indent with changes.

> > > 

> > > Also, maybe there are other occurrences of this pattern.

> > 

> > Maybe 80 or so uses of paired

> > 

> > 	strcpy(foo, bar);

> > 	strlen(foo)

> > 

> > where another function like stpcpy, which doesn't exist

> > in the kernel, could have been used.

> 

> Under drivers/edac/ I found this one place only.

> 

> So I could create a separate patch as a oneliner with that (trivial)

> change?


Of course yes.
diff mbox series

Patch

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index f2cbca77bc50..45b02bb31964 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -1171,37 +1171,38 @@  void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 		 * channel/memory controller/...  may be affected.
 		 * Also, don't show errors for empty DIMM slots.
 		 */
-		if (e->enable_per_layer_report && dimm->nr_pages) {
-			if (n_labels >= EDAC_MAX_LABELS) {
-				e->enable_per_layer_report = false;
-				break;
-			}
-			n_labels++;
-			if (p != e->label) {
-				strcpy(p, OTHER_LABEL);
-				p += strlen(OTHER_LABEL);
-			}
-			strcpy(p, dimm->label);
-			p += strlen(p);
-			*p = '\0';
+		if (!e->enable_per_layer_report || !dimm->nr_pages)
+			continue;
 
-			/*
-			 * get csrow/channel of the DIMM, in order to allow
-			 * incrementing the compat API counters
-			 */
-			edac_dbg(4, "%s csrows map: (%d,%d)\n",
-				 mci->csbased ? "rank" : "dimm",
-				 dimm->csrow, dimm->cschannel);
-			if (row == -1)
-				row = dimm->csrow;
-			else if (row >= 0 && row != dimm->csrow)
-				row = -2;
-
-			if (chan == -1)
-				chan = dimm->cschannel;
-			else if (chan >= 0 && chan != dimm->cschannel)
-				chan = -2;
+		if (n_labels >= EDAC_MAX_LABELS) {
+			e->enable_per_layer_report = false;
+			break;
+		}
+		n_labels++;
+		if (p != e->label) {
+			strcpy(p, OTHER_LABEL);
+			p += strlen(OTHER_LABEL);
 		}
+		strcpy(p, dimm->label);
+		p += strlen(p);
+		*p = '\0';
+
+		/*
+		 * get csrow/channel of the DIMM, in order to allow
+		 * incrementing the compat API counters
+		 */
+		edac_dbg(4, "%s csrows map: (%d,%d)\n",
+			mci->csbased ? "rank" : "dimm",
+			dimm->csrow, dimm->cschannel);
+		if (row == -1)
+			row = dimm->csrow;
+		else if (row >= 0 && row != dimm->csrow)
+			row = -2;
+
+		if (chan == -1)
+			chan = dimm->cschannel;
+		else if (chan >= 0 && chan != dimm->cschannel)
+			chan = -2;
 	}
 
 	if (!e->enable_per_layer_report) {