[PATCH 0/2] 390/qeth: Fine-tuning for qeth_core_set_online()

[PATCH 0/2] 390/qeth: Fine-tuning for qeth_core_set_online()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 10:56:45 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an unnecessary variable initialisation
  Refactoring

 drivers/s390/net/qeth_core_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.6.3

[PATCH 1/2] 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 10:48:05 +0100

Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/net/qeth_core_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 7871537..54fde2e 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -5637,7 +5637,7 @@ static void qeth_core_remove_device(struct ccwgroup_device *gdev)
 static int qeth_core_set_online(struct ccwgroup_device *gdev)
 {
 	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
-	int rc = 0;
+	int rc;
 	int def_discipline;
 
 	if (!card->discipline) {
-- 
2.6.3

[PATCH 2/2] 390/qeth: Refactoring for qeth_core_set_online()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 10:50:11 +0100

Reduce the scope for the local variable "def_discipline" to one branch
of an if statement.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/net/qeth_core_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 54fde2e..3261977 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -5638,9 +5638,10 @@ static int qeth_core_set_online(struct ccwgroup_device *gdev)
 {
 	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
 	int rc;
-	int def_discipline;
 
 	if (!card->discipline) {
+		int def_discipline;
+
 		if (card->info.type == QETH_CARD_TYPE_IQD)
 			def_discipline = QETH_DISCIPLINE_LAYER3;
 		else
-- 
2.6.3

Re: [PATCH 1/2] 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[Heiko Carstens]

On Sun, Jan 03, 2016 at 11:02:00AM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 10:48:05 +0100
> 
> Omit explicit initialisation at the beginning for one local variable
> that is redefined before its first use.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/net/qeth_core_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index 7871537..54fde2e 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -5637,7 +5637,7 @@ static void qeth_core_remove_device(struct ccwgroup_device *gdev)
>  static int qeth_core_set_online(struct ccwgroup_device *gdev)
>  {
>  	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
> -	int rc = 0;
> +	int rc;
>  	int def_discipline;

You can generate hundreds of patches like this one. There are even plenty
more opportunities within this same file. I don't think we need this.

If at all then change all occurrences within a file at once, but that is
Ursula's call.

Re: [PATCH 2/2] 390/qeth: Refactoring for qeth_core_set_online()

[Heiko Carstens]

On Sun, Jan 03, 2016 at 11:02:56AM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 10:50:11 +0100
> 
> Reduce the scope for the local variable "def_discipline" to one branch
> of an if statement.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/net/qeth_core_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index 54fde2e..3261977 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -5638,9 +5638,10 @@ static int qeth_core_set_online(struct ccwgroup_device *gdev)
>  {
>  	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
>  	int rc;
> -	int def_discipline;
>  
>  	if (!card->discipline) {
> +		int def_discipline;
> +
>  		if (card->info.type == QETH_CARD_TYPE_IQD)
>  			def_discipline = QETH_DISCIPLINE_LAYER3;

Same here: I don't think we want to start with patches like this. This
going to be a never ending story without much benefit.

Re: [PATCH 2/2] 390/qeth: Refactoring for qeth_core_set_online()

[SF Markus Elfring]

>> +++ b/drivers/s390/net/qeth_core_main.c
>> @@ -5638,9 +5638,10 @@ static int qeth_core_set_online(struct ccwgroup_device *gdev)
>>  {
>>  	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
>>  	int rc;
>> -	int def_discipline;
>>  
>>  	if (!card->discipline) {
>> +		int def_discipline;
>> +
>>  		if (card->info.type == QETH_CARD_TYPE_IQD)
>>  			def_discipline = QETH_DISCIPLINE_LAYER3;
> 
> Same here: I don't think we want to start with patches like this.

Thanks for your feedback.


> This going to be a never ending story without much benefit.

Is the source code a bit clearer and safer if it will be expressed
directly that the use of a specific variable is not intended for
a complete function implementation but for the smaller scope
of an if branch?

Regards,
Markus

Re: [PATCH 2/2] 390/qeth: Refactoring for qeth_core_set_online()

[Heiko Carstens]

On Mon, Jan 04, 2016 at 02:10:34PM +0100, SF Markus Elfring wrote:
> >> +++ b/drivers/s390/net/qeth_core_main.c
> >> @@ -5638,9 +5638,10 @@ static int qeth_core_set_online(struct ccwgroup_device *gdev)
> >>  {
> >>  	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
> >>  	int rc;
> >> -	int def_discipline;
> >>  
> >>  	if (!card->discipline) {
> >> +		int def_discipline;
> >> +
> >>  		if (card->info.type == QETH_CARD_TYPE_IQD)
> >>  			def_discipline = QETH_DISCIPLINE_LAYER3;
> > 
> > Same here: I don't think we want to start with patches like this.
> 
> Thanks for your feedback.
> 
> 
> > This going to be a never ending story without much benefit.
> 
> Is the source code a bit clearer and safer if it will be expressed
> directly that the use of a specific variable is not intended for
> a complete function implementation but for the smaller scope
> of an if branch?

This depends on the function and what the author prefers. In this case the
function body is very small so I don't see any benefit at all.

Re: 390/qeth: Refactoring for qeth_core_set_online()

[SF Markus Elfring]

> In this case the function body is very small
> so I don't see any benefit at all.

Do you care for fine-tuning of variable placement occasionally?

Regards,
Markus

Re: 390/qeth: Refactoring for qeth_core_set_online()

[Heiko Carstens]

On Mon, Jan 04, 2016 at 03:10:41PM +0100, SF Markus Elfring wrote:
> > In this case the function body is very small
> > so I don't see any benefit at all.
> 
> Do you care for fine-tuning of variable placement occasionally?

No.

Re: [PATCH 1/2] 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[Ursula Braun]

On Sun, 2016-01-03 at 11:02 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 10:48:05 +0100
> 
> Omit explicit initialisation at the beginning for one local variable
> that is redefined before its first use.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/net/qeth_core_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index 7871537..54fde2e 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -5637,7 +5637,7 @@ static void qeth_core_remove_device(struct ccwgroup_device *gdev)
>  static int qeth_core_set_online(struct ccwgroup_device *gdev)
>  {
>  	struct qeth_card *card = dev_get_drvdata(&gdev->dev);
> -	int rc = 0;
> +	int rc;
>  	int def_discipline;
> 
>  	if (!card->discipline) {

As Heiko already answered, you could propose a lot of this kind of
changes with just minor benefit. I do not want to push them in single
patches. Either there is a cleanup patch for explicit initialisation of
local variables in the whole qeth driver, or we take care about such
minor changes, once we touch the code anyway due to other reasons.

Re: 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[SF Markus Elfring]

> As Heiko already answered, you could propose a lot of this kind of changes
> with just minor benefit. I do not want to push them in single patches.

Thanks for your clarification.


> Either there is a cleanup patch for explicit initialisation of
> local variables in the whole qeth driver,

Is there any more fine-tuning cooking in the background?


> or we take care about such minor changes, once we touch the code anyway

How often will this really happen?


> due to other reasons.

I am curious which ones will trigger further related software improvements.

Regards,
Markus

Re: 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[Ursula Braun]

On Fri, 2016-01-08 at 08:18 +0100, SF Markus Elfring wrote:
> > As Heiko already answered, you could propose a lot of this kind of changes
> > with just minor benefit. I do not want to push them in single patches.
> 
> Thanks for your clarification.
> 
> 
> > Either there is a cleanup patch for explicit initialisation of
> > local variables in the whole qeth driver,
> 
> Is there any more fine-tuning cooking in the background?
Not yet; qeth is an important driver for Linux on System z; there are
lots of investigation ideas for improvements, which we will take care
about according to their priorities. I regard your proposed fine-tuning
code change as valid, but prioritize it as one with lowest benefit,
since it does not really make a difference once compiled.
> 
> 
> > or we take care about such minor changes, once we touch the code anyway
> 
> How often will this really happen?
There is no general rule. Check our git history to answer this question.
> 
> 
> > due to other reasons.
> 
> I am curious which ones will trigger further related software improvements.
> 
> Regards,
> Markus
> 

Re: 390/qeth: Delete an unnecessary variable initialisation in qeth_core_set_online()

[SF Markus Elfring]

>> Is there any more fine-tuning cooking in the background?
> Not yet;

I am a bit surprised by this information.


> qeth is an important driver for Linux on System z;

Good to know …


> there are lots of investigation ideas for improvements,
> which we will take care about according to their priorities.

Software development as usual …


> I regard your proposed fine-tuning code change as valid,

Thanks for a bit of positive feedback.


> but prioritize it as one with lowest benefit,

This is fine in principle.


> since it does not really make a difference once compiled.

Would you like to help in the determination if deletion of unnecessary variable
initialisations (besides in the implementation of the function "qeth_core_set_online")
will result in measurable effects?

Regards,
Markus

[PATCH] s390/tape: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 19:25:34 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/char/tape_3590.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/char/tape_3590.c b/drivers/s390/char/tape_3590.c
index d3d1936..e352047 100644
--- a/drivers/s390/char/tape_3590.c
+++ b/drivers/s390/char/tape_3590.c
@@ -312,15 +312,10 @@ static int tape_3592_ioctl_kekl_set(struct tape_device *device,
 		return -ENOSYS;
 	if (!crypt_enabled(device))
 		return -EUNATCH;
-	ext_kekls = kmalloc(sizeof(*ext_kekls), GFP_KERNEL);
-	if (!ext_kekls)
-		return -ENOMEM;
-	if (copy_from_user(ext_kekls, (char __user *)arg, sizeof(*ext_kekls))) {
-		rc = -EFAULT;
-		goto out;
-	}
+	ext_kekls = memdup_user((char __user *)arg, sizeof(*ext_kekls));
+	if (IS_ERR(ext_kekls))
+		return PTR_ERR(ext_kekls);
 	rc = tape_3592_kekl_set(device, ext_kekls);
-out:
 	kfree(ext_kekls);
 	return rc;
 }
-- 
2.9.3

Re: [PATCH] s390/tape: Use memdup_user() rather than duplicating its implementation

[Martin Schwidefsky]

On Sat, 20 Aug 2016 19:32:03 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 19:25:34 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to linux-s390. Thanks.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[PATCH 00/17] s390/debug: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Sep 2016 13:54:32 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (17):
  Use kmalloc_array() in debug_areas_alloc()
  Delete unnecessary braces
  Add some spaces for better code readability
  Rename jump labels in debug_areas_alloc()
  Fix jump targets in debug_info_alloc()
  Rename jump labels in debug_info_copy()
  Rename jump labels in debug_open()
  Fix a jump target in debug_register_mode()
  Return directly if a null pointer was passed to debug_unregister()
  Delete an unnecessary initialisation in debug_prolog_level_fn()
  Fix indentation in 13 functions
  Use memdup_user() rather than duplicating its implementation
  Improve a size determination in debug_open()
  Improve a size determination in debug_sprintf_format_fn()
  Improve a size determination in debug_raw_header_fn()
  Improve determination of sizes in debug_info_alloc()
  Improve another size determination in debug_info_alloc()

 arch/s390/kernel/debug.c | 433 ++++++++++++++++++++++-------------------------
 1 file changed, 204 insertions(+), 229 deletions(-)

-- 
2.9.3

[PATCH 01/17] s390/debug: Use kmalloc_array() in debug_areas_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 14:41:01 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

* Replace the specification of data types by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index aa12de7..8e2be30 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -196,14 +196,13 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 	debug_entry_t*** areas;
 	int i,j;
 
-	areas = kmalloc(nr_areas *
-					sizeof(debug_entry_t**),
-					GFP_KERNEL);
+	areas = kmalloc_array(nr_areas, sizeof(*areas), GFP_KERNEL);
 	if (!areas)
 		goto fail_malloc_areas;
 	for (i = 0; i < nr_areas; i++) {
-		areas[i] = kmalloc(pages_per_area *
-				sizeof(debug_entry_t*),GFP_KERNEL);
+		areas[i] = kmalloc_array(pages_per_area,
+					 sizeof(*areas[i]),
+					 GFP_KERNEL);
 		if (!areas[i]) {
 			goto fail_malloc_areas2;
 		}
-- 
2.9.3

[PATCH 02/17] s390/debug: Delete unnecessary braces

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 15:06:06 +0200

Do not use curly brackets at some source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 48 ++++++++++++++++--------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 8e2be30..ddfc5e4 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -203,15 +203,13 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 		areas[i] = kmalloc_array(pages_per_area,
 					 sizeof(*areas[i]),
 					 GFP_KERNEL);
-		if (!areas[i]) {
+		if (!areas[i])
 			goto fail_malloc_areas2;
-		}
 		for(j = 0; j < pages_per_area; j++) {
 			areas[i][j] = kzalloc(PAGE_SIZE, GFP_KERNEL);
 			if(!areas[i][j]) {
-				for(j--; j >=0 ; j--) {
+				for (j--; j >= 0; j--)
 					kfree(areas[i][j]);
-				}
 				kfree(areas[i]);
 				goto fail_malloc_areas2;
 			}
@@ -221,9 +219,8 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 
 fail_malloc_areas2:
 	for(i--; i >= 0; i--){
-		for(j=0; j < pages_per_area;j++){
+		for (j = 0; j < pages_per_area; j++)
 			kfree(areas[i][j]);
-		}
 		kfree(areas[i]);
 	}
 	kfree(areas);
@@ -303,9 +300,8 @@ debug_areas_free(debug_info_t* db_info)
 	if(!db_info->areas)
 		return;
 	for (i = 0; i < db_info->nr_areas; i++) {
-		for(j = 0; j < db_info->pages_per_area; j++) {
+		for (j = 0; j < db_info->pages_per_area; j++)
 			kfree(db_info->areas[i][j]);
-		}
 		kfree(db_info->areas[i]);
 	}
 	kfree(db_info->areas);
@@ -396,11 +392,9 @@ debug_info_copy(debug_info_t* in, int mode)
 	if (mode == NO_AREAS)
                 goto out;
 
-        for(i = 0; i < in->nr_areas; i++){
-		for(j = 0; j < in->pages_per_area; j++) {
+	for (i = 0; i < in->nr_areas; i++)
+		for (j = 0; j < in->pages_per_area; j++)
 			memcpy(rc->areas[i][j], in->areas[i][j],PAGE_SIZE);
-		}
-        }
 out:
         spin_unlock_irqrestore(&in->lock, flags);
         return rc;
@@ -711,9 +705,8 @@ debug_info_t *debug_register_mode(const char *name, int pages_per_area,
         debug_register_view(rc, &debug_flush_view);
 	debug_register_view(rc, &debug_pages_view);
 out:
-        if (!rc){
+	if (!rc)
 		pr_err("Registering debug feature %s failed\n", name);
-        }
 	mutex_unlock(&debug_mutex);
 	return rc;
 }
@@ -1005,10 +998,9 @@ debug_count_numargs(char *string)
 {
 	int numargs=0;
 
-	while(*string) {
+	while (*string)
 		if(*string++=='%')
 			numargs++;
-	}
 	return(numargs);
 }
 
@@ -1114,10 +1106,9 @@ debug_register_view(debug_info_t * id, struct debug_view *view)
 		goto out;
 	}
 	spin_lock_irqsave(&id->lock, flags);
-	for (i = 0; i < DEBUG_MAX_VIEWS; i++) {
+	for (i = 0; i < DEBUG_MAX_VIEWS; i++)
 		if (!id->views[i])
 			break;
-	}
 	if (i == DEBUG_MAX_VIEWS) {
 		pr_err("Registering view %s/%s would exceed the maximum "
 		       "number of views %i\n", id->name, view->name, i);
@@ -1148,10 +1139,9 @@ debug_unregister_view(debug_info_t * id, struct debug_view *view)
 	if (!id)
 		goto out;
 	spin_lock_irqsave(&id->lock, flags);
-	for (i = 0; i < DEBUG_MAX_VIEWS; i++) {
+	for (i = 0; i < DEBUG_MAX_VIEWS; i++)
 		if (id->views[i] == view)
 			break;
-	}
 	if (i == DEBUG_MAX_VIEWS)
 		rc = -1;
 	else {
@@ -1193,9 +1183,8 @@ debug_get_uint(char *buf)
 
 	buf = skip_spaces(buf);
 	rc = simple_strtoul(buf, &buf, 10);
-	if(*buf){
+	if (*buf)
 		rc = -EINVAL;
-	}
 	return rc;
 }
 
@@ -1265,12 +1254,10 @@ debug_prolog_level_fn(debug_info_t * id, struct debug_view *view, char *out_buf)
 {
 	int rc = 0;
 
-	if(id->level == DEBUG_OFF_LEVEL) {
+	if (id->level == DEBUG_OFF_LEVEL)
 		rc = sprintf(out_buf,"-\n");
-	}
-	else {
+	else
 		rc = sprintf(out_buf, "%i\n", id->level);
-	}
 	return rc;
 }
 
@@ -1336,16 +1323,14 @@ static void debug_flush(debug_info_t* id, int area)
                 memset(id->active_entries, 0, id->nr_areas * sizeof(int));
                 for (i = 0; i < id->nr_areas; i++) {
 			id->active_pages[i] = 0;
-			for(j = 0; j < id->pages_per_area; j++) {
+			for (j = 0; j < id->pages_per_area; j++)
                         	memset(id->areas[i][j], 0, PAGE_SIZE);
-			}
 		}
         } else if(area >= 0 && area < id->nr_areas) {
                 id->active_entries[area] = 0;
 		id->active_pages[area] = 0;
-		for(i = 0; i < id->pages_per_area; i++) {
+		for (i = 0; i < id->pages_per_area; i++)
                 	memset(id->areas[area][i],0,PAGE_SIZE);
-		}
         }
         spin_unlock_irqrestore(&id->lock,flags);
 }
@@ -1430,10 +1415,9 @@ debug_hex_ascii_format_fn(debug_info_t * id, struct debug_view *view,
 {
 	int i, rc = 0;
 
-	for (i = 0; i < id->buf_size; i++) {
+	for (i = 0; i < id->buf_size; i++)
                 rc += sprintf(out_buf + rc, "%02x ",
                               ((unsigned char *) in_buf)[i]);
-        }
 	rc += sprintf(out_buf + rc, "| ");
 	for (i = 0; i < id->buf_size; i++) {
 		unsigned char c = in_buf[i];
-- 
2.9.3

[PATCH 03/17] s390/debug: Add some spaces for better code readability

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 16:00:39 +0200

Use space characters at some source code places according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 177 +++++++++++++++++++++++++----------------------
 1 file changed, 93 insertions(+), 84 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index ddfc5e4..5bb9108 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -205,9 +205,9 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 					 GFP_KERNEL);
 		if (!areas[i])
 			goto fail_malloc_areas2;
-		for(j = 0; j < pages_per_area; j++) {
+		for (j = 0; j < pages_per_area; j++) {
 			areas[i][j] = kzalloc(PAGE_SIZE, GFP_KERNEL);
-			if(!areas[i][j]) {
+			if (!areas[i][j]) {
 				for (j--; j >= 0; j--)
 					kfree(areas[i][j]);
 				kfree(areas[i]);
@@ -218,7 +218,7 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 	return areas;
 
 fail_malloc_areas2:
-	for(i--; i >= 0; i--){
+	for (i--; i >= 0; i--) {
 		for (j = 0; j < pages_per_area; j++)
 			kfree(areas[i][j]);
 		kfree(areas[i]);
@@ -271,7 +271,7 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 	rc->entry_size     = sizeof(debug_entry_t) + buf_size;
 	strlcpy(rc->name, name, sizeof(rc->name));
 	memset(rc->views, 0, DEBUG_MAX_VIEWS * sizeof(struct debug_view *));
-	memset(rc->debugfs_entries, 0 ,DEBUG_MAX_VIEWS *
+	memset(rc->debugfs_entries, 0, DEBUG_MAX_VIEWS *
 		sizeof(struct dentry*));
 	atomic_set(&(rc->ref_count), 0);
 
@@ -297,7 +297,7 @@ debug_areas_free(debug_info_t* db_info)
 {
 	int i,j;
 
-	if(!db_info->areas)
+	if (!db_info->areas)
 		return;
 	for (i = 0; i < db_info->nr_areas; i++) {
 		for (j = 0; j < db_info->pages_per_area; j++)
@@ -334,7 +334,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
 
         rc = debug_info_alloc(name, pages_per_area, nr_areas, buf_size,
 				DEBUG_DEFAULT_LEVEL, ALL_AREAS);
-        if(!rc) 
+	if (!rc)
 		goto out;
 
 	rc->mode = mode & ~S_IFMT;
@@ -378,10 +378,10 @@ debug_info_copy(debug_info_t* in, int mode)
 		rc = debug_info_alloc(in->name, in->pages_per_area,
 			in->nr_areas, in->buf_size, in->level, mode);
 		spin_lock_irqsave(&in->lock, flags);
-		if(!rc)
+		if (!rc)
 			goto out;
 		/* has something changed in the meantime ? */
-		if((rc->pages_per_area == in->pages_per_area) &&
+		if ((rc->pages_per_area == in->pages_per_area) &&
 		   (rc->nr_areas == in->nr_areas)) {
 			break;
 		}
@@ -394,7 +394,7 @@ debug_info_copy(debug_info_t* in, int mode)
 
 	for (i = 0; i < in->nr_areas; i++)
 		for (j = 0; j < in->pages_per_area; j++)
-			memcpy(rc->areas[i][j], in->areas[i][j],PAGE_SIZE);
+			memcpy(rc->areas[i][j], in->areas[i][j], PAGE_SIZE);
 out:
         spin_unlock_irqrestore(&in->lock, flags);
         return rc;
@@ -431,12 +431,14 @@ debug_info_put(debug_info_t *db_info)
 			debugfs_remove(db_info->debugfs_entries[i]);
 		}
 		debugfs_remove(db_info->debugfs_root_entry);
-		if(db_info == debug_area_first)
+		if (db_info == debug_area_first)
 			debug_area_first = db_info->next;
-		if(db_info == debug_area_last)
+		if (db_info == debug_area_last)
 			debug_area_last = db_info->prev;
-		if(db_info->prev) db_info->prev->next = db_info->next;
-		if(db_info->next) db_info->next->prev = db_info->prev;
+		if (db_info->prev)
+			db_info->prev->next = db_info->next;
+		if (db_info->next)
+			db_info->next->prev = db_info->prev;
 		debug_info_free(db_info);
 	}
 }
@@ -453,10 +455,13 @@ debug_format_entry(file_private_info_t *p_info)
 	struct debug_view *view = p_info->view;
 	debug_entry_t *act_entry;
 	size_t len = 0;
-	if(p_info->act_entry == DEBUG_PROLOG_ENTRY){
+
+	if (p_info->act_entry == DEBUG_PROLOG_ENTRY) {
 		/* print prolog */
         	if (view->prolog_proc)
-                	len += view->prolog_proc(id_snap,view,p_info->temp_buf);
+			len += view->prolog_proc(id_snap,
+						 view,
+						 p_info->temp_buf);
 		goto out;
 	}
 	if (!id_snap->areas) /* this is true, if we have a prolog only view */
@@ -487,25 +492,25 @@ debug_next_entry(file_private_info_t *p_info)
 	debug_info_t *id;
 
 	id = p_info->debug_info_snap;
-	if(p_info->act_entry == DEBUG_PROLOG_ENTRY){
+	if (p_info->act_entry == DEBUG_PROLOG_ENTRY) {
 		p_info->act_entry = 0;
 		p_info->act_page  = 0;
 		goto out;
 	}
-	if(!id->areas)
+	if (!id->areas)
 		return 1;
 	p_info->act_entry += id->entry_size;
 	/* switch to next page, if we reached the end of the page  */
-	if (p_info->act_entry > (PAGE_SIZE - id->entry_size)){
+	if (p_info->act_entry > (PAGE_SIZE - id->entry_size)) {
 		/* next page */
 		p_info->act_entry = 0;
 		p_info->act_page += 1;
-		if((p_info->act_page % id->pages_per_area) == 0) {
+		if ((p_info->act_page % id->pages_per_area) == 0) {
 			/* next area */
         		p_info->act_area++;
 			p_info->act_page=0;
 		}
-        	if(p_info->act_area >= id->nr_areas)
+		if (p_info->act_area >= id->nr_areas)
 			return 1;
 	}
 out:
@@ -531,10 +536,10 @@ debug_output(struct file *file,		/* file descriptor */
 	p_info = ((file_private_info_t *) file->private_data);
 	if (*offset != p_info->offset) 
 		return -EPIPE;
-	if(p_info->act_area >= p_info->debug_info_snap->nr_areas)
+	if (p_info->act_area >= p_info->debug_info_snap->nr_areas)
 		return 0;
 	entry_offset = p_info->act_entry_offset;
-	while(count < len){
+	while (count < len) {
 		int formatted_line_size;
 		int formatted_line_residue;
 		int user_buf_residue;
@@ -544,16 +549,16 @@ debug_output(struct file *file,		/* file descriptor */
 		formatted_line_residue = formatted_line_size - entry_offset;
 		user_buf_residue = len-count;
 		copy_size = min(user_buf_residue, formatted_line_residue);
-		if(copy_size){
+		if (copy_size) {
 			if (copy_to_user(user_buf + count, p_info->temp_buf
 					+ entry_offset, copy_size))
 				return -EFAULT;
 			count += copy_size;
 			entry_offset += copy_size;
 		}
-		if(copy_size == formatted_line_residue){
+		if (copy_size == formatted_line_residue) {
 			entry_offset = 0;
-			if(debug_next_entry(p_info))
+			if (debug_next_entry(p_info))
 				goto out;
 		}
 	}
@@ -624,20 +629,20 @@ found:
 	/* To copy all the areas is only needed, if we have a view which  */
 	/* formats the debug areas. */
 
-	if(!debug_info->views[i]->format_proc &&
-		!debug_info->views[i]->header_proc){
+	if (!debug_info->views[i]->format_proc &&
+		!debug_info->views[i]->header_proc) {
 		debug_info_snapshot = debug_info_copy(debug_info, NO_AREAS);
 	} else {
 		debug_info_snapshot = debug_info_copy(debug_info, ALL_AREAS);
 	}
 
-	if(!debug_info_snapshot){
+	if (!debug_info_snapshot) {
 		rc = -ENOMEM;
 		goto out;
 	}
 	p_info = kmalloc(sizeof(file_private_info_t),
 						GFP_KERNEL);
-	if(!p_info){
+	if (!p_info) {
 		debug_info_free(debug_info_snapshot);
 		rc = -ENOMEM;
 		goto out;
@@ -669,7 +674,7 @@ debug_close(struct inode *inode, struct file *file)
 {
 	file_private_info_t *p_info;
 	p_info = (file_private_info_t *) file->private_data;
-	if(p_info->debug_info_snap)
+	if (p_info->debug_info_snap)
 		debug_info_free(p_info->debug_info_snap);
 	debug_info_put(p_info->debug_info_org);
 	kfree(file->private_data);
@@ -699,7 +704,7 @@ debug_info_t *debug_register_mode(const char *name, int pages_per_area,
         /* create new debug_info */
 
 	rc = debug_info_create(name, pages_per_area, nr_areas, buf_size, mode);
-	if(!rc) 
+	if (!rc)
 		goto out;
 	debug_register_view(rc, &debug_level_view);
         debug_register_view(rc, &debug_flush_view);
@@ -754,11 +759,11 @@ debug_set_size(debug_info_t* id, int nr_areas, int pages_per_area)
 {
 	unsigned long flags;
 	debug_entry_t *** new_areas;
-	int rc=0;
+	int rc = 0;
 
-	if(!id || (nr_areas <= 0) || (pages_per_area < 0))
+	if (!id || (nr_areas <= 0) || (pages_per_area < 0))
 		return -EINVAL;
-	if(pages_per_area > 0){
+	if (pages_per_area > 0) {
 		new_areas = debug_areas_alloc(pages_per_area, nr_areas);
 		if(!new_areas) {
 			pr_info("Allocating memory for %i pages failed\n",
@@ -769,16 +774,16 @@ debug_set_size(debug_info_t* id, int nr_areas, int pages_per_area)
 	} else {
 		new_areas = NULL;
 	}
-	spin_lock_irqsave(&id->lock,flags);
+	spin_lock_irqsave(&id->lock, flags);
 	debug_areas_free(id);
 	id->areas = new_areas;
 	id->nr_areas = nr_areas;
 	id->pages_per_area = pages_per_area;
 	id->active_area = 0;
-	memset(id->active_entries,0,sizeof(int)*id->nr_areas);
-	memset(id->active_pages, 0, sizeof(int)*id->nr_areas);
-	spin_unlock_irqrestore(&id->lock,flags);
-	pr_info("%s: set new size (%i pages)\n" ,id->name, pages_per_area);
+	memset(id->active_entries, 0, sizeof(int) * id->nr_areas);
+	memset(id->active_pages, 0, sizeof(int) * id->nr_areas);
+	spin_unlock_irqrestore(&id->lock, flags);
+	pr_info("%s: set new size (%i pages)\n", id->name, pages_per_area);
 out:
 	return rc;
 }
@@ -794,17 +799,17 @@ debug_set_level(debug_info_t* id, int new_level)
 	unsigned long flags;
 	if(!id)
 		return;	
-	spin_lock_irqsave(&id->lock,flags);
-        if(new_level == DEBUG_OFF_LEVEL){
+	spin_lock_irqsave(&id->lock, flags);
+	if (new_level == DEBUG_OFF_LEVEL) {
                 id->level = DEBUG_OFF_LEVEL;
-		pr_info("%s: switched off\n",id->name);
+		pr_info("%s: switched off\n", id->name);
         } else if ((new_level > DEBUG_MAX_LEVEL) || (new_level < 0)) {
 		pr_info("%s: level %i is out of range (%i - %i)\n",
                         id->name, new_level, 0, DEBUG_MAX_LEVEL);
         } else {
                 id->level = new_level;
         }
-	spin_unlock_irqrestore(&id->lock,flags);
+	spin_unlock_irqrestore(&id->lock, flags);
 }
 EXPORT_SYMBOL(debug_set_level);
 
@@ -996,10 +1001,10 @@ EXPORT_SYMBOL(debug_exception_common);
 static inline int
 debug_count_numargs(char *string)
 {
-	int numargs=0;
+	int numargs = 0;
 
 	while (*string)
-		if(*string++=='%')
+		if (*string++ == '%')
 			numargs++;
 	return(numargs);
 }
@@ -1019,7 +1024,7 @@ __debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
 
 	if (!debug_active || !id->areas)
 		return NULL;
-	numargs=debug_count_numargs(string);
+	numargs = debug_count_numargs(string);
 
 	if (debug_critical) {
 		if (!spin_trylock_irqsave(&id->lock, flags))
@@ -1027,11 +1032,13 @@ __debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
 	} else
 		spin_lock_irqsave(&id->lock, flags);
 	active = get_active_entry(id);
-	curr_event=(debug_sprintf_entry_t *) DEBUG_DATA(active);
+	curr_event = (debug_sprintf_entry_t *) DEBUG_DATA(active);
 	va_start(ap,string);
-	curr_event->string=string;
-	for(idx=0;idx<min(numargs,(int)(id->buf_size / sizeof(long))-1);idx++)
-		curr_event->args[idx]=va_arg(ap,long);
+	curr_event->string = string;
+	for (idx = 0;
+	     idx < min(numargs, (int)(id->buf_size / sizeof(long)) - 1);
+	     idx++)
+		curr_event->args[idx] = va_arg(ap, long);
 	va_end(ap);
 	debug_finish_entry(id, active, level, 0);
 	spin_unlock_irqrestore(&id->lock, flags);
@@ -1056,7 +1063,7 @@ __debug_sprintf_exception(debug_info_t *id, int level, char *string, ...)
 	if (!debug_active || !id->areas)
 		return NULL;
 
-	numargs=debug_count_numargs(string);
+	numargs = debug_count_numargs(string);
 
 	if (debug_critical) {
 		if (!spin_trylock_irqsave(&id->lock, flags))
@@ -1064,11 +1071,13 @@ __debug_sprintf_exception(debug_info_t *id, int level, char *string, ...)
 	} else
 		spin_lock_irqsave(&id->lock, flags);
 	active = get_active_entry(id);
-	curr_event=(debug_sprintf_entry_t *)DEBUG_DATA(active);
-	va_start(ap,string);
-	curr_event->string=string;
-	for(idx=0;idx<min(numargs,(int)(id->buf_size / sizeof(long))-1);idx++)
-		curr_event->args[idx]=va_arg(ap,long);
+	curr_event = (debug_sprintf_entry_t *) DEBUG_DATA(active);
+	va_start(ap, string);
+	curr_event->string = string;
+	for (idx = 0;
+	     idx < min(numargs, (int)(id->buf_size / sizeof(long)) - 1);
+	     idx++)
+		curr_event->args[idx] = va_arg(ap, long);
 	va_end(ap);
 	debug_finish_entry(id, active, level, 1);
 	spin_unlock_irqrestore(&id->lock, flags);
@@ -1214,26 +1223,26 @@ debug_input_pages_fn(debug_info_t * id, struct debug_view *view,
 			size_t user_len, loff_t * offset)
 {
 	char *str;
-	int rc,new_pages;
+	int rc, new_pages;
 
 	if (user_len > 0x10000)
                 user_len = 0x10000;
-	if (*offset != 0){
+	if (*offset != 0) {
 		rc = -EPIPE;
 		goto out;
 	}
-	str = debug_get_user_string(user_buf,user_len);
-	if(IS_ERR(str)){
+	str = debug_get_user_string(user_buf, user_len);
+	if (IS_ERR(str)) {
 		rc = PTR_ERR(str);
 		goto out;
 	}
 	new_pages = debug_get_uint(str);
-	if(new_pages < 0){
+	if (new_pages < 0) {
 		rc = -EINVAL;
 		goto free_str;
 	}
-	rc = debug_set_size(id,id->nr_areas, new_pages);
-	if(rc != 0){
+	rc = debug_set_size(id, id->nr_areas, new_pages);
+	if (rc != 0) {
 		rc = -EINVAL;
 		goto free_str;
 	}
@@ -1271,27 +1280,27 @@ debug_input_level_fn(debug_info_t * id, struct debug_view *view,
 			size_t user_len, loff_t * offset)
 {
 	char *str;
-	int rc,new_level;
+	int rc, new_level;
 
 	if (user_len > 0x10000)
                 user_len = 0x10000;
-	if (*offset != 0){
+	if (*offset != 0) {
 		rc = -EPIPE;
 		goto out;
 	}
-	str = debug_get_user_string(user_buf,user_len);
-	if(IS_ERR(str)){
+	str = debug_get_user_string(user_buf, user_len);
+	if (IS_ERR(str)) {
 		rc = PTR_ERR(str);
 		goto out;
 	}
-	if(str[0] == '-'){
+	if (str[0] == '-') {
 		debug_set_level(id, DEBUG_OFF_LEVEL);
 		rc = user_len;
 		goto free_str;
 	} else {
 		new_level = debug_get_uint(str);
 	}
-	if(new_level < 0) {
+	if (new_level < 0) {
 		pr_warn("%s is not a valid level for a debug feature\n", str);
 		rc = -EINVAL;
 	} else {
@@ -1313,12 +1322,12 @@ out:
 static void debug_flush(debug_info_t* id, int area)
 {
         unsigned long flags;
-        int i,j;
+	int i, j;
 
-        if(!id || !id->areas)
+	if (!id || !id->areas)
                 return;
-        spin_lock_irqsave(&id->lock,flags);
-        if(area == DEBUG_FLUSH_ALL){
+	spin_lock_irqsave(&id->lock, flags);
+	if (area == DEBUG_FLUSH_ALL) {
                 id->active_area = 0;
                 memset(id->active_entries, 0, id->nr_areas * sizeof(int));
                 for (i = 0; i < id->nr_areas; i++) {
@@ -1326,13 +1335,13 @@ static void debug_flush(debug_info_t* id, int area)
 			for (j = 0; j < id->pages_per_area; j++)
                         	memset(id->areas[i][j], 0, PAGE_SIZE);
 		}
-        } else if(area >= 0 && area < id->nr_areas) {
+	} else if (area >= 0 && area < id->nr_areas) {
                 id->active_entries[area] = 0;
 		id->active_pages[area] = 0;
 		for (i = 0; i < id->pages_per_area; i++)
-                	memset(id->areas[area][i],0,PAGE_SIZE);
+			memset(id->areas[area][i], 0, PAGE_SIZE);
         }
-        spin_unlock_irqrestore(&id->lock,flags);
+	spin_unlock_irqrestore(&id->lock, flags);
 }
 
 /*
@@ -1349,15 +1358,15 @@ debug_input_flush_fn(debug_info_t * id, struct debug_view *view,
 
 	if (user_len > 0x10000)
                 user_len = 0x10000;
-        if (*offset != 0){
+	if (*offset != 0) {
 		rc = -EPIPE;
                 goto out;
 	}
-        if (copy_from_user(input_buf, user_buf, 1)){
+	if (copy_from_user(input_buf, user_buf, 1)) {
                 rc = -EFAULT;
                 goto out;
         }
-        if(input_buf[0] == '-') { 
+	if (input_buf[0] == '-') {
                 debug_flush(id, DEBUG_FLUSH_ALL);
                 goto out;
         }
@@ -1386,7 +1395,7 @@ debug_raw_header_fn(debug_info_t * id, struct debug_view *view,
         int rc;
 
 	rc = sizeof(debug_entry_t);
-	memcpy(out_buf,entry,sizeof(debug_entry_t));
+	memcpy(out_buf, entry, sizeof(debug_entry_t));
         return rc;
 }
 
@@ -1477,9 +1486,9 @@ debug_sprintf_format_fn(debug_info_t * id, struct debug_view *view,
 	/* count of longs fit into one entry */
 	num_longs = id->buf_size /  sizeof(long); 
 
-	if(num_longs < 1)
+	if (num_longs < 1)
 		goto out; /* bufsize of entry too small */
-	if(num_longs == 1) {
+	if (num_longs == 1) {
 		/* no args, we use only the string */
 		strcpy(out_buf, curr_event->string);
 		rc = strlen(curr_event->string);
@@ -1489,9 +1498,9 @@ debug_sprintf_format_fn(debug_info_t * id, struct debug_view *view,
 	/* number of arguments used for sprintf (without the format string) */
 	num_used_args   = min(DEBUG_SPRINTF_MAX_ARGS, (num_longs - 1));
 
-	memset(index,0, DEBUG_SPRINTF_MAX_ARGS * sizeof(int));
+	memset(index, 0, DEBUG_SPRINTF_MAX_ARGS * sizeof(int));
 
-	for(i = 0; i < num_used_args; i++)
+	for (i = 0; i < num_used_args; i++)
 		index[i] = i;
 
 	rc =  sprintf(out_buf, curr_event->string, curr_event->args[index[0]],
-- 
2.9.3

[PATCH 04/17] s390/debug: Rename jump labels in debug_areas_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 18:10:21 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 5bb9108..0af14a4 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -198,33 +198,32 @@ debug_areas_alloc(int pages_per_area, int nr_areas)
 
 	areas = kmalloc_array(nr_areas, sizeof(*areas), GFP_KERNEL);
 	if (!areas)
-		goto fail_malloc_areas;
+		goto exit;
 	for (i = 0; i < nr_areas; i++) {
 		areas[i] = kmalloc_array(pages_per_area,
 					 sizeof(*areas[i]),
 					 GFP_KERNEL);
 		if (!areas[i])
-			goto fail_malloc_areas2;
+			goto free_areas;
 		for (j = 0; j < pages_per_area; j++) {
 			areas[i][j] = kzalloc(PAGE_SIZE, GFP_KERNEL);
 			if (!areas[i][j]) {
 				for (j--; j >= 0; j--)
 					kfree(areas[i][j]);
 				kfree(areas[i]);
-				goto fail_malloc_areas2;
+				goto free_areas;
 			}
 		}
 	}
 	return areas;
-
-fail_malloc_areas2:
+ free_areas:
 	for (i--; i >= 0; i--) {
 		for (j = 0; j < pages_per_area; j++)
 			kfree(areas[i][j]);
 		kfree(areas[i]);
 	}
 	kfree(areas);
-fail_malloc_areas:
+ exit:
 	return NULL;
 
 }
-- 
2.9.3

[PATCH 05/17] s390/debug: Fix jump targets in debug_info_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 18:26:58 +0200

Adjust jump targets according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 0af14a4..c5da41b 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -244,17 +244,17 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 
 	rc = kmalloc(sizeof(debug_info_t), GFP_KERNEL);
 	if(!rc)
-		goto fail_malloc_rc;
+		goto exit;
 	rc->active_entries = kcalloc(nr_areas, sizeof(int), GFP_KERNEL);
 	if(!rc->active_entries)
-		goto fail_malloc_active_entries;
+		goto free_rc;
 	rc->active_pages = kcalloc(nr_areas, sizeof(int), GFP_KERNEL);
 	if(!rc->active_pages)
-		goto fail_malloc_active_pages;
+		goto free_entries;
 	if((mode == ALL_AREAS) && (pages_per_area != 0)){
 		rc->areas = debug_areas_alloc(pages_per_area, nr_areas);
 		if(!rc->areas)
-			goto fail_malloc_areas;
+			goto free_pages;
 	} else {
 		rc->areas = NULL;
 	}
@@ -275,14 +275,13 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 	atomic_set(&(rc->ref_count), 0);
 
 	return rc;
-
-fail_malloc_areas:
+ free_pages:
 	kfree(rc->active_pages);
-fail_malloc_active_pages:
+ free_entries:
 	kfree(rc->active_entries);
-fail_malloc_active_entries:
+ free_rc:
 	kfree(rc);
-fail_malloc_rc:
+ exit:
 	return NULL;
 }
 
-- 
2.9.3

[PATCH 06/17] s390/debug: Rename jump labels in debug_info_copy()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 18:32:19 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index c5da41b..1f32578 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -377,7 +377,7 @@ debug_info_copy(debug_info_t* in, int mode)
 			in->nr_areas, in->buf_size, in->level, mode);
 		spin_lock_irqsave(&in->lock, flags);
 		if (!rc)
-			goto out;
+			goto unlock;
 		/* has something changed in the meantime ? */
 		if ((rc->pages_per_area == in->pages_per_area) &&
 		   (rc->nr_areas == in->nr_areas)) {
@@ -388,12 +388,12 @@ debug_info_copy(debug_info_t* in, int mode)
 	} while (1);
 
 	if (mode == NO_AREAS)
-                goto out;
+		goto unlock;
 
 	for (i = 0; i < in->nr_areas; i++)
 		for (j = 0; j < in->pages_per_area; j++)
 			memcpy(rc->areas[i][j], in->areas[i][j], PAGE_SIZE);
-out:
+ unlock:
         spin_unlock_irqrestore(&in->lock, flags);
         return rc;
 }
-- 
2.9.3

[PATCH 07/17] s390/debug: Rename jump labels in debug_open()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 18:44:17 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 1f32578..1cce76a 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -614,15 +614,13 @@ debug_open(struct inode *inode, struct file *file)
 			continue;
 		else if (debug_info->debugfs_entries[i] ==
 			 file->f_path.dentry) {
-			goto found;	/* found view ! */
+			goto copy_info;
 		}
 	}
 	/* no entry found */
 	rc = -EINVAL;
-	goto out;
-
-found:
-
+	goto unlock;
+ copy_info:
 	/* Make snapshot of current debug areas to get it consistent.     */
 	/* To copy all the areas is only needed, if we have a view which  */
 	/* formats the debug areas. */
@@ -636,14 +634,14 @@ found:
 
 	if (!debug_info_snapshot) {
 		rc = -ENOMEM;
-		goto out;
+		goto unlock;
 	}
 	p_info = kmalloc(sizeof(file_private_info_t),
 						GFP_KERNEL);
 	if (!p_info) {
 		debug_info_free(debug_info_snapshot);
 		rc = -ENOMEM;
-		goto out;
+		goto unlock;
 	}
 	p_info->offset = 0;
 	p_info->debug_info_snap = debug_info_snapshot;
@@ -656,7 +654,7 @@ found:
 	file->private_data = p_info;
 	debug_info_get(debug_info);
 	nonseekable_open(inode, file);
-out:
+ unlock:
 	mutex_unlock(&debug_mutex);
 	return rc;
 }
-- 
2.9.3

[PATCH 08/17] s390/debug: Fix a jump target in debug_register_mode()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 19:26:16 +0200

* Move an error message.

* Adjust a jump target according to the current Linux coding
  style convention.

* Delete a repeated check which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 1cce76a..c4a4641 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -700,14 +700,14 @@ debug_info_t *debug_register_mode(const char *name, int pages_per_area,
         /* create new debug_info */
 
 	rc = debug_info_create(name, pages_per_area, nr_areas, buf_size, mode);
-	if (!rc)
-		goto out;
+	if (!rc) {
+		pr_err("Registering debug feature %s failed\n", name);
+		goto unlock;
+	}
 	debug_register_view(rc, &debug_level_view);
         debug_register_view(rc, &debug_flush_view);
 	debug_register_view(rc, &debug_pages_view);
-out:
-	if (!rc)
-		pr_err("Registering debug feature %s failed\n", name);
+ unlock:
 	mutex_unlock(&debug_mutex);
 	return rc;
 }
-- 
2.9.3

[PATCH 09/17] s390/debug: Return directly if a null pointer was passed to debug_unregister()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 19:34:45 +0200

Return directly at the beginning if a null pointer was passed for
the input parameter of this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index c4a4641..d137150 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -736,13 +736,10 @@ void
 debug_unregister(debug_info_t * id)
 {
 	if (!id)
-		goto out;
+		return;
 	mutex_lock(&debug_mutex);
 	debug_info_put(id);
 	mutex_unlock(&debug_mutex);
-
-out:
-	return;
 }
 EXPORT_SYMBOL(debug_unregister);
 
-- 
2.9.3

[PATCH 10/17] s390/debug: Delete an unnecessary initialisation in debug_prolog_level_fn()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 21:19:02 +0200

The local variable "rc" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index d137150..ef0db06 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1254,7 +1254,7 @@ out:
 static int
 debug_prolog_level_fn(debug_info_t * id, struct debug_view *view, char *out_buf)
 {
-	int rc = 0;
+	int rc;
 
 	if (id->level == DEBUG_OFF_LEVEL)
 		rc = sprintf(out_buf,"-\n");
-- 
2.9.3

[PATCH 11/17] s390/debug: Fix indentation in 13 functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 1 Sep 2016 23:05:51 +0200

The script "checkpatch.pl" can point the following information out.

ERROR: code indent should use tabs where possible

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 118 +++++++++++++++++++++++------------------------
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index ef0db06..4a12faf 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -330,7 +330,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
 {
 	debug_info_t* rc;
 
-        rc = debug_info_alloc(name, pages_per_area, nr_areas, buf_size,
+	rc = debug_info_alloc(name, pages_per_area, nr_areas, buf_size,
 				DEBUG_DEFAULT_LEVEL, ALL_AREAS);
 	if (!rc)
 		goto out;
@@ -338,21 +338,21 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
 	rc->mode = mode & ~S_IFMT;
 
 	/* create root directory */
-        rc->debugfs_root_entry = debugfs_create_dir(rc->name,
+	rc->debugfs_root_entry = debugfs_create_dir(rc->name,
 					debug_debugfs_root_entry);
 
 	/* append new element to linked list */
-        if (!debug_area_first) {
-                /* first element in list */
-                debug_area_first = rc;
-                rc->prev = NULL;
-        } else {
-                /* append element to end of list */
-                debug_area_last->next = rc;
-                rc->prev = debug_area_last;
-        }
-        debug_area_last = rc;
-        rc->next = NULL;
+	if (!debug_area_first) {
+		/* first element in list */
+		debug_area_first = rc;
+		rc->prev = NULL;
+	} else {
+		/* append element to end of list */
+		debug_area_last->next = rc;
+		rc->prev = debug_area_last;
+	}
+	debug_area_last = rc;
+	rc->next = NULL;
 
 	debug_info_get(rc);
 out:
@@ -394,8 +394,8 @@ debug_info_copy(debug_info_t* in, int mode)
 		for (j = 0; j < in->pages_per_area; j++)
 			memcpy(rc->areas[i][j], in->areas[i][j], PAGE_SIZE);
  unlock:
-        spin_unlock_irqrestore(&in->lock, flags);
-        return rc;
+	spin_unlock_irqrestore(&in->lock, flags);
+	return rc;
 }
 
 /*
@@ -456,7 +456,7 @@ debug_format_entry(file_private_info_t *p_info)
 
 	if (p_info->act_entry == DEBUG_PROLOG_ENTRY) {
 		/* print prolog */
-        	if (view->prolog_proc)
+		if (view->prolog_proc)
 			len += view->prolog_proc(id_snap,
 						 view,
 						 p_info->temp_buf);
@@ -476,7 +476,7 @@ debug_format_entry(file_private_info_t *p_info)
 		len += view->format_proc(id_snap, view, p_info->temp_buf + len,
 						DEBUG_DATA(act_entry));
 out:
-        return len;
+	return len;
 }
 
 /*
@@ -505,7 +505,7 @@ debug_next_entry(file_private_info_t *p_info)
 		p_info->act_page += 1;
 		if ((p_info->act_page % id->pages_per_area) == 0) {
 			/* next area */
-        		p_info->act_area++;
+			p_info->act_area++;
 			p_info->act_page=0;
 		}
 		if (p_info->act_area >= id->nr_areas)
@@ -697,15 +697,14 @@ debug_info_t *debug_register_mode(const char *name, int pages_per_area,
 	BUG_ON(!initialized);
 	mutex_lock(&debug_mutex);
 
-        /* create new debug_info */
-
+	/* create new debug_info */
 	rc = debug_info_create(name, pages_per_area, nr_areas, buf_size, mode);
 	if (!rc) {
 		pr_err("Registering debug feature %s failed\n", name);
 		goto unlock;
 	}
 	debug_register_view(rc, &debug_level_view);
-        debug_register_view(rc, &debug_flush_view);
+	debug_register_view(rc, &debug_flush_view);
 	debug_register_view(rc, &debug_pages_view);
  unlock:
 	mutex_unlock(&debug_mutex);
@@ -794,14 +793,14 @@ debug_set_level(debug_info_t* id, int new_level)
 		return;	
 	spin_lock_irqsave(&id->lock, flags);
 	if (new_level == DEBUG_OFF_LEVEL) {
-                id->level = DEBUG_OFF_LEVEL;
+		id->level = DEBUG_OFF_LEVEL;
 		pr_info("%s: switched off\n", id->name);
-        } else if ((new_level > DEBUG_MAX_LEVEL) || (new_level < 0)) {
+	} else if ((new_level > DEBUG_MAX_LEVEL) || (new_level < 0)) {
 		pr_info("%s: level %i is out of range (%i - %i)\n",
-                        id->name, new_level, 0, DEBUG_MAX_LEVEL);
-        } else {
-                id->level = new_level;
-        }
+			id->name, new_level, 0, DEBUG_MAX_LEVEL);
+	} else {
+		id->level = new_level;
+	}
 	spin_unlock_irqrestore(&id->lock, flags);
 }
 EXPORT_SYMBOL(debug_set_level);
@@ -1175,7 +1174,7 @@ debug_get_user_string(const char __user *user_buf, size_t user_len)
 		buffer[user_len - 1] = 0;
 	else
 		buffer[user_len] = 0;
-        return buffer;
+	return buffer;
 }
 
 static inline int
@@ -1219,7 +1218,7 @@ debug_input_pages_fn(debug_info_t * id, struct debug_view *view,
 	int rc, new_pages;
 
 	if (user_len > 0x10000)
-                user_len = 0x10000;
+		user_len = 0x10000;
 	if (*offset != 0) {
 		rc = -EPIPE;
 		goto out;
@@ -1276,7 +1275,7 @@ debug_input_level_fn(debug_info_t * id, struct debug_view *view,
 	int rc, new_level;
 
 	if (user_len > 0x10000)
-                user_len = 0x10000;
+		user_len = 0x10000;
 	if (*offset != 0) {
 		rc = -EPIPE;
 		goto out;
@@ -1314,26 +1313,26 @@ out:
  
 static void debug_flush(debug_info_t* id, int area)
 {
-        unsigned long flags;
+	unsigned long flags;
 	int i, j;
 
 	if (!id || !id->areas)
-                return;
+		return;
 	spin_lock_irqsave(&id->lock, flags);
 	if (area == DEBUG_FLUSH_ALL) {
-                id->active_area = 0;
-                memset(id->active_entries, 0, id->nr_areas * sizeof(int));
-                for (i = 0; i < id->nr_areas; i++) {
+		id->active_area = 0;
+		memset(id->active_entries, 0, id->nr_areas * sizeof(int));
+		for (i = 0; i < id->nr_areas; i++) {
 			id->active_pages[i] = 0;
 			for (j = 0; j < id->pages_per_area; j++)
-                        	memset(id->areas[i][j], 0, PAGE_SIZE);
+				memset(id->areas[i][j], 0, PAGE_SIZE);
 		}
 	} else if (area >= 0 && area < id->nr_areas) {
-                id->active_entries[area] = 0;
+		id->active_entries[area] = 0;
 		id->active_pages[area] = 0;
 		for (i = 0; i < id->pages_per_area; i++)
 			memset(id->areas[area][i], 0, PAGE_SIZE);
-        }
+	}
 	spin_unlock_irqrestore(&id->lock, flags);
 }
 
@@ -1346,35 +1345,36 @@ debug_input_flush_fn(debug_info_t * id, struct debug_view *view,
 			struct file *file, const char __user *user_buf,
 			size_t user_len, loff_t * offset)
 {
-        char input_buf[1];
-        int rc = user_len;
+	char input_buf[1];
+	int rc = user_len;
 
 	if (user_len > 0x10000)
-                user_len = 0x10000;
+		user_len = 0x10000;
 	if (*offset != 0) {
 		rc = -EPIPE;
-                goto out;
+		goto out;
 	}
 	if (copy_from_user(input_buf, user_buf, 1)) {
-                rc = -EFAULT;
-                goto out;
-        }
+		rc = -EFAULT;
+		goto out;
+	}
 	if (input_buf[0] == '-') {
-                debug_flush(id, DEBUG_FLUSH_ALL);
-                goto out;
-        }
-        if (isdigit(input_buf[0])) {
-                int area = ((int) input_buf[0] - (int) '0');
-                debug_flush(id, area);
-                goto out;
-        }
+		debug_flush(id, DEBUG_FLUSH_ALL);
+		goto out;
+	}
+	if (isdigit(input_buf[0])) {
+		int area = ((int) input_buf[0] - (int) '0');
+
+		debug_flush(id, area);
+		goto out;
+	}
 
 	pr_info("Flushing debug data failed because %c is not a valid "
 		 "area\n", input_buf[0]);
 
 out:
-        *offset += user_len;
-        return rc;              /* number of input characters */
+	*offset += user_len;
+	return rc;              /* number of input characters */
 }
 
 /*
@@ -1385,11 +1385,11 @@ static int
 debug_raw_header_fn(debug_info_t * id, struct debug_view *view,
 			int area, debug_entry_t * entry, char *out_buf)
 {
-        int rc;
+	int rc;
 
 	rc = sizeof(debug_entry_t);
 	memcpy(out_buf, entry, sizeof(debug_entry_t));
-        return rc;
+	return rc;
 }
 
 /*
@@ -1418,8 +1418,8 @@ debug_hex_ascii_format_fn(debug_info_t * id, struct debug_view *view,
 	int i, rc = 0;
 
 	for (i = 0; i < id->buf_size; i++)
-                rc += sprintf(out_buf + rc, "%02x ",
-                              ((unsigned char *) in_buf)[i]);
+		rc += sprintf(out_buf + rc, "%02x ",
+			      ((unsigned char *) in_buf)[i]);
 	rc += sprintf(out_buf + rc, "| ");
 	for (i = 0; i < id->buf_size; i++) {
 		unsigned char c = in_buf[i];
-- 
2.9.3

[PATCH 12/17] s390/debug: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 08:45:26 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 4a12faf..a9fcf72 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1162,13 +1162,9 @@ debug_get_user_string(const char __user *user_buf, size_t user_len)
 {
 	char* buffer;
 
-	buffer = kmalloc(user_len + 1, GFP_KERNEL);
-	if (!buffer)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(buffer, user_buf, user_len) != 0) {
-		kfree(buffer);
-		return ERR_PTR(-EFAULT);
-	}
+	buffer = memdup_user(user_buf, user_len + 1);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
 	/* got the string, now strip linefeed. */
 	if (buffer[user_len - 1] == '\n')
 		buffer[user_len - 1] = 0;
-- 
2.9.3

[PATCH 13/17] s390/debug: Improve a size determination in debug_open()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 09:02:22 +0200

* Replace the specification of a data type by a pointer dereference
  as the parameter for the operator "sizeof" to make the corresponding size
  determination a bit safer according to the Linux coding style convention.

* Improve source code layout for one function call.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index a9fcf72..32ceb5e 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -636,8 +636,7 @@ debug_open(struct inode *inode, struct file *file)
 		rc = -ENOMEM;
 		goto unlock;
 	}
-	p_info = kmalloc(sizeof(file_private_info_t),
-						GFP_KERNEL);
+	p_info = kmalloc(sizeof(*p_info), GFP_KERNEL);
 	if (!p_info) {
 		debug_info_free(debug_info_snapshot);
 		rc = -ENOMEM;
-- 
2.9.3

[PATCH 14/17] s390/debug: Improve a size determination in debug_sprintf_format_fn()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 14:32:08 +0200

Replace a multiplication by a reference for a local array variable
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 32ceb5e..6792a9c 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1486,7 +1486,7 @@ debug_sprintf_format_fn(debug_info_t * id, struct debug_view *view,
 	/* number of arguments used for sprintf (without the format string) */
 	num_used_args   = min(DEBUG_SPRINTF_MAX_ARGS, (num_longs - 1));
 
-	memset(index, 0, DEBUG_SPRINTF_MAX_ARGS * sizeof(int));
+	memset(index, 0, sizeof(index));
 
 	for (i = 0; i < num_used_args; i++)
 		index[i] = i;
-- 
2.9.3

[PATCH 15/17] s390/debug: Improve a size determination in debug_raw_header_fn()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 14:34:22 +0200

* Replace the specification of a data type by pointer dereferences
  as the parameter for the operator "sizeof" to make the corresponding size
  determination a bit safer according to the Linux coding style convention.

* Return a constant directly without storing the desired value
  in a local variable.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 6792a9c..ff8e705 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1380,11 +1380,8 @@ static int
 debug_raw_header_fn(debug_info_t * id, struct debug_view *view,
 			int area, debug_entry_t * entry, char *out_buf)
 {
-	int rc;
-
-	rc = sizeof(debug_entry_t);
-	memcpy(out_buf, entry, sizeof(debug_entry_t));
-	return rc;
+	memcpy(out_buf, entry, sizeof(*entry));
+	return sizeof(*entry);
 }
 
 /*
-- 
2.9.3

[PATCH 16/17] s390/debug: Improve determination of sizes in debug_info_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 14:39:00 +0200

* Replace two multiplications by references for an array in a local
  data structure as the parameter for the operator "sizeof" to make
  the corresponding size determination a bit safer.

* Improve source code layout for one function call.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index ff8e705..408a8da 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -269,9 +269,8 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 	rc->buf_size       = buf_size;
 	rc->entry_size     = sizeof(debug_entry_t) + buf_size;
 	strlcpy(rc->name, name, sizeof(rc->name));
-	memset(rc->views, 0, DEBUG_MAX_VIEWS * sizeof(struct debug_view *));
-	memset(rc->debugfs_entries, 0, DEBUG_MAX_VIEWS *
-		sizeof(struct dentry*));
+	memset(rc->views, 0, sizeof(rc->views));
+	memset(rc->debugfs_entries, 0, sizeof(rc->debugfs_entries));
 	atomic_set(&(rc->ref_count), 0);
 
 	return rc;
-- 
2.9.3

[PATCH 17/17] s390/debug: Improve another size determination in debug_info_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 2 Sep 2016 14:41:02 +0200

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/debug.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 408a8da..ffc6af6 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -241,8 +241,7 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 	debug_info_t* rc;
 
 	/* alloc everything */
-
-	rc = kmalloc(sizeof(debug_info_t), GFP_KERNEL);
+	rc = kmalloc(sizeof(*rc), GFP_KERNEL);
 	if(!rc)
 		goto exit;
 	rc->active_entries = kcalloc(nr_areas, sizeof(int), GFP_KERNEL);
-- 
2.9.3

Re: [PATCH 09/17] s390/debug: Return directly if a null pointer was passed to debug_unregister()

[walter harms]

Am 03.09.2016 14:24, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 1 Sep 2016 19:34:45 +0200
> 
> Return directly at the beginning if a null pointer was passed for
> the input parameter of this function.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kernel/debug.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
> index c4a4641..d137150 100644
> --- a/arch/s390/kernel/debug.c
> +++ b/arch/s390/kernel/debug.c
> @@ -736,13 +736,10 @@ void
>  debug_unregister(debug_info_t * id)
>  {
>  	if (!id)
> -		goto out;
> +		return;
>  	mutex_lock(&debug_mutex);
>  	debug_info_put(id);
>  	mutex_unlock(&debug_mutex);
> -
> -out:
> -	return;
>  }
>  EXPORT_SYMBOL(debug_unregister);
> 

debug_info_put() will check for NULL, the whole check can be removed.

re,
 wh

Re: [PATCH 12/17] s390/debug: Use memdup_user() rather than duplicating its implementation

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

Hi Markus,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.8-rc4 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/s390-debug-Fine-tuning-for-several-function-implementations/20160903-204622
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All warnings (new ones prefixed by >>):

   arch/s390/kernel/debug.c: In function 'debug_get_user_string':
>> arch/s390/kernel/debug.c:1167:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
      return PTR_ERR(buffer);
             ^

vim +1167 arch/s390/kernel/debug.c

  1151			id->debugfs_entries[i] = NULL;
  1152		}
  1153		spin_unlock_irqrestore(&id->lock, flags);
  1154		debugfs_remove(dentry);
  1155	out:
  1156		return rc;
  1157	}
  1158	EXPORT_SYMBOL(debug_unregister_view);
  1159	
  1160	static inline char *
  1161	debug_get_user_string(const char __user *user_buf, size_t user_len)
  1162	{
  1163		char* buffer;
  1164	
  1165		buffer = memdup_user(user_buf, user_len + 1);
  1166		if (IS_ERR(buffer))
> 1167			return PTR_ERR(buffer);
  1168		/* got the string, now strip linefeed. */
  1169		if (buffer[user_len - 1] == '\n')
  1170			buffer[user_len - 1] = 0;
  1171		else
  1172			buffer[user_len] = 0;
  1173		return buffer;
  1174	}
  1175	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 42341 bytes --]

Re: [PATCH 00/17] s390/debug: Fine-tuning for several function implementations

[Martin Schwidefsky]

On Sat, 3 Sep 2016 14:04:18 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 3 Sep 2016 13:54:32 +0200
> 
> Several update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (17):
>   Use kmalloc_array() in debug_areas_alloc()
>   Delete unnecessary braces
>   Add some spaces for better code readability
>   Rename jump labels in debug_areas_alloc()
>   Fix jump targets in debug_info_alloc()
>   Rename jump labels in debug_info_copy()
>   Rename jump labels in debug_open()
>   Fix a jump target in debug_register_mode()
>   Return directly if a null pointer was passed to debug_unregister()
>   Delete an unnecessary initialisation in debug_prolog_level_fn()
>   Fix indentation in 13 functions
>   Use memdup_user() rather than duplicating its implementation
>   Improve a size determination in debug_open()
>   Improve a size determination in debug_sprintf_format_fn()
>   Improve a size determination in debug_raw_header_fn()
>   Improve determination of sizes in debug_info_alloc()
>   Improve another size determination in debug_info_alloc()
> 
>  arch/s390/kernel/debug.c | 433 ++++++++++++++++++++++-------------------------
>  1 file changed, 204 insertions(+), 229 deletions(-)
 
While I agree that the old code in arch/s390/kernel/debug.c does not abide to
the current coding style standards, I doubt there is much value in these patches.
To be honest I got annoyed after the third patch and stopped reading after
the forth. 

NAK.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: s390/debug: Fine-tuning for several function implementations

[SF Markus Elfring]

> While I agree that the old code in arch/s390/kernel/debug.c does not abide to
> the current coding style standards, I doubt there is much value in these patches.

How do you value the recommended compliance with the current Linux coding style convention?

Will my contribution be useful for further considerations?


> To be honest I got annoyed after the third patch

Thanks for your response.


> and stopped reading after the forth.

Does anybody in your company care for further improvements also in this software module?

Are there still opportunities to continue development in more constructive ways here?

Regards,
Markus

Re: s390/debug: Fine-tuning for several function implementations

[SF Markus Elfring]

> While I agree that the old code in arch/s390/kernel/debug.c does not abide to
> the current coding style standards,

Thanks for this kind of acknowledgement.

Is such an information worth for further development considerations?


> I doubt there is much value in these patches.

I assume that your doubts could be adjusted, couldn't they?

Are there any other concerns involved in the background?


> To be honest I got annoyed after the third patch

Which of the proposed changes did trigger such a reaction?


I find this response also a bit surprising because of the aspect
that I offered you some results from my work as a free software developer.


> and stopped reading after the forth.

I imagine that you could have aborted the review of my update suggestions
a bit too early for your debug software module.

I agree that the value is varying for the presented 17 update steps.
But I hope that their value is potentially bigger overall
than you categorise them at first glance.

Now I would like to try to get a bit of your software development attention
once more for two of them at least. I hope that it can be easier to clarify
their value.

* Do the implementations of the functions "debug_areas_alloc"
  and "debug_get_user_string" need another look together with a more detailed
  source code review?

* How do you think about to use functions like "kmalloc_array"
  and "memdup_user" there instead?

Regards,
Markus