Re: [v3] Coccinelle: suggest replacing strncpy+truncation by strscpy

[SF Markus Elfring]

> The problem is that I don't know if the script is correct

Will the clarification of such a concern evolve into another interesting
software development adventure?


> - I'm not familiar with these string functions.

How are the chances to improve the understanding of affected programming
interfaces anyhow?
https://elixir.bootlin.com/linux/v4.18-rc5/source/lib/string.c#L154
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/string.c?id=28c20cc73b9cc4288c86c2a3fc62af4087de4b19#n154

Regards,
Markus

Re: [v3] Coccinelle: suggest replacing strncpy+truncation by strscpy

[SF Markus Elfring]

> The problem is that I don't know if the script is correct

Will the clarification of such a concern evolve into another interesting
software development adventure?


> - I'm not familiar with these string functions.

How are the chances to improve the understanding of affected programming
interfaces anyhow?
https://elixir.bootlin.com/linux/v4.18-rc5/source/lib/string.c#L154
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/string.c?id=28c20cc73b9cc4288c86c2a3fc62af4087de4b19#n154

Regards,
Markus

[PATCH 4/6] Coccinelle: atomic_as_refcounter: Replace disjunction by a constraint in two SmPL rules

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 2 Jul 2018 18:45:15 +0200

Three function names were specified for a search of function calls
by the means of a disjunction in two rules of a script for
the semantic patch language.
Use a regular expression as a constraint for this source code search
pattern instead so that duplication of SmPL code can be avoided.

Signed-off-by: Markus Elfring 
---
 .../coccinelle/api/atomic_as_refcounter.cocci | 21 +--
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci 
b/scripts/coccinelle/api/atomic_as_refcounter.cocci
index 57af2db9463e..372ae99591fd 100644
--- a/scripts/coccinelle/api/atomic_as_refcounter.cocci
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -80,17 +80,11 @@ coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r2 exists@
 expression a;
-identifier x;
+identifier F =~ "^atomic(?:64|_long)?_add_unless$", x;
 position p1;
 @@
 
-(
-atomic_add_unless(&(a)->x,-1,1)@p1
-|
-atomic_long_add_unless(&(a)->x,-1,1)@p1
-|
-atomic64_add_unless(&(a)->x,-1,1)@p1
-)
+ F(&(a)->x, -1, 1)@p1
 
 @script:python depends on report@
 p1 << r2.p1;
@@ -99,17 +93,12 @@ msg = "atomic_add_unless"
 coccilib.report.print_report(p1[0], msg)
 
 @r3 exists@
-identifier x;
+expression E;
+identifier F =~ "^atomic(?:64|_long)?_add_return$";
 position p1;
 @@
 
-(
-x = atomic_add_return@p1(-1, ...);
-|
-x = atomic_long_add_return@p1(-1, ...);
-|
-x = atomic64_add_return@p1(-1, ...);
-)
+ E = F@p1(-1, ...);
 
 @script:python depends on report@
 p1 << r3.p1;
-- 
2.18.0

[PATCH 4/6] Coccinelle: atomic_as_refcounter: Replace disjunction by a constraint in two SmPL rules

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 2 Jul 2018 18:45:15 +0200

Three function names were specified for a search of function calls
by the means of a disjunction in two rules of a script for
the semantic patch language.
Use a regular expression as a constraint for this source code search
pattern instead so that duplication of SmPL code can be avoided.

Signed-off-by: Markus Elfring 
---
 .../coccinelle/api/atomic_as_refcounter.cocci | 21 +--
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci 
b/scripts/coccinelle/api/atomic_as_refcounter.cocci
index 57af2db9463e..372ae99591fd 100644
--- a/scripts/coccinelle/api/atomic_as_refcounter.cocci
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -80,17 +80,11 @@ coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r2 exists@
 expression a;
-identifier x;
+identifier F =~ "^atomic(?:64|_long)?_add_unless$", x;
 position p1;
 @@
 
-(
-atomic_add_unless(&(a)->x,-1,1)@p1
-|
-atomic_long_add_unless(&(a)->x,-1,1)@p1
-|
-atomic64_add_unless(&(a)->x,-1,1)@p1
-)
+ F(&(a)->x, -1, 1)@p1
 
 @script:python depends on report@
 p1 << r2.p1;
@@ -99,17 +93,12 @@ msg = "atomic_add_unless"
 coccilib.report.print_report(p1[0], msg)
 
 @r3 exists@
-identifier x;
+expression E;
+identifier F =~ "^atomic(?:64|_long)?_add_return$";
 position p1;
 @@
 
-(
-x = atomic_add_return@p1(-1, ...);
-|
-x = atomic_long_add_return@p1(-1, ...);
-|
-x = atomic64_add_return@p1(-1, ...);
-)
+ E = F@p1(-1, ...);
 
 @script:python depends on report@
 p1 << r3.p1;
-- 
2.18.0

[PATCH 3/6] Coccinelle: atomic_as_refcounter: Use type “expression” for another metavariable

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 2 Jul 2018 17:55:27 +0200

The metavariable “a” is enclosed by parentheses in three rules of
a script for the semantic patch language.
Replace its type by “expression” so that the corresponding source code
search becomes more powerful.

Signed-off-by: Markus Elfring 
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci 
b/scripts/coccinelle/api/atomic_as_refcounter.cocci
index 5571eea04c7b..57af2db9463e 100644
--- a/scripts/coccinelle/api/atomic_as_refcounter.cocci
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -10,7 +10,8 @@
 virtual report
 
 @r1 exists@
-identifier a, x;
+expression a;
+identifier x;
 position p1, p2;
 identifier fname =~ "free";
 identifier fname2 =~ "(?:call_rcu|de(?:l|stroy)|(?:queue|schedule)_work)";
@@ -45,7 +46,8 @@ msg = "atomic_dec_and_test variation before object free at 
line %s."
 coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r4 exists@
-identifier a, x, y;
+expression a;
+identifier x, y;
 position p1, p2;
 identifier fname =~ "free";
 @@
@@ -77,7 +79,8 @@ msg = "atomic_dec_and_test variation before object free at 
line %s."
 coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r2 exists@
-identifier a, x;
+expression a;
+identifier x;
 position p1;
 @@
 
-- 
2.18.0

[PATCH 3/6] Coccinelle: atomic_as_refcounter: Use type “expression” for another metavariable

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 2 Jul 2018 17:55:27 +0200

The metavariable “a” is enclosed by parentheses in three rules of
a script for the semantic patch language.
Replace its type by “expression” so that the corresponding source code
search becomes more powerful.

Signed-off-by: Markus Elfring 
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci 
b/scripts/coccinelle/api/atomic_as_refcounter.cocci
index 5571eea04c7b..57af2db9463e 100644
--- a/scripts/coccinelle/api/atomic_as_refcounter.cocci
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -10,7 +10,8 @@
 virtual report
 
 @r1 exists@
-identifier a, x;
+expression a;
+identifier x;
 position p1, p2;
 identifier fname =~ "free";
 identifier fname2 =~ "(?:call_rcu|de(?:l|stroy)|(?:queue|schedule)_work)";
@@ -45,7 +46,8 @@ msg = "atomic_dec_and_test variation before object free at 
line %s."
 coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r4 exists@
-identifier a, x, y;
+expression a;
+identifier x, y;
 position p1, p2;
 identifier fname =~ "free";
 @@
@@ -77,7 +79,8 @@ msg = "atomic_dec_and_test variation before object free at 
line %s."
 coccilib.report.print_report(p1[0], msg % (p2[0].line))
 
 @r2 exists@
-identifier a, x;
+expression a;
+identifier x;
 position p1;
 @@
 
-- 
2.18.0

[PATCH 0/6] Coccinelle: atomic_as_refcounter: Improvements for source code search specifications

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 3 Jul 2018 09:15:26 +0200

This source code search pattern was programmed in the way that
some implementation details could be improved further.
I suggest to avoid unnecessary code repetition also in this script
for the semantic patch language.

Markus Elfring (6):
  Omit placeholder specifications from two SmPL constraints
  Optimise a disjunction in the first SmPL rule
  Use type “expression” for another metavariable
  Replace disjunction by a constraint in two SmPL rules
  Use nested disjunctions in two SmPL rules
  Use format strings directly in SmPL rules

 .../coccinelle/api/atomic_as_refcounter.cocci | 104 +++---
 1 file changed, 39 insertions(+), 65 deletions(-)

-- 
2.18.0

[PATCH 0/6] Coccinelle: atomic_as_refcounter: Improvements for source code search specifications

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 3 Jul 2018 09:15:26 +0200

This source code search pattern was programmed in the way that
some implementation details could be improved further.
I suggest to avoid unnecessary code repetition also in this script
for the semantic patch language.

Markus Elfring (6):
  Omit placeholder specifications from two SmPL constraints
  Optimise a disjunction in the first SmPL rule
  Use type “expression” for another metavariable
  Replace disjunction by a constraint in two SmPL rules
  Use nested disjunctions in two SmPL rules
  Use format strings directly in SmPL rules

 .../coccinelle/api/atomic_as_refcounter.cocci | 104 +++---
 1 file changed, 39 insertions(+), 65 deletions(-)

-- 
2.18.0

Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family

[SF Markus Elfring]

>> * The repetition of such a constraint in subsequent SmPL rules could be 
>> avoided
>>   if inheritance will be used for this metavariable.
> 
> This is quite incorrect.

I suggest to consider additional software design options.


> Inheritance is only possible when a match of the previous rule has succeeded.

I agree with this information.


> If a rule never applies in a given file, the rules that inherit from it
> won't apply either.

I would like to point the possibility out to specify a source code search
which will find interesting function calls at least by an inital SmPL rule.


> Furthermore, what is inherited is the value, not the constraint.

This technical detail can be fine.


> If the original binding of alloc only ever matches kmalloc,
> then the inherited references will only match kmalloc too.

Can the desired search pattern be extended in significant ways?

Regards,
Markus

Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family

[SF Markus Elfring]

>> * The repetition of such a constraint in subsequent SmPL rules could be 
>> avoided
>>   if inheritance will be used for this metavariable.
> 
> This is quite incorrect.

I suggest to consider additional software design options.


> Inheritance is only possible when a match of the previous rule has succeeded.

I agree with this information.


> If a rule never applies in a given file, the rules that inherit from it
> won't apply either.

I would like to point the possibility out to specify a source code search
which will find interesting function calls at least by an inital SmPL rule.


> Furthermore, what is inherited is the value, not the constraint.

This technical detail can be fine.


> If the original binding of alloc only ever matches kmalloc,
> then the inherited references will only match kmalloc too.

Can the desired search pattern be extended in significant ways?

Regards,
Markus

Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family

[SF Markus Elfring]

> For kmalloc()-family allocations, instead of A * B, use array_size().
> Similarly, instead of A * B *C, use array3_size().

It took a while until my software development attention was caught also
by this update suggestion.


> Note that:
>   kmalloc(array_size(a, b), ...);
> could be written as:
>   kmalloc_array(a, b, ...)
> (and similarly, kzalloc(array_size(a, b), ...) as kcalloc(a, b, ...))

This is good to know, isn't it?


> but this made the Coccinelle script WAY larger.

Such a consequence is usual when the corresponding software development
challenges grow.
Are there further approaches to consider?


> There is no desire to replace kmalloc_array() with kmalloc(array_size(...), 
> ...),
> but given the number of changes made treewide,

The number of changed source files can be impressive overall.


> I opted for Coccinelle simplicity.

I suggest to reconsider corresponding consequences.

I find that an important aspect can be missing in this commit description then.
How would you like to determine if the array size calculation (multiplication)
should be performed together with an overflow check (or not)?

How do you think about to express such a case distinction also in a bigger
script for the semantic patch language?


> This also tries to isolate sizeof() and constant factors, in an attempt
> to regularize the argument ordering.

This desire is reasonable.


> Automatically generated using the following Coccinelle script:

I have taken another look at implementation details.

* My view might not matter for the generated changes from this run
  of a limited SmPL script.

* My suggestions will influence the run time characteristics if such a source
  code transformation pattern will be executed again.


> // 2-factor product with sizeof(variable)
> @@
> identifier alloc =~ "kmalloc|kzalloc|kvmalloc|kvzalloc";

* This regular expression could be optimised to the specification 
“kv?[mz]alloc”.
  Extensions will be useful for further function names.

* The repetition of such a constraint in subsequent SmPL rules could be avoided
  if inheritance will be used for this metavariable.


> expression GFP, THING;
> identifier COUNT;
> @@
>
> - alloc(sizeof(THING) * COUNT, GFP)
> + alloc(array_size(COUNT, sizeof(THING)), GFP)

More change items are specified here than what would be essentially necessary.
* Function name
* Second parameter

This can be a design option to give the Coccinelle software the opportunity
for additional source code formatting (pretty printing).


These SmPL rules were designed in the way so far that they are independent
from previous rules. This approach contains the risk that a metavariable type
like “expression” can match more source code than it was expected.
This technical detail matters for the selection of the replacement 
“array3_size”.


The comments in the script indicate a desire for specific case distinctions.
I have got the impression that the use of SmPL disjunctions will be more
appropriate for the desired condition checks.
A priority could be specified then for involved pattern evaluation.

Would you like to adjust the transformation pattern any further?

Regards,
Markus

Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family

[SF Markus Elfring]

> For kmalloc()-family allocations, instead of A * B, use array_size().
> Similarly, instead of A * B *C, use array3_size().

It took a while until my software development attention was caught also
by this update suggestion.


> Note that:
>   kmalloc(array_size(a, b), ...);
> could be written as:
>   kmalloc_array(a, b, ...)
> (and similarly, kzalloc(array_size(a, b), ...) as kcalloc(a, b, ...))

This is good to know, isn't it?


> but this made the Coccinelle script WAY larger.

Such a consequence is usual when the corresponding software development
challenges grow.
Are there further approaches to consider?


> There is no desire to replace kmalloc_array() with kmalloc(array_size(...), 
> ...),
> but given the number of changes made treewide,

The number of changed source files can be impressive overall.


> I opted for Coccinelle simplicity.

I suggest to reconsider corresponding consequences.

I find that an important aspect can be missing in this commit description then.
How would you like to determine if the array size calculation (multiplication)
should be performed together with an overflow check (or not)?

How do you think about to express such a case distinction also in a bigger
script for the semantic patch language?


> This also tries to isolate sizeof() and constant factors, in an attempt
> to regularize the argument ordering.

This desire is reasonable.


> Automatically generated using the following Coccinelle script:

I have taken another look at implementation details.

* My view might not matter for the generated changes from this run
  of a limited SmPL script.

* My suggestions will influence the run time characteristics if such a source
  code transformation pattern will be executed again.


> // 2-factor product with sizeof(variable)
> @@
> identifier alloc =~ "kmalloc|kzalloc|kvmalloc|kvzalloc";

* This regular expression could be optimised to the specification 
“kv?[mz]alloc”.
  Extensions will be useful for further function names.

* The repetition of such a constraint in subsequent SmPL rules could be avoided
  if inheritance will be used for this metavariable.


> expression GFP, THING;
> identifier COUNT;
> @@
>
> - alloc(sizeof(THING) * COUNT, GFP)
> + alloc(array_size(COUNT, sizeof(THING)), GFP)

More change items are specified here than what would be essentially necessary.
* Function name
* Second parameter

This can be a design option to give the Coccinelle software the opportunity
for additional source code formatting (pretty printing).


These SmPL rules were designed in the way so far that they are independent
from previous rules. This approach contains the risk that a metavariable type
like “expression” can match more source code than it was expected.
This technical detail matters for the selection of the replacement 
“array3_size”.


The comments in the script indicate a desire for specific case distinctions.
I have got the impression that the use of SmPL disjunctions will be more
appropriate for the desired condition checks.
A priority could be specified then for involved pattern evaluation.

Would you like to adjust the transformation pattern any further?

Regards,
Markus

Re: [v3] [media] Use common error handling code in 19 functions

[SF Markus Elfring]

> @@ -656,18 +656,18 @@  static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
>   tsfeed->priv = filter;
>  
>   ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>  
>   ret = tsfeed->start_filtering(tsfeed);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>  
>   return 0;
> +
> +release_feed:
> + dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> + return ret;
>  }
> 
> There's *nothing* wrong with the above. It works fine,

I can agree to this view in principle according to the required control flow.


> avoids goto

How does this wording fit to information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”?


> and probably even produce the same code, as gcc will likely optimize it.

Would you like to clarify the current situation around supported
software optimisations any more?


> It is also easier to review, as the error handling is closer
> to the code.

Do we stumble on different coding style preferences once more?


> On the other hand, there's nothing wrong on taking the approach
> you're proposing.

Thanks for another bit of positive feedback.


> In the end, using goto or not on error handling like the above is 
> a matter of personal taste - and taste changes with time

Do Linux guidelines need any adjustments?


> and with developer. I really don't have time to keep reviewing patches
> that are just churning the code just due to someone's personal taste.

I tried to apply another general source code transformation pattern.


> I'm pretty sure if I start accepting things like that,
> someone else would be on some future doing patches just reverting it,
> and I would be likely having to apply them too.

Why?

I hope also that the source code can be kept consistent to some degree.


> So, except if the patch is really fixing something - e.g. a broken
> error handling code, I'll just ignore such patches and mark as
> rejected without further notice/comments from now on.

I would find such a communication style questionable.
Do you distinguish between bug fixes and possible corrections for
other error categories (or software weaknesses)?

Regards,
Markus

Re: [v3] [media] Use common error handling code in 19 functions

[SF Markus Elfring]

> @@ -656,18 +656,18 @@  static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
>   tsfeed->priv = filter;
>  
>   ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>  
>   ret = tsfeed->start_filtering(tsfeed);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>  
>   return 0;
> +
> +release_feed:
> + dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> + return ret;
>  }
> 
> There's *nothing* wrong with the above. It works fine,

I can agree to this view in principle according to the required control flow.


> avoids goto

How does this wording fit to information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”?


> and probably even produce the same code, as gcc will likely optimize it.

Would you like to clarify the current situation around supported
software optimisations any more?


> It is also easier to review, as the error handling is closer
> to the code.

Do we stumble on different coding style preferences once more?


> On the other hand, there's nothing wrong on taking the approach
> you're proposing.

Thanks for another bit of positive feedback.


> In the end, using goto or not on error handling like the above is 
> a matter of personal taste - and taste changes with time

Do Linux guidelines need any adjustments?


> and with developer. I really don't have time to keep reviewing patches
> that are just churning the code just due to someone's personal taste.

I tried to apply another general source code transformation pattern.


> I'm pretty sure if I start accepting things like that,
> someone else would be on some future doing patches just reverting it,
> and I would be likely having to apply them too.

Why?

I hope also that the source code can be kept consistent to some degree.


> So, except if the patch is really fixing something - e.g. a broken
> error handling code, I'll just ignore such patches and mark as
> rejected without further notice/comments from now on.

I would find such a communication style questionable.
Do you distinguish between bug fixes and possible corrections for
other error categories (or software weaknesses)?

Regards,
Markus

Re: [v3] [media] Use common error handling code in 19 functions

[SF Markus Elfring]

> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.

Why was this update suggestion rejected once more a moment ago?

https://patchwork.linuxtv.org/patch/47827/
lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c...@users.sourceforge.net>
https://lkml.org/lkml/2018/3/9/823

Would you like to integrate such a source code transformation after any further 
adjustments?

Regards,
Markus

Re: [v3] [media] Use common error handling code in 19 functions

[SF Markus Elfring]

> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.

Why was this update suggestion rejected once more a moment ago?

https://patchwork.linuxtv.org/patch/47827/
lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c...@users.sourceforge.net>
https://lkml.org/lkml/2018/3/9/823

Would you like to integrate such a source code transformation after any further 
adjustments?

Regards,
Markus

Re: [0/9] UML vector network driver: Adjustments for three function implementations

[SF Markus Elfring]

> Date: Sun, 11 Mar 2018 16:06:16 +0100
> 
> Some update suggestions were taken into account
> from static source code analysis.
…
>   Delete unnecessary code in user_init_raw_fds()
>   Less checks in user_init_raw_fds() after error detection
>   Adjust an error message in user_init_socket_fds()
>   Delete an unnecessary check before kfree() in user_init_socket_fds()
>   Delete two unnecessary checks before freeaddrinfo() in 
> user_init_socket_fds()
>   Less checks in user_init_socket_fds() after error detection
>   Adjust an error message in user_init_tap_fds()
>   Less checks in user_init_tap_fds() after error detection
>   Delete an unnecessary variable initialisation in user_init_tap_fds()
…

Would you like to pick any idea up from this patch series?

Regards,
Markus

Re: [0/9] UML vector network driver: Adjustments for three function implementations

[SF Markus Elfring]

> Date: Sun, 11 Mar 2018 16:06:16 +0100
> 
> Some update suggestions were taken into account
> from static source code analysis.
…
>   Delete unnecessary code in user_init_raw_fds()
>   Less checks in user_init_raw_fds() after error detection
>   Adjust an error message in user_init_socket_fds()
>   Delete an unnecessary check before kfree() in user_init_socket_fds()
>   Delete two unnecessary checks before freeaddrinfo() in 
> user_init_socket_fds()
>   Less checks in user_init_socket_fds() after error detection
>   Adjust an error message in user_init_tap_fds()
>   Less checks in user_init_tap_fds() after error detection
>   Delete an unnecessary variable initialisation in user_init_tap_fds()
…

Would you like to pick any idea up from this patch series?

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

>> The mutex was (and is still only) locked within case branches, isn't it?
>>
> You are correct, this does however reflect the issue with the resulting
> lack of balance here.

Do you find changes for the other function implementations easier to integrate?

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

>> The mutex was (and is still only) locked within case branches, isn't it?
>>
> You are correct, this does however reflect the issue with the resulting
> lack of balance here.

Do you find changes for the other function implementations easier to integrate?

Regards,
Markus

Re: [2/4] scsi: hpsa: Less function calls in hpsa_big_passthru_ioctl() after error detection

[SF Markus Elfring]

>> @@ -6501,14 +6501,16 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info
>> *h, void __user *argp)
>>  cleanup0:
>> cmd_free(h, c);
>>  cleanup1:
>> -   if (buff) {
>> +   {
>> int i;
>>
>> for (i = 0; i < sg_used; i++)
>> kfree(buff[i]);
>> kfree(buff);
>> }
> 
> Thanks for looking at the hpsa driver.
> 
> This HUNK ends up with an unnamed block.

Which identifier would you like to use there?


> I would prefer to not have it structured like this.

Would you like to show a source code layout alternative?

Regards,
Markus

Re: [2/4] scsi: hpsa: Less function calls in hpsa_big_passthru_ioctl() after error detection

[SF Markus Elfring]

>> @@ -6501,14 +6501,16 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info
>> *h, void __user *argp)
>>  cleanup0:
>> cmd_free(h, c);
>>  cleanup1:
>> -   if (buff) {
>> +   {
>> int i;
>>
>> for (i = 0; i < sg_used; i++)
>> kfree(buff[i]);
>> kfree(buff);
>> }
> 
> Thanks for looking at the hpsa driver.
> 
> This HUNK ends up with an unnamed block.

Which identifier would you like to use there?


> I would prefer to not have it structured like this.

Would you like to show a source code layout alternative?

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

>> The mutex was (and is still only) locked within case branches, isn't it?
>>
> You are correct, this does however reflect the issue with the resulting
> lack of balance here.

I suggest to reconsider affected software aspects a bit more.


> I saw the mutex was getting unlocked outside the local scope and so assumed
> that it was also take outside the local scope.

Assumptions and corresponding expectations might need further clarifications.


> That isn't true, so we have hurt readability.

Does your conclusion need any adjustment?


> I read it quickly and got the wrong idea which generally implies it is not
> as clear as we would like.
> 
> Hence this change isn't going anywhere I'm afraid.

I imagine that more time will be needed then to get used to additional 
adjustments
of implementation details in these functions.

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

>> The mutex was (and is still only) locked within case branches, isn't it?
>>
> You are correct, this does however reflect the issue with the resulting
> lack of balance here.

I suggest to reconsider affected software aspects a bit more.


> I saw the mutex was getting unlocked outside the local scope and so assumed
> that it was also take outside the local scope.

Assumptions and corresponding expectations might need further clarifications.


> That isn't true, so we have hurt readability.

Does your conclusion need any adjustment?


> I read it quickly and got the wrong idea which generally implies it is not
> as clear as we would like.
> 
> Hence this change isn't going anywhere I'm afraid.

I imagine that more time will be needed then to get used to additional 
adjustments
of implementation details in these functions.

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

Am 17.03.2018 um 20:54 schrieb Jonathan Cameron:
> On Wed, 14 Mar 2018 16:15:32 +0100
> SF Markus Elfring <elfr...@users.sourceforge.net> wrote:
> 
>> From: Markus Elfring <elfr...@users.sourceforge.net>
>> Date: Wed, 14 Mar 2018 16:06:49 +0100
>>
>> * Add jump targets so that a call of the function "mutex_unlock" is stored
>>   only once in these function implementations.
>>
>> * Replace 19 calls by goto statements.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> 
> Hi Markus,
> 
> Some of these are good and sensible changes

Such feedback is nice.


> - others break the code.

Which concrete places do you find questionable here?


>> -return ret;
>> +
>> +goto set_power_state;
>>  default:
>>  return -EINVAL;
> We exit with the mutex locked now and it should not be.

I wonder about your source code interpretation here.
The mutex was (and is still only) locked within case branches, isn't it?


> 
>>  }
>>  
>>  return -EINVAL;
> Mutex is still locked here and the return is wrong.

Should this statement get any more software development attention?

Regards,
Markus

Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

Am 17.03.2018 um 20:54 schrieb Jonathan Cameron:
> On Wed, 14 Mar 2018 16:15:32 +0100
> SF Markus Elfring  wrote:
> 
>> From: Markus Elfring 
>> Date: Wed, 14 Mar 2018 16:06:49 +0100
>>
>> * Add jump targets so that a call of the function "mutex_unlock" is stored
>>   only once in these function implementations.
>>
>> * Replace 19 calls by goto statements.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring 
> 
> Hi Markus,
> 
> Some of these are good and sensible changes

Such feedback is nice.


> - others break the code.

Which concrete places do you find questionable here?


>> -return ret;
>> +
>> +goto set_power_state;
>>  default:
>>  return -EINVAL;
> We exit with the mutex locked now and it should not be.

I wonder about your source code interpretation here.
The mutex was (and is still only) locked within case branches, isn't it?


> 
>>  }
>>  
>>  return -EINVAL;
> Mutex is still locked here and the return is wrong.

Should this statement get any more software development attention?

Regards,
Markus

[PATCH] cxgb4/cudbg_lib: Use common error handling code in cudbg_collect_tid()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 15 Mar 2018 14:56:10 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c 
b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
index 9da6f57901a9..d5b5b6221d1f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
@@ -1660,11 +1660,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[0] = FW_PARAM_PFVF_A(ETHOFLD_START);
para[1] = FW_PARAM_PFVF_A(ETHOFLD_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2, para, val);
-   if (rc <  0) {
-   cudbg_err->sys_err = rc;
-   cudbg_put_buff(pdbg_init, _buff);
-   return rc;
-   }
+   if (rc < 0)
+   goto put_buffer;
+
tid->uotid_base = val[0];
tid->nuotids = val[1] - val[0] + 1;
 
@@ -1679,11 +1677,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[1] = FW_PARAM_PFVF_A(HPFILTER_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2,
 para, val);
-   if (rc < 0) {
-   cudbg_err->sys_err = rc;
-   cudbg_put_buff(pdbg_init, _buff);
-   return rc;
-   }
+   if (rc < 0)
+   goto put_buffer;
+
tid->hpftid_base = val[0];
tid->nhpftids = val[1] - val[0] + 1;
}
@@ -1711,6 +1707,11 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
tid->ipv6_users = t4_read_reg(padap, LE_DB_ACT_CNT_IPV6_A);
 
return cudbg_write_and_release_buff(pdbg_init, _buff, dbg_buff);
+
+put_buffer:
+   cudbg_put_buff(pdbg_init, _buff);
+   cudbg_err->sys_err = rc;
+   return rc;
 }
 
 int cudbg_collect_pcie_config(struct cudbg_init *pdbg_init,
-- 
2.16.2

[PATCH] cxgb4/cudbg_lib: Use common error handling code in cudbg_collect_tid()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 15 Mar 2018 14:56:10 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c 
b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
index 9da6f57901a9..d5b5b6221d1f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
@@ -1660,11 +1660,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[0] = FW_PARAM_PFVF_A(ETHOFLD_START);
para[1] = FW_PARAM_PFVF_A(ETHOFLD_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2, para, val);
-   if (rc <  0) {
-   cudbg_err->sys_err = rc;
-   cudbg_put_buff(pdbg_init, _buff);
-   return rc;
-   }
+   if (rc < 0)
+   goto put_buffer;
+
tid->uotid_base = val[0];
tid->nuotids = val[1] - val[0] + 1;
 
@@ -1679,11 +1677,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[1] = FW_PARAM_PFVF_A(HPFILTER_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2,
 para, val);
-   if (rc < 0) {
-   cudbg_err->sys_err = rc;
-   cudbg_put_buff(pdbg_init, _buff);
-   return rc;
-   }
+   if (rc < 0)
+   goto put_buffer;
+
tid->hpftid_base = val[0];
tid->nhpftids = val[1] - val[0] + 1;
}
@@ -1711,6 +1707,11 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
tid->ipv6_users = t4_read_reg(padap, LE_DB_ACT_CNT_IPV6_A);
 
return cudbg_write_and_release_buff(pdbg_init, _buff, dbg_buff);
+
+put_buffer:
+   cudbg_put_buff(pdbg_init, _buff);
+   cudbg_err->sys_err = rc;
+   return rc;
 }
 
 int cudbg_collect_pcie_config(struct cudbg_init *pdbg_init,
-- 
2.16.2

Re: [media] ov5645: Move an error code assignment in ov5645_probe()

[SF Markus Elfring]

>> Move an assignment for a specific error code so that it is stored only once
>> in this function implementation.
>>
>> This issue was detected by using the Coccinelle software.
> 
> How?

Would you like to experiment a bit more with the following approach
for the semantic patch language?

show_same_statements3.cocci:

@duplicated_code@
identifier work;
statement s1, s2;
type T;
@@
 T work(...)
 {
 ... when any
*if ((...) < 0)
*{
...
*   s1
*   s2
*}
 ... when any
*if ((...) < 0)
*{
...
*   s1
*   s2
*}
 ... when any
 }


>> @@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
>>  
>>  power_down:
>>  ov5645_s_power(>sd, false);
>> +ret = -ENODEV;
> 
> I don't think this is where people would expect you to set the error code
> in general.

This can be. - The view depends on some factors.


> It should rather take place before goto, not after it.

I proposed another software design direction.


> That'd mean another variable,

To which detail do you refer here?


> and I'm not convinced the result would improve the driver.

Can you see the relevance of a small code reduction in this function?

Regards,
Markus

Re: [media] ov5645: Move an error code assignment in ov5645_probe()

[SF Markus Elfring]

>> Move an assignment for a specific error code so that it is stored only once
>> in this function implementation.
>>
>> This issue was detected by using the Coccinelle software.
> 
> How?

Would you like to experiment a bit more with the following approach
for the semantic patch language?

show_same_statements3.cocci:

@duplicated_code@
identifier work;
statement s1, s2;
type T;
@@
 T work(...)
 {
 ... when any
*if ((...) < 0)
*{
...
*   s1
*   s2
*}
 ... when any
*if ((...) < 0)
*{
...
*   s1
*   s2
*}
 ... when any
 }


>> @@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
>>  
>>  power_down:
>>  ov5645_s_power(>sd, false);
>> +ret = -ENODEV;
> 
> I don't think this is where people would expect you to set the error code
> in general.

This can be. - The view depends on some factors.


> It should rather take place before goto, not after it.

I proposed another software design direction.


> That'd mean another variable,

To which detail do you refer here?


> and I'm not convinced the result would improve the driver.

Can you see the relevance of a small code reduction in this function?

Regards,
Markus

[PATCH] [media] ov5645: Move an error code assignment in ov5645_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 14 Mar 2018 22:02:52 +0100

Move an assignment for a specific error code so that it is stored only once
in this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/i2c/ov5645.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index d28845f7356f..374576380fd4 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, _id_high);
if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
dev_err(dev, "could not read ID high\n");
-   ret = -ENODEV;
goto power_down;
}
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, _id_low);
if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
dev_err(dev, "could not read ID low\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
  >aec_pk_manual);
if (ret < 0) {
dev_err(dev, "could not read AEC/AGC mode\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
  >timing_tc_reg20);
if (ret < 0) {
dev_err(dev, "could not read vflip value\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
  >timing_tc_reg21);
if (ret < 0) {
dev_err(dev, "could not read hflip value\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
 
 power_down:
ov5645_s_power(>sd, false);
+   ret = -ENODEV;
 free_entity:
media_entity_cleanup(>sd.entity);
 free_ctrl:
-- 
2.16.2

[PATCH] [media] ov5645: Move an error code assignment in ov5645_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 14 Mar 2018 22:02:52 +0100

Move an assignment for a specific error code so that it is stored only once
in this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/i2c/ov5645.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index d28845f7356f..374576380fd4 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, _id_high);
if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
dev_err(dev, "could not read ID high\n");
-   ret = -ENODEV;
goto power_down;
}
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, _id_low);
if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
dev_err(dev, "could not read ID low\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
  >aec_pk_manual);
if (ret < 0) {
dev_err(dev, "could not read AEC/AGC mode\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
  >timing_tc_reg20);
if (ret < 0) {
dev_err(dev, "could not read vflip value\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
  >timing_tc_reg21);
if (ret < 0) {
dev_err(dev, "could not read hflip value\n");
-   ret = -ENODEV;
goto power_down;
}
 
@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
 
 power_down:
ov5645_s_power(>sd, false);
+   ret = -ENODEV;
 free_entity:
media_entity_cleanup(>sd.entity);
 free_ctrl:
-- 
2.16.2

[PATCH] iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 14 Mar 2018 16:06:49 +0100

* Add jump targets so that a call of the function "mutex_unlock" is stored
  only once in these function implementations.

* Replace 19 calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/gyro/bmg160_core.c | 103 ++---
 1 file changed, 45 insertions(+), 58 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 63ca31628a93..fa367fd7bc8c 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -499,21 +499,19 @@ static int bmg160_get_temp(struct bmg160_data *data, int 
*val)
 
mutex_lock(>mutex);
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret = regmap_read(data->regmap, BMG160_REG_TEMP, _val);
if (ret < 0) {
dev_err(dev, "Error reading reg_temp\n");
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
*val = sign_extend32(raw_val, 7);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(>mutex);
if (ret < 0)
return ret;
@@ -529,22 +527,20 @@ static int bmg160_get_axis(struct bmg160_data *data, int 
axis, int *val)
 
mutex_lock(>mutex);
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), _val,
   sizeof(raw_val));
if (ret < 0) {
dev_err(dev, "Error reading axis %d\n", axis);
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
*val = sign_extend32(le16_to_cpu(raw_val), 15);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(>mutex);
if (ret < 0)
return ret;
@@ -632,19 +628,16 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
 * mode to power on for other writes.
 */
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = bmg160_set_bw(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
-   ret = bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+
+   goto set_power_state;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
if (val2)
return -EINVAL;
@@ -653,18 +646,15 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
ret = bmg160_set_power_state(data, true);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
ret = bmg160_set_filter(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
-   ret = bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+
+   goto set_power_state;
case IIO_CHAN_INFO_SCALE:
if (val)
return -EINVAL;
@@ -672,24 +662,27 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
mutex_lock(>mutex);
/* Refer to comments above for the suspend mode ops */
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = bmg160_set_scale(data, val2);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;

[PATCH] iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 14 Mar 2018 16:06:49 +0100

* Add jump targets so that a call of the function "mutex_unlock" is stored
  only once in these function implementations.

* Replace 19 calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/gyro/bmg160_core.c | 103 ++---
 1 file changed, 45 insertions(+), 58 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 63ca31628a93..fa367fd7bc8c 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -499,21 +499,19 @@ static int bmg160_get_temp(struct bmg160_data *data, int 
*val)
 
mutex_lock(>mutex);
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret = regmap_read(data->regmap, BMG160_REG_TEMP, _val);
if (ret < 0) {
dev_err(dev, "Error reading reg_temp\n");
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
*val = sign_extend32(raw_val, 7);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(>mutex);
if (ret < 0)
return ret;
@@ -529,22 +527,20 @@ static int bmg160_get_axis(struct bmg160_data *data, int 
axis, int *val)
 
mutex_lock(>mutex);
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), _val,
   sizeof(raw_val));
if (ret < 0) {
dev_err(dev, "Error reading axis %d\n", axis);
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
*val = sign_extend32(le16_to_cpu(raw_val), 15);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(>mutex);
if (ret < 0)
return ret;
@@ -632,19 +628,16 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
 * mode to power on for other writes.
 */
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = bmg160_set_bw(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
-   ret = bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+
+   goto set_power_state;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
if (val2)
return -EINVAL;
@@ -653,18 +646,15 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
ret = bmg160_set_power_state(data, true);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
ret = bmg160_set_filter(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
-   ret = bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+
+   goto set_power_state;
case IIO_CHAN_INFO_SCALE:
if (val)
return -EINVAL;
@@ -672,24 +662,27 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
mutex_lock(>mutex);
/* Refer to comments above for the suspend mode ops */
ret = bmg160_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = bmg160_set_scale(data, val2);
if (ret < 0) {
bmg160_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
-   ret = bmg160_set_power_state(data, 

[PATCH] iio/adc/nau7802: Improve unlocking of a mutex in nau7802_read_raw()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 20:52:26 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
  only once in this function implementation.

* Replace two calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/adc/nau7802.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
index 8997e74a8847..68d06a492760 100644
--- a/drivers/iio/adc/nau7802.c
+++ b/drivers/iio/adc/nau7802.c
@@ -303,10 +303,8 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
 *   - Channel 2 is value 1 in the CHS register
 */
ret = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL2);
-   if (ret < 0) {
-   mutex_unlock(>lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
if (((ret & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
(!(ret & NAU7802_CTRL2_CHS_BIT) &&
@@ -316,18 +314,15 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
NAU7802_REG_CTRL2,
NAU7802_CTRL2_CHS(chan->channel) |
NAU7802_CTRL2_CRS(st->sample_rate));
-
-   if (ret < 0) {
-   mutex_unlock(>lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
}
 
if (st->client->irq)
ret = nau7802_read_irq(indio_dev, chan, val);
else
ret = nau7802_read_poll(indio_dev, chan, val);
-
+unlock:
mutex_unlock(>lock);
return ret;
 
-- 
2.16.2

[PATCH] iio/adc/nau7802: Improve unlocking of a mutex in nau7802_read_raw()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 20:52:26 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
  only once in this function implementation.

* Replace two calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/adc/nau7802.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
index 8997e74a8847..68d06a492760 100644
--- a/drivers/iio/adc/nau7802.c
+++ b/drivers/iio/adc/nau7802.c
@@ -303,10 +303,8 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
 *   - Channel 2 is value 1 in the CHS register
 */
ret = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL2);
-   if (ret < 0) {
-   mutex_unlock(>lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
if (((ret & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
(!(ret & NAU7802_CTRL2_CHS_BIT) &&
@@ -316,18 +314,15 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
NAU7802_REG_CTRL2,
NAU7802_CTRL2_CHS(chan->channel) |
NAU7802_CTRL2_CRS(st->sample_rate));
-
-   if (ret < 0) {
-   mutex_unlock(>lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
}
 
if (st->client->irq)
ret = nau7802_read_irq(indio_dev, chan, val);
else
ret = nau7802_read_poll(indio_dev, chan, val);
-
+unlock:
mutex_unlock(>lock);
return ret;
 
-- 
2.16.2

[PATCH] iio/adc/ad7291: Improve unlocking of a mutex in ad7291_read_raw()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 20:08:40 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
  only once in this function implementation.

* Replace three calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/adc/ad7291.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
index a862b5d8fb4b..b2c3d4c79909 100644
--- a/drivers/iio/adc/ad7291.c
+++ b/drivers/iio/adc/ad7291.c
@@ -335,27 +335,27 @@ static int ad7291_read_raw(struct iio_dev *indio_dev,
mutex_lock(>state_lock);
/* If in autocycle mode drop through */
if (chip->command & AD7291_AUTOCYCLE) {
-   mutex_unlock(>state_lock);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
/* Enable this channel alone */
regval = chip->command & (~AD7291_VOLTAGE_MASK);
regval |= BIT(15 - chan->channel);
ret = ad7291_i2c_write(chip, AD7291_COMMAND, regval);
-   if (ret < 0) {
-   mutex_unlock(>state_lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
/* Read voltage */
ret = i2c_smbus_read_word_swapped(chip->client,
  AD7291_VOLTAGE);
-   if (ret < 0) {
-   mutex_unlock(>state_lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
*val = ret & AD7291_VALUE_MASK;
+   ret = IIO_VAL_INT;
+unlock:
mutex_unlock(>state_lock);
-   return IIO_VAL_INT;
+   return ret;
case IIO_TEMP:
/* Assumes tsense bit of command register always set */
ret = i2c_smbus_read_word_swapped(chip->client,
-- 
2.16.2

[PATCH] iio/adc/ad7291: Improve unlocking of a mutex in ad7291_read_raw()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 20:08:40 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
  only once in this function implementation.

* Replace three calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/adc/ad7291.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
index a862b5d8fb4b..b2c3d4c79909 100644
--- a/drivers/iio/adc/ad7291.c
+++ b/drivers/iio/adc/ad7291.c
@@ -335,27 +335,27 @@ static int ad7291_read_raw(struct iio_dev *indio_dev,
mutex_lock(>state_lock);
/* If in autocycle mode drop through */
if (chip->command & AD7291_AUTOCYCLE) {
-   mutex_unlock(>state_lock);
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}
/* Enable this channel alone */
regval = chip->command & (~AD7291_VOLTAGE_MASK);
regval |= BIT(15 - chan->channel);
ret = ad7291_i2c_write(chip, AD7291_COMMAND, regval);
-   if (ret < 0) {
-   mutex_unlock(>state_lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
/* Read voltage */
ret = i2c_smbus_read_word_swapped(chip->client,
  AD7291_VOLTAGE);
-   if (ret < 0) {
-   mutex_unlock(>state_lock);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
*val = ret & AD7291_VALUE_MASK;
+   ret = IIO_VAL_INT;
+unlock:
mutex_unlock(>state_lock);
-   return IIO_VAL_INT;
+   return ret;
case IIO_TEMP:
/* Assumes tsense bit of command register always set */
ret = i2c_smbus_read_word_swapped(chip->client,
-- 
2.16.2

[PATCH] iio/accel/kxcjk-1013: Improve unlocking of a mutex in three functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 13:40:12 +0100

* Add jump targets so that a call of the function "mutex_unlock" is stored
  less often in these function implementations.

* Replace eight calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/accel/kxcjk-1013.c | 46 ++
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index af53a1084ee5..4adf38b6d939 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -765,19 +765,18 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
ret = -EBUSY;
else {
ret = kxcjk1013_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
*val = sign_extend32(ret >> 4, 11);
ret = kxcjk1013_set_power_state(data, false);
}
+unlock:
mutex_unlock(>mutex);
 
if (ret < 0)
@@ -905,8 +904,8 @@ static int kxcjk1013_write_event_config(struct iio_dev 
*indio_dev,
 
if (!state && data->motion_trigger_on) {
data->ev_enable_state = 0;
-   mutex_unlock(>mutex);
-   return 0;
+   ret = 0;
+   goto unlock;
}
 
/*
@@ -919,23 +918,20 @@ static int kxcjk1013_write_event_config(struct iio_dev 
*indio_dev,
 * is always on so sequence doesn't matter
 */
ret = kxcjk1013_set_power_state(data, state);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret =  kxcjk1013_setup_any_motion_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
data->ev_enable_state = 0;
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
data->ev_enable_state = state;
+unlock:
mutex_unlock(>mutex);
-
-   return 0;
+   return ret;
 }
 
 static int kxcjk1013_buffer_preenable(struct iio_dev *indio_dev)
@@ -1086,32 +1082,30 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct 
iio_trigger *trig,
 
if (!state && data->ev_enable_state && data->motion_trigger_on) {
data->motion_trigger_on = false;
-   mutex_unlock(>mutex);
-   return 0;
+   ret = 0;
+   goto unlock;
}
 
ret = kxcjk1013_set_power_state(data, state);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
if (data->motion_trig == trig)
ret = kxcjk1013_setup_any_motion_interrupt(data, state);
else
ret = kxcjk1013_setup_new_data_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
if (data->motion_trig == trig)
data->motion_trigger_on = state;
else
data->dready_trigger_on = state;
 
+unlock:
mutex_unlock(>mutex);
-
-   return 0;
+   return ret;
 }
 
 static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
-- 
2.16.2

[PATCH] iio/accel/kxcjk-1013: Improve unlocking of a mutex in three functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 13 Mar 2018 13:40:12 +0100

* Add jump targets so that a call of the function "mutex_unlock" is stored
  less often in these function implementations.

* Replace eight calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iio/accel/kxcjk-1013.c | 46 ++
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index af53a1084ee5..4adf38b6d939 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -765,19 +765,18 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
ret = -EBUSY;
else {
ret = kxcjk1013_set_power_state(data, true);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
*val = sign_extend32(ret >> 4, 11);
ret = kxcjk1013_set_power_state(data, false);
}
+unlock:
mutex_unlock(>mutex);
 
if (ret < 0)
@@ -905,8 +904,8 @@ static int kxcjk1013_write_event_config(struct iio_dev 
*indio_dev,
 
if (!state && data->motion_trigger_on) {
data->ev_enable_state = 0;
-   mutex_unlock(>mutex);
-   return 0;
+   ret = 0;
+   goto unlock;
}
 
/*
@@ -919,23 +918,20 @@ static int kxcjk1013_write_event_config(struct iio_dev 
*indio_dev,
 * is always on so sequence doesn't matter
 */
ret = kxcjk1013_set_power_state(data, state);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
 
ret =  kxcjk1013_setup_any_motion_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
data->ev_enable_state = 0;
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
 
data->ev_enable_state = state;
+unlock:
mutex_unlock(>mutex);
-
-   return 0;
+   return ret;
 }
 
 static int kxcjk1013_buffer_preenable(struct iio_dev *indio_dev)
@@ -1086,32 +1082,30 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct 
iio_trigger *trig,
 
if (!state && data->ev_enable_state && data->motion_trigger_on) {
data->motion_trigger_on = false;
-   mutex_unlock(>mutex);
-   return 0;
+   ret = 0;
+   goto unlock;
}
 
ret = kxcjk1013_set_power_state(data, state);
-   if (ret < 0) {
-   mutex_unlock(>mutex);
-   return ret;
-   }
+   if (ret < 0)
+   goto unlock;
+
if (data->motion_trig == trig)
ret = kxcjk1013_setup_any_motion_interrupt(data, state);
else
ret = kxcjk1013_setup_new_data_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
-   mutex_unlock(>mutex);
-   return ret;
+   goto unlock;
}
if (data->motion_trig == trig)
data->motion_trigger_on = state;
else
data->dready_trigger_on = state;
 
+unlock:
mutex_unlock(>mutex);
-
-   return 0;
+   return ret;
 }
 
 static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
-- 
2.16.2

Re: hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> So you are asking people to review 60 changed lines to save 2,

A bit of object code reduction might become useful also in this case.


> that alone should be the point where you stop yourself from
> *even* sending this patch.

I proposed just another collateral evolution.


> Next time before you send a patch please carefully think if the
> saving is worth the combination of reviewers time + the risk of
> regressions (and keep in mind that both the reviewers time and
> the risk of regressions cost increase for more complex changes).

Source code transformations were integrated in other software areas
according to such a change pattern.


> As for this specific discussion, there are certain "design-patterns"
> in the kernel, goto style error handling is one of them, the pattern
> there ALWAYS is:
…
> Notice the fall-thoughs those are ALWAYS there, never, ever is
> there a goto after a cleanup label.

It seems that I present an unusual update suggestion as a software
design variant.


> Your patches black goto magic completely messes this up

You can view the proposal in such a way.


> and clearly falls under the CS101 rule: never use goto.

There might a target conflict with information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”.

Regards,
Markus

Re: hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> So you are asking people to review 60 changed lines to save 2,

A bit of object code reduction might become useful also in this case.


> that alone should be the point where you stop yourself from
> *even* sending this patch.

I proposed just another collateral evolution.


> Next time before you send a patch please carefully think if the
> saving is worth the combination of reviewers time + the risk of
> regressions (and keep in mind that both the reviewers time and
> the risk of regressions cost increase for more complex changes).

Source code transformations were integrated in other software areas
according to such a change pattern.


> As for this specific discussion, there are certain "design-patterns"
> in the kernel, goto style error handling is one of them, the pattern
> there ALWAYS is:
…
> Notice the fall-thoughs those are ALWAYS there, never, ever is
> there a goto after a cleanup label.

It seems that I present an unusual update suggestion as a software
design variant.


> Your patches black goto magic completely messes this up

You can view the proposal in such a way.


> and clearly falls under the CS101 rule: never use goto.

There might a target conflict with information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”.

Regards,
Markus

Re: hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

>> Adjust jump targets so that a bit of exception handling can be better
>> reused at the end of this function.
…
> goto-s going to a label calling another goto is completely unreadable.

I got an other software development view.


> I really do not see any reason for the proposed changes,

I suggest to look once more.


> they may remove a small amount of code duplication,

This was my software design goal in this case.


> but at a hugh cost wrt readability.

I proposed a different trade-off here.

Regards,
Markus

Re: hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

>> Adjust jump targets so that a bit of exception handling can be better
>> reused at the end of this function.
…
> goto-s going to a label calling another goto is completely unreadable.

I got an other software development view.


> I really do not see any reason for the proposed changes,

I suggest to look once more.


> they may remove a small amount of code duplication,

This was my software design goal in this case.


> but at a hugh cost wrt readability.

I proposed a different trade-off here.

Regards,
Markus

Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

[SF Markus Elfring]

>> Add a jump target so that the setting of a specific error code is stored
>> only once at the end of this function.
>>
>> Signed-off-by: Markus Elfring 
>> ---
>> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btmrvl_sdio.c 
>> b/drivers/bluetooth/btmrvl_sdio.c
>> index 05c78fcc13ff..24ed62fe2aeb 100644
>> --- a/drivers/bluetooth/btmrvl_sdio.c
>> +++ b/drivers/bluetooth/btmrvl_sdio.c
>> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base0 = 0x%04X(%d)."
>>  " Terminating download",
>>  base0, base0);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  base1 = sdio_readb(card->func,
>>  card->reg->sq_read_base_addr_a1, );
>> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base1 = 0x%04X(%d)."
>>  " Terminating download",
>>  base1, base1);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>
>>  len = (((u16) base1) << 8) | base0;
>> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  if (count > MAX_WRITE_IOMEM_RETRY) {
>>  BT_ERR("FW download failure @%d, "
>>  "over max retry count", offset);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  BT_ERR("FW CRC error indicated by the helper: "
>>  "len = 0x%04X, txlen = %d", len, txlen);
>> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  kfree(tmpfwbuf);
>>  release_firmware(fw_firmware);
>>  return ret;
>> +
>> +e_io:
>> +ret = -EIO;
>> +goto done;
>> }
> 
> I am not applying this one. I see zero benefit in this change.

Would you care for a bit of object code reduction in this function 
implementation.


> It is not even saving a single line since it actually is more code.

Should I fiddle with any other source code layout?

Do you find an extra blank line inappropriate before the added jump target
in this use case?

Regards,
Markus

Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

[SF Markus Elfring]

>> Add a jump target so that the setting of a specific error code is stored
>> only once at the end of this function.
>>
>> Signed-off-by: Markus Elfring 
>> ---
>> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btmrvl_sdio.c 
>> b/drivers/bluetooth/btmrvl_sdio.c
>> index 05c78fcc13ff..24ed62fe2aeb 100644
>> --- a/drivers/bluetooth/btmrvl_sdio.c
>> +++ b/drivers/bluetooth/btmrvl_sdio.c
>> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base0 = 0x%04X(%d)."
>>  " Terminating download",
>>  base0, base0);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  base1 = sdio_readb(card->func,
>>  card->reg->sq_read_base_addr_a1, );
>> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base1 = 0x%04X(%d)."
>>  " Terminating download",
>>  base1, base1);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>
>>  len = (((u16) base1) << 8) | base0;
>> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  if (count > MAX_WRITE_IOMEM_RETRY) {
>>  BT_ERR("FW download failure @%d, "
>>  "over max retry count", offset);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  BT_ERR("FW CRC error indicated by the helper: "
>>  "len = 0x%04X, txlen = %d", len, txlen);
>> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  kfree(tmpfwbuf);
>>  release_firmware(fw_firmware);
>>  return ret;
>> +
>> +e_io:
>> +ret = -EIO;
>> +goto done;
>> }
> 
> I am not applying this one. I see zero benefit in this change.

Would you care for a bit of object code reduction in this function 
implementation.


> It is not even saving a single line since it actually is more code.

Should I fiddle with any other source code layout?

Do you find an extra blank line inappropriate before the added jump target
in this use case?

Regards,
Markus

Re: [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host()

[SF Markus Elfring]

>> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
>> btmrvl_private *priv)
>>  break;
>>  }
>>
>> -exit:
>> -if (ret) {
>> -hdev->stat.err_rx++;
>> -kfree_skb(skb);
>> -}
>> +return 0;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +e_io:
>> +ret = -EIO;
>> +goto increment_counter;
>>
>> +e_inval:
>> +ret = -EINVAL;
>> +increment_counter:
>> +hdev->stat.err_rx++;
>>  return ret;
> 
> Nope!
> 
> This is not easier to read for me. This goto exit jumping and I hate that.

Can the software design direction become feasible to omit the repeated check
for the variable “ret” (and further initialisations)?

Regards,
Markus

Re: [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host()

[SF Markus Elfring]

>> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct 
>> btmrvl_private *priv)
>>  break;
>>  }
>>
>> -exit:
>> -if (ret) {
>> -hdev->stat.err_rx++;
>> -kfree_skb(skb);
>> -}
>> +return 0;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +e_io:
>> +ret = -EIO;
>> +goto increment_counter;
>>
>> +e_inval:
>> +ret = -EINVAL;
>> +increment_counter:
>> +hdev->stat.err_rx++;
>>  return ret;
> 
> Nope!
> 
> This is not easier to read for me. This goto exit jumping and I hate that.

Can the software design direction become feasible to omit the repeated check
for the variable “ret” (and further initialisations)?

Regards,
Markus

Re: [2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()

[SF Markus Elfring]

>> Use three values directly for a condition check without assigning them
>> to intermediate variables.
> 
> Hi,
> 
> what is the benefit of this?

I proposed a small source code reduction.

Other software design directions might become more interesting for this use 
case.

Regards,
Markus

Re: [2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()

[SF Markus Elfring]

>> Use three values directly for a condition check without assigning them
>> to intermediate variables.
> 
> Hi,
> 
> what is the benefit of this?

I proposed a small source code reduction.

Other software design directions might become more interesting for this use 
case.

Regards,
Markus

Re: [v2 14/17 4/4] mfd: tps65910: Checking patch structures

[SF Markus Elfring]

> How have you managed to insert 4 patches into the x/17 thread?

I dared to group the desired patch series into dedicated mail threads.

Regards,
Markus

Re: [v2 14/17 4/4] mfd: tps65910: Checking patch structures

[SF Markus Elfring]

> How have you managed to insert 4 patches into the x/17 thread?

I dared to group the desired patch series into dedicated mail threads.

Regards,
Markus

Re: [2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection

[SF Markus Elfring]

> +set_error_code:
> + err = -errno;
> + os_close_file(rxfd);

I have taken another look at this change idea.
Now I notice that I should have preserved a sanity check there.

if (rxfd >= 0)
os_close_file(rxfd);


Regards,
Markus

Re: [2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection

[SF Markus Elfring]

> +set_error_code:
> + err = -errno;
> + os_close_file(rxfd);

I have taken another look at this change idea.
Now I notice that I should have preserved a sanity check there.

if (rxfd >= 0)
os_close_file(rxfd);


Regards,
Markus

[PATCH] hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 22:15:59 +0100

Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/hwmon/sch5627.c | 60 -
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 91544f2312e6..e7a2a974ab74 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_HWMON_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
   val, SCH5627_HWMON_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_COMPANY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
   val, SCH5627_COMPANY_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_PRIMARY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
   val, SCH5627_PRIMARY_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
build_code = sch56xx_read_virtual_reg(data->addr,
  SCH5627_REG_BUILD_CODE);
if (build_code < 0) {
err = build_code;
-   goto error;
+   goto remove_device;
}
 
build_id = sch56xx_read_virtual_reg16(data->addr,
  SCH5627_REG_BUILD_ID);
if (build_id < 0) {
err = build_id;
-   goto error;
+   goto remove_device;
}
 
hwmon_rev = sch56xx_read_virtual_reg(data->addr,
 SCH5627_REG_HWMON_REV);
if (hwmon_rev < 0) {
err = hwmon_rev;
-   goto error;
+   goto remove_device;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
data->control = val;
if (!(data->control & 0x01)) {
pr_err("hardware monitoring not enabled\n");
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
/* Trigger a Vbat voltage measurement, so that we get a valid reading
   the first time we read Vbat */
@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
 */
err = sch5627_read_limits(data);
if (err)
-   goto error;
+   goto remove_device;
 
pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
/* Register sysfs interface files */
err = sysfs_create_group(>dev.kobj, _group);
if (err)
-   goto error;
+   goto remove_device;
 
data->hwmon_dev = hwmon_device_register(>dev);
if (IS_ERR(data->hwmon_dev)) {
err = PTR_ERR(data->hwmon_dev);
data->hwmon_dev = NULL;
-   goto error;
+   goto remove_device;
}
 
/* Note failing to register the watchdog is not a fatal error */
@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
 
return 0;
 
-error:
+set_error_code:
+   err = val;
+   goto remove_device;
+
+e_nodev:
+   err = -ENODEV;
+remove_device:
sch5627_remove(pdev);
return err;
 }
-- 
2.16.2

[PATCH] hwmon/sch5627: Use common error handling code in sch5627_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 22:15:59 +0100

Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/hwmon/sch5627.c | 60 -
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 91544f2312e6..e7a2a974ab74 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_HWMON_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
   val, SCH5627_HWMON_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_COMPANY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
   val, SCH5627_COMPANY_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
if (val != SCH5627_PRIMARY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
   val, SCH5627_PRIMARY_ID);
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
 
build_code = sch56xx_read_virtual_reg(data->addr,
  SCH5627_REG_BUILD_CODE);
if (build_code < 0) {
err = build_code;
-   goto error;
+   goto remove_device;
}
 
build_id = sch56xx_read_virtual_reg16(data->addr,
  SCH5627_REG_BUILD_ID);
if (build_id < 0) {
err = build_id;
-   goto error;
+   goto remove_device;
}
 
hwmon_rev = sch56xx_read_virtual_reg(data->addr,
 SCH5627_REG_HWMON_REV);
if (hwmon_rev < 0) {
err = hwmon_rev;
-   goto error;
+   goto remove_device;
}
 
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
-   if (val < 0) {
-   err = val;
-   goto error;
-   }
+   if (val < 0)
+   goto set_error_code;
+
data->control = val;
if (!(data->control & 0x01)) {
pr_err("hardware monitoring not enabled\n");
-   err = -ENODEV;
-   goto error;
+   goto e_nodev;
}
/* Trigger a Vbat voltage measurement, so that we get a valid reading
   the first time we read Vbat */
@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
 */
err = sch5627_read_limits(data);
if (err)
-   goto error;
+   goto remove_device;
 
pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
/* Register sysfs interface files */
err = sysfs_create_group(>dev.kobj, _group);
if (err)
-   goto error;
+   goto remove_device;
 
data->hwmon_dev = hwmon_device_register(>dev);
if (IS_ERR(data->hwmon_dev)) {
err = PTR_ERR(data->hwmon_dev);
data->hwmon_dev = NULL;
-   goto error;
+   goto remove_device;
}
 
/* Note failing to register the watchdog is not a fatal error */
@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
 
return 0;
 
-error:
+set_error_code:
+   err = val;
+   goto remove_device;
+
+e_nodev:
+   err = -ENODEV;
+remove_device:
sch5627_remove(pdev);
return err;
 }
-- 
2.16.2

[PATCH] altera_edac: Use common error handling code in altr_sdram_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 16:23:53 +0100

Add a jump target so that a specific error code is assigned to the
local variable "res" at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/edac/altera_edac.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 11d6419788c2..d5c15b27d520 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -419,8 +419,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
   "Unable to request irq %d\n", irq2);
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
@@ -435,8 +434,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
   "Unable to request irq %d\n", irq);
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
/* Infrastructure ready - enable the IRQ */
@@ -444,8 +442,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
   priv->ecc_irq_en_mask, priv->ecc_irq_en_mask)) {
edac_mc_printk(mci, KERN_ERR,
   "Error enabling SDRAM ECC IRQ\n");
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
altr_sdr_mc_create_debugfs_nodes(mci);
@@ -454,6 +451,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
 
return 0;
 
+e_nodev:
+   res = -ENODEV;
 err2:
edac_mc_del_mc(>dev);
 err:
-- 
2.16.2

[PATCH] altera_edac: Use common error handling code in altr_sdram_probe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 16:23:53 +0100

Add a jump target so that a specific error code is assigned to the
local variable "res" at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/edac/altera_edac.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 11d6419788c2..d5c15b27d520 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -419,8 +419,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
   "Unable to request irq %d\n", irq2);
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
@@ -435,8 +434,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
   "Unable to request irq %d\n", irq);
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
/* Infrastructure ready - enable the IRQ */
@@ -444,8 +442,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
   priv->ecc_irq_en_mask, priv->ecc_irq_en_mask)) {
edac_mc_printk(mci, KERN_ERR,
   "Error enabling SDRAM ECC IRQ\n");
-   res = -ENODEV;
-   goto err2;
+   goto e_nodev;
}
 
altr_sdr_mc_create_debugfs_nodes(mci);
@@ -454,6 +451,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
 
return 0;
 
+e_nodev:
+   res = -ENODEV;
 err2:
edac_mc_del_mc(>dev);
 err:
-- 
2.16.2

Re: dmaengine: edma: Use common error handling code in three functions

[SF Markus Elfring]

> Date: Sun, 22 Oct 2017 16:46:34 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of these functions.

How are the chances to integrate such a change into another Linux repository?
https://lkml.org/lkml/2017/10/22/78
https://patchwork.kernel.org/patch/10021725/
https://lkml.kernel.org/r/

Regards,
Markus

Re: dmaengine: edma: Use common error handling code in three functions

[SF Markus Elfring]

> Date: Sun, 22 Oct 2017 16:46:34 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of these functions.

How are the chances to integrate such a change into another Linux repository?
https://lkml.org/lkml/2017/10/22/78
https://patchwork.kernel.org/patch/10021725/
https://lkml.kernel.org/r/

Regards,
Markus

[PATCH 2/2] crypto: talitos: Delete an error message for a failed memory allocation in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:18:23 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a2271322db34..4c7318981d28 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1399,7 +1399,6 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 
edesc = kmalloc(alloc_len, GFP_DMA | flags);
if (!edesc) {
-   dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}
-- 
2.16.2

[PATCH 2/2] crypto: talitos: Delete an error message for a failed memory allocation in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:18:23 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a2271322db34..4c7318981d28 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1399,7 +1399,6 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 
edesc = kmalloc(alloc_len, GFP_DMA | flags);
if (!edesc) {
-   dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}
-- 
2.16.2

[PATCH 1/2] crypto: talitos: Use common error handling code in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:08:55 +0100

Add jump targets so that an error message and the setting of a specific
error code is stored only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6882fa2f8bad..a2271322db34 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1352,29 +1352,24 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
dst_len = 0;
} else { /* dst && dst != src*/
src_len = assoclen + cryptlen + (encrypt ? 0 : authsize);
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   goto set_error_code;
}
dst_nents = (dst_nents == 1) ? 0 : dst_nents;
}
@@ -1424,6 +1419,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 DMA_BIDIRECTIONAL);
}
return edesc;
+
+report_failure:
+   dev_err(dev, "Invalid number of src SG.\n");
+set_error_code:
+   err = ERR_PTR(-EINVAL);
 error_sg:
if (iv_dma)
dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
-- 
2.16.2

[PATCH 1/2] crypto: talitos: Use common error handling code in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:08:55 +0100

Add jump targets so that an error message and the setting of a specific
error code is stored only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6882fa2f8bad..a2271322db34 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1352,29 +1352,24 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
dst_len = 0;
} else { /* dst && dst != src*/
src_len = assoclen + cryptlen + (encrypt ? 0 : authsize);
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   goto set_error_code;
}
dst_nents = (dst_nents == 1) ? 0 : dst_nents;
}
@@ -1424,6 +1419,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 DMA_BIDIRECTIONAL);
}
return edesc;
+
+report_failure:
+   dev_err(dev, "Invalid number of src SG.\n");
+set_error_code:
+   err = ERR_PTR(-EINVAL);
 error_sg:
if (iv_dma)
dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
-- 
2.16.2

[PATCH 0/2] crypto/talitos: Adjustments for talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:24:34 +0100

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

Markus Elfring (2):
  Use common error handling code
  Delete an error message for a failed memory allocation

 drivers/crypto/talitos.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
2.16.2

[PATCH 0/2] crypto/talitos: Adjustments for talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:24:34 +0100

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

Markus Elfring (2):
  Use common error handling code
  Delete an error message for a failed memory allocation

 drivers/crypto/talitos.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
2.16.2

[PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:30:28 +0100

Add a jump target so that the setting of a specific error code is stored
only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 05c78fcc13ff..24ed62fe2aeb 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base0 = 0x%04X(%d)."
" Terminating download",
base0, base0);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
base1 = sdio_readb(card->func,
card->reg->sq_read_base_addr_a1, );
@@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base1 = 0x%04X(%d)."
" Terminating download",
base1, base1);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
 
len = (((u16) base1) << 8) | base0;
@@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
if (count > MAX_WRITE_IOMEM_RETRY) {
BT_ERR("FW download failure @%d, "
"over max retry count", offset);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
BT_ERR("FW CRC error indicated by the helper: "
"len = 0x%04X, txlen = %d", len, txlen);
@@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
kfree(tmpfwbuf);
release_firmware(fw_firmware);
return ret;
+
+e_io:
+   ret = -EIO;
+   goto done;
 }
 
 static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv)
-- 
2.16.2

[PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:30:28 +0100

Add a jump target so that the setting of a specific error code is stored
only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 05c78fcc13ff..24ed62fe2aeb 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base0 = 0x%04X(%d)."
" Terminating download",
base0, base0);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
base1 = sdio_readb(card->func,
card->reg->sq_read_base_addr_a1, );
@@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base1 = 0x%04X(%d)."
" Terminating download",
base1, base1);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
 
len = (((u16) base1) << 8) | base0;
@@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
if (count > MAX_WRITE_IOMEM_RETRY) {
BT_ERR("FW download failure @%d, "
"over max retry count", offset);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
BT_ERR("FW CRC error indicated by the helper: "
"len = 0x%04X, txlen = %d", len, txlen);
@@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
kfree(tmpfwbuf);
release_firmware(fw_firmware);
return ret;
+
+e_io:
+   ret = -EIO;
+   goto done;
 }
 
 static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv)
-- 
2.16.2

[PATCH 4/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:15:59 +0100

The variable "payload" will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 9854addc8e96..05c78fcc13ff 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -691,5 +691,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
u32 type;
-   u8 *payload = NULL;
+   u8 *payload;
struct hci_dev *hdev = priv->btmrvl_dev.hcidev;
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
 
-- 
2.16.2

[PATCH 4/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:15:59 +0100

The variable "payload" will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 9854addc8e96..05c78fcc13ff 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -691,5 +691,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
u32 type;
-   u8 *payload = NULL;
+   u8 *payload;
struct hci_dev *hdev = priv->btmrvl_dev.hcidev;
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
 
-- 
2.16.2

[PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:13:00 +0100

One check could be repeated by the btmrvl_sdio_card_to_host() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Reuse a bit of exception handling better.

* Delete an initialisation for the local variable "skb"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 84c222abf0f7..9854addc8e96 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 {
u16 buf_len = 0;
int ret, num_blocks, blksz;
-   struct sk_buff *skb = NULL;
+   struct sk_buff *skb;
u32 type;
@@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 
if (!card || !card->func) {
BT_ERR("card or function is NULL!");
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Read the length of data to be transferred */
ret = btmrvl_sdio_read_rx_len(card, _len);
if (ret < 0) {
BT_ERR("read rx_len failed");
-   ret = -EIO;
-   goto exit;
+   goto e_io;
}
 
blksz = SDIO_BLOCK_SIZE;
@@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len <= SDIO_HEADER_LEN
|| (num_blocks * blksz) > ALLOC_BUF_SIZE) {
BT_ERR("invalid packet length: %d", buf_len);
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Allocate buffer */
@@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (!skb) {
BT_ERR("No free skb");
ret = -ENOMEM;
-   goto exit;
+   goto increment_counter;
}
 
if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
@@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
  num_blocks * blksz);
if (ret < 0) {
BT_ERR("readsb failed: %d", ret);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
/* This is SDIO specific header length: byte[2][1][0], type: byte[3]
@@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len > blksz * num_blocks) {
BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
   buf_len, blksz * num_blocks);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
type = payload[3];
@@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
break;
}
 
-exit:
-   if (ret) {
-   hdev->stat.err_rx++;
-   kfree_skb(skb);
-   }
+   return 0;
+
+free_skb:
+   kfree_skb(skb);
+e_io:
+   ret = -EIO;
+   goto increment_counter;
 
+e_inval:
+   ret = -EINVAL;
+increment_counter:
+   hdev->stat.err_rx++;
return ret;
 }
 
-- 
2.16.2

[PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:13:00 +0100

One check could be repeated by the btmrvl_sdio_card_to_host() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Reuse a bit of exception handling better.

* Delete an initialisation for the local variable "skb"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 84c222abf0f7..9854addc8e96 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 {
u16 buf_len = 0;
int ret, num_blocks, blksz;
-   struct sk_buff *skb = NULL;
+   struct sk_buff *skb;
u32 type;
@@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
 
if (!card || !card->func) {
BT_ERR("card or function is NULL!");
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Read the length of data to be transferred */
ret = btmrvl_sdio_read_rx_len(card, _len);
if (ret < 0) {
BT_ERR("read rx_len failed");
-   ret = -EIO;
-   goto exit;
+   goto e_io;
}
 
blksz = SDIO_BLOCK_SIZE;
@@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len <= SDIO_HEADER_LEN
|| (num_blocks * blksz) > ALLOC_BUF_SIZE) {
BT_ERR("invalid packet length: %d", buf_len);
-   ret = -EINVAL;
-   goto exit;
+   goto e_inval;
}
 
/* Allocate buffer */
@@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (!skb) {
BT_ERR("No free skb");
ret = -ENOMEM;
-   goto exit;
+   goto increment_counter;
}
 
if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
@@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
  num_blocks * blksz);
if (ret < 0) {
BT_ERR("readsb failed: %d", ret);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
/* This is SDIO specific header length: byte[2][1][0], type: byte[3]
@@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
if (buf_len > blksz * num_blocks) {
BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
   buf_len, blksz * num_blocks);
-   ret = -EIO;
-   goto exit;
+   goto free_skb;
}
 
type = payload[3];
@@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private 
*priv)
break;
}
 
-exit:
-   if (ret) {
-   hdev->stat.err_rx++;
-   kfree_skb(skb);
-   }
+   return 0;
+
+free_skb:
+   kfree_skb(skb);
+e_io:
+   ret = -EIO;
+   goto increment_counter;
 
+e_inval:
+   ret = -EINVAL;
+increment_counter:
+   hdev->stat.err_rx++;
return ret;
 }
 
-- 
2.16.2

[PATCH 2/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 10:20:04 +0100

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

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index df2a04bd8428..84c222abf0f7 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -920,7 +920,7 @@ static int btmrvl_sdio_register_dev(struct btmrvl_sdio_card 
*card)
 {
struct sdio_func *func;
u8 reg;
-   int ret = 0;
+   int ret;
 
if (!card || !card->func) {
BT_ERR("Error: card or function is NULL!");
-- 
2.16.2

[PATCH 2/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 10:20:04 +0100

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

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index df2a04bd8428..84c222abf0f7 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -920,7 +920,7 @@ static int btmrvl_sdio_register_dev(struct btmrvl_sdio_card 
*card)
 {
struct sdio_func *func;
u8 reg;
-   int ret = 0;
+   int ret;
 
if (!card || !card->func) {
BT_ERR("Error: card or function is NULL!");
-- 
2.16.2

[PATCH 1/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_register_dev()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 10:15:17 +0100

Adjust a jump target so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 50 -
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 0f020254fd39..df2a04bd8428 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -949,31 +949,24 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
ret = sdio_set_block_size(card->func, SDIO_BLOCK_SIZE);
if (ret) {
BT_ERR("cannot set SDIO block size");
-   ret = -EIO;
-   goto release_irq;
+   goto release_with_eio;
}
 
reg = sdio_readb(func, card->reg->io_port_0, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport = reg;
 
reg = sdio_readb(func, card->reg->io_port_1, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport |= (reg << 8);
 
reg = sdio_readb(func, card->reg->io_port_2, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport |= (reg << 16);
 
@@ -981,26 +974,20 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
 
if (card->reg->int_read_to_clear) {
reg = sdio_readb(func, card->reg->host_int_rsr, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
+
sdio_writeb(func, reg | 0x3f, card->reg->host_int_rsr, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
reg = sdio_readb(func, card->reg->card_misc_cfg, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
+
sdio_writeb(func, reg | 0x10, card->reg->card_misc_cfg, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
}
 
sdio_set_drvdata(func, card);
@@ -1009,7 +996,8 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
 
return 0;
 
-release_irq:
+release_with_eio:
+   ret = -EIO;
sdio_release_irq(func);
 
 disable_func:
-- 
2.16.2

[PATCH 1/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_register_dev()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 10:15:17 +0100

Adjust a jump target so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 50 -
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 0f020254fd39..df2a04bd8428 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -949,31 +949,24 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
ret = sdio_set_block_size(card->func, SDIO_BLOCK_SIZE);
if (ret) {
BT_ERR("cannot set SDIO block size");
-   ret = -EIO;
-   goto release_irq;
+   goto release_with_eio;
}
 
reg = sdio_readb(func, card->reg->io_port_0, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport = reg;
 
reg = sdio_readb(func, card->reg->io_port_1, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport |= (reg << 8);
 
reg = sdio_readb(func, card->reg->io_port_2, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
card->ioport |= (reg << 16);
 
@@ -981,26 +974,20 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
 
if (card->reg->int_read_to_clear) {
reg = sdio_readb(func, card->reg->host_int_rsr, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
+
sdio_writeb(func, reg | 0x3f, card->reg->host_int_rsr, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
 
reg = sdio_readb(func, card->reg->card_misc_cfg, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
+
sdio_writeb(func, reg | 0x10, card->reg->card_misc_cfg, );
-   if (ret < 0) {
-   ret = -EIO;
-   goto release_irq;
-   }
+   if (ret < 0)
+   goto release_with_eio;
}
 
sdio_set_drvdata(func, card);
@@ -1009,7 +996,8 @@ static int btmrvl_sdio_register_dev(struct 
btmrvl_sdio_card *card)
 
return 0;
 
-release_irq:
+release_with_eio:
+   ret = -EIO;
sdio_release_irq(func);
 
 disable_func:
-- 
2.16.2

[PATCH 0/5] Bluetooth/btmrvl_sdio: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 12:10:24 +0100

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

Markus Elfring (5):
  Use common error handling code in btmrvl_sdio_register_dev()
  Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev()
  One check less in btmrvl_sdio_card_to_host() after error detection
  Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host()
  Use common error handling code in btmrvl_sdio_download_fw_w_helper()

 drivers/bluetooth/btmrvl_sdio.c | 102 ++--
 1 file changed, 46 insertions(+), 56 deletions(-)

-- 
2.16.2

[PATCH 0/5] Bluetooth/btmrvl_sdio: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 12:10:24 +0100

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

Markus Elfring (5):
  Use common error handling code in btmrvl_sdio_register_dev()
  Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev()
  One check less in btmrvl_sdio_card_to_host() after error detection
  Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host()
  Use common error handling code in btmrvl_sdio_download_fw_w_helper()

 drivers/bluetooth/btmrvl_sdio.c | 102 ++--
 1 file changed, 46 insertions(+), 56 deletions(-)

-- 
2.16.2

[PATCH 9/9] um/drivers/vector_user: Delete an unnecessary variable initialisation in user_init_tap_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:50:29 +0100

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

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index bd625034a0f0..3bd510eded58 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -119,9 +119,8 @@ struct arglist *uml_parse_vector_ifspec(char *arg)
 static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
 {
struct ifreq ifr;
-   int fd = -1;
struct sockaddr_ll sock;
-   int err = -ENOMEM, offload;
+   int err = -ENOMEM, fd, offload;
char *iface;
struct vector_fds *result;
 
-- 
2.16.2

[PATCH 9/9] um/drivers/vector_user: Delete an unnecessary variable initialisation in user_init_tap_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:50:29 +0100

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

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index bd625034a0f0..3bd510eded58 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -119,9 +119,8 @@ struct arglist *uml_parse_vector_ifspec(char *arg)
 static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
 {
struct ifreq ifr;
-   int fd = -1;
struct sockaddr_ll sock;
-   int err = -ENOMEM, offload;
+   int err = -ENOMEM, fd, offload;
char *iface;
struct vector_fds *result;
 
-- 
2.16.2

[PATCH 8/9] um/drivers/vector_user: Less checks in user_init_tap_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:43:31 +0100

Three checks could be repeated by the user_init_tap_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets.

* Delete three sanity checks and an initialisation (for the local
  variable "result") which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 5e78723c34d4..bd625034a0f0 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -123,18 +123,18 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM, offload;
char *iface;
-   struct vector_fds *result = NULL;
+   struct vector_fds *result;
 
iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
if (iface == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to parse interface spec\n");
-   goto tap_cleanup;
+   goto report_failure;
}
 
result = uml_kmalloc(sizeof(struct vector_fds), UM_GFP_KERNEL);
if (result == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to allocate file 
descriptors\n");
-   goto tap_cleanup;
+   goto report_failure;
}
result->rx_fd = -1;
result->tx_fd = -1;
@@ -146,7 +146,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
fd = open(PATH_NET_TUN, O_RDWR);
if (fd < 0) {
printk(UM_KERN_ERR "uml_tap: failed to open tun device\n");
-   goto tap_cleanup;
+   goto free_result;
}
result->tx_fd = fd;
memset(, 0, sizeof(ifr));
@@ -156,7 +156,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
err = ioctl(fd, TUNSETIFF, (void *) );
if (err != 0) {
printk(UM_KERN_ERR "uml_tap: failed to select tap interface\n");
-   goto tap_cleanup;
+   goto close_tx_fd;
}
 
offload = TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6;
@@ -168,7 +168,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
if (fd == -1) {
printk(UM_KERN_ERR
"uml_tap: failed to create socket: %i\n", -errno);
-   goto tap_cleanup;
+   goto close_tx_fd;
}
result->rx_fd = fd;
memset(, 0, sizeof(ifr));
@@ -176,7 +176,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
if (ioctl(fd, SIOCGIFINDEX, (void *) ) < 0) {
printk(UM_KERN_ERR
"uml_tap: failed to set interface: %i\n", -errno);
-   goto tap_cleanup;
+   goto close_rx_fd;
}
 
sock.sll_family = AF_PACKET;
@@ -188,18 +188,17 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
printk(UM_KERN_ERR
"user_init_tap: failed to bind raw pair, err %d\n",
-errno);
-   goto tap_cleanup;
+   goto close_rx_fd;
}
return result;
-tap_cleanup:
-   if (result != NULL) {
-   if (result->rx_fd >= 0)
-   os_close_file(result->rx_fd);
-   if (result->tx_fd >= 0)
-   os_close_file(result->tx_fd);
-   kfree(result);
-   }
 
+close_rx_fd:
+   os_close_file(result->rx_fd);
+close_tx_fd:
+   os_close_file(result->tx_fd);
+free_result:
+   kfree(result);
+report_failure:
printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
-- 
2.16.2

[PATCH 8/9] um/drivers/vector_user: Less checks in user_init_tap_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:43:31 +0100

Three checks could be repeated by the user_init_tap_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets.

* Delete three sanity checks and an initialisation (for the local
  variable "result") which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 5e78723c34d4..bd625034a0f0 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -123,18 +123,18 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM, offload;
char *iface;
-   struct vector_fds *result = NULL;
+   struct vector_fds *result;
 
iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
if (iface == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to parse interface spec\n");
-   goto tap_cleanup;
+   goto report_failure;
}
 
result = uml_kmalloc(sizeof(struct vector_fds), UM_GFP_KERNEL);
if (result == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to allocate file 
descriptors\n");
-   goto tap_cleanup;
+   goto report_failure;
}
result->rx_fd = -1;
result->tx_fd = -1;
@@ -146,7 +146,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
fd = open(PATH_NET_TUN, O_RDWR);
if (fd < 0) {
printk(UM_KERN_ERR "uml_tap: failed to open tun device\n");
-   goto tap_cleanup;
+   goto free_result;
}
result->tx_fd = fd;
memset(, 0, sizeof(ifr));
@@ -156,7 +156,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
err = ioctl(fd, TUNSETIFF, (void *) );
if (err != 0) {
printk(UM_KERN_ERR "uml_tap: failed to select tap interface\n");
-   goto tap_cleanup;
+   goto close_tx_fd;
}
 
offload = TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6;
@@ -168,7 +168,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
if (fd == -1) {
printk(UM_KERN_ERR
"uml_tap: failed to create socket: %i\n", -errno);
-   goto tap_cleanup;
+   goto close_tx_fd;
}
result->rx_fd = fd;
memset(, 0, sizeof(ifr));
@@ -176,7 +176,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
if (ioctl(fd, SIOCGIFINDEX, (void *) ) < 0) {
printk(UM_KERN_ERR
"uml_tap: failed to set interface: %i\n", -errno);
-   goto tap_cleanup;
+   goto close_rx_fd;
}
 
sock.sll_family = AF_PACKET;
@@ -188,18 +188,17 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
printk(UM_KERN_ERR
"user_init_tap: failed to bind raw pair, err %d\n",
-errno);
-   goto tap_cleanup;
+   goto close_rx_fd;
}
return result;
-tap_cleanup:
-   if (result != NULL) {
-   if (result->rx_fd >= 0)
-   os_close_file(result->rx_fd);
-   if (result->tx_fd >= 0)
-   os_close_file(result->tx_fd);
-   kfree(result);
-   }
 
+close_rx_fd:
+   os_close_file(result->rx_fd);
+close_tx_fd:
+   os_close_file(result->tx_fd);
+free_result:
+   kfree(result);
+report_failure:
printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
-- 
2.16.2

[PATCH 7/9] um/drivers/vector_user: Adjust an error message in user_init_tap_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:10:05 +0100

Adjust an error message at the end of this function so that its name
will be automatically determined as a parameter.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4c265262a369..5e78723c34d4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -192,7 +192,6 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
}
return result;
 tap_cleanup:
-   printk(UM_KERN_ERR "user_init_tap: init failed, error %d", err);
if (result != NULL) {
if (result->rx_fd >= 0)
os_close_file(result->rx_fd);
@@ -200,6 +199,8 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
os_close_file(result->tx_fd);
kfree(result);
}
+
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
 
-- 
2.16.2

[PATCH 7/9] um/drivers/vector_user: Adjust an error message in user_init_tap_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 15:10:05 +0100

Adjust an error message at the end of this function so that its name
will be automatically determined as a parameter.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4c265262a369..5e78723c34d4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -192,7 +192,6 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
}
return result;
 tap_cleanup:
-   printk(UM_KERN_ERR "user_init_tap: init failed, error %d", err);
if (result != NULL) {
if (result->rx_fd >= 0)
os_close_file(result->rx_fd);
@@ -200,6 +199,8 @@ static struct vector_fds *user_init_tap_fds(struct arglist 
*ifspec)
os_close_file(result->tx_fd);
kfree(result);
}
+
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
 
-- 
2.16.2

[PATCH 6/9] um/drivers/vector_user: Less checks in user_init_socket_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:56:38 +0100

Two checks could be repeated by the user_init_socket_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets.

* Delete two sanity checks and a call of the function "kfree"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 2dee1e183387..4c265262a369 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -375,13 +375,13 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
"socket_open : could not open socket, error = %d",
-errno
);
-   goto cleanup;
+   goto free_info;
}
if (bind(fd,
(struct sockaddr *) gairesult->ai_addr,
gairesult->ai_addrlen)) {
printk(UM_KERN_ERR L2TPV3_BIND_FAIL, errno);
-   goto cleanup;
+   goto close_file;
}
 
freeaddrinfo(gairesult);
@@ -403,7 +403,8 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
result->remote_addr = uml_kmalloc(
gairesult->ai_addrlen, UM_GFP_KERNEL);
if (result->remote_addr == NULL)
-   goto cleanup;
+   goto free_result;
+
result->remote_addr_size = gairesult->ai_addrlen;
memcpy(
result->remote_addr,
@@ -413,16 +414,13 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
}
freeaddrinfo(gairesult);
return result;
-cleanup:
-   freeaddrinfo(gairesult);
-
-   if (fd >= 0)
-   os_close_file(fd);
-   if (result != NULL) {
-   kfree(result->remote_addr);
-   kfree(result);
-   }
 
+free_result:
+   kfree(result);
+close_file:
+   os_close_file(fd);
+free_info:
+   freeaddrinfo(gairesult);
printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
 }
-- 
2.16.2

[PATCH 6/9] um/drivers/vector_user: Less checks in user_init_socket_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:56:38 +0100

Two checks could be repeated by the user_init_socket_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets.

* Delete two sanity checks and a call of the function "kfree"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 2dee1e183387..4c265262a369 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -375,13 +375,13 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
"socket_open : could not open socket, error = %d",
-errno
);
-   goto cleanup;
+   goto free_info;
}
if (bind(fd,
(struct sockaddr *) gairesult->ai_addr,
gairesult->ai_addrlen)) {
printk(UM_KERN_ERR L2TPV3_BIND_FAIL, errno);
-   goto cleanup;
+   goto close_file;
}
 
freeaddrinfo(gairesult);
@@ -403,7 +403,8 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
result->remote_addr = uml_kmalloc(
gairesult->ai_addrlen, UM_GFP_KERNEL);
if (result->remote_addr == NULL)
-   goto cleanup;
+   goto free_result;
+
result->remote_addr_size = gairesult->ai_addrlen;
memcpy(
result->remote_addr,
@@ -413,16 +414,13 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
}
freeaddrinfo(gairesult);
return result;
-cleanup:
-   freeaddrinfo(gairesult);
-
-   if (fd >= 0)
-   os_close_file(fd);
-   if (result != NULL) {
-   kfree(result->remote_addr);
-   kfree(result);
-   }
 
+free_result:
+   kfree(result);
+close_file:
+   os_close_file(fd);
+free_info:
+   freeaddrinfo(gairesult);
printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
 }
-- 
2.16.2

[PATCH 5/9] um/drivers/vector_user: Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:20:46 +0100

The implementation returns from this function if a null pointer
was detected in the local variable "gairesult". Thus the check
before two calls of the function "freeaddrinfo" is not needed.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index e831bd85cad4..2dee1e183387 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -384,9 +384,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
goto cleanup;
}
 
-   if (gairesult != NULL)
-   freeaddrinfo(gairesult);
-
+   freeaddrinfo(gairesult);
gairesult = NULL;
 
gairet = getaddrinfo(dst, dstport, , );
@@ -416,8 +414,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
freeaddrinfo(gairesult);
return result;
 cleanup:
-   if (gairesult != NULL)
-   freeaddrinfo(gairesult);
+   freeaddrinfo(gairesult);
 
if (fd >= 0)
os_close_file(fd);
-- 
2.16.2

[PATCH 5/9] um/drivers/vector_user: Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:20:46 +0100

The implementation returns from this function if a null pointer
was detected in the local variable "gairesult". Thus the check
before two calls of the function "freeaddrinfo" is not needed.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index e831bd85cad4..2dee1e183387 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -384,9 +384,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
goto cleanup;
}
 
-   if (gairesult != NULL)
-   freeaddrinfo(gairesult);
-
+   freeaddrinfo(gairesult);
gairesult = NULL;
 
gairet = getaddrinfo(dst, dstport, , );
@@ -416,8 +414,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
freeaddrinfo(gairesult);
return result;
 cleanup:
-   if (gairesult != NULL)
-   freeaddrinfo(gairesult);
+   freeaddrinfo(gairesult);
 
if (fd >= 0)
os_close_file(fd);
-- 
2.16.2

[PATCH 4/9] um/drivers/vector_user: Delete an unnecessary check before kfree() in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:00:09 +0100

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 9cc4651990e3..e831bd85cad4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -422,8 +422,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
-   if (result->remote_addr != NULL)
-   kfree(result->remote_addr);
+   kfree(result->remote_addr);
kfree(result);
}
 
-- 
2.16.2

[PATCH 4/9] um/drivers/vector_user: Delete an unnecessary check before kfree() in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 14:00:09 +0100

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 9cc4651990e3..e831bd85cad4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -422,8 +422,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
-   if (result->remote_addr != NULL)
-   kfree(result->remote_addr);
+   kfree(result->remote_addr);
kfree(result);
}
 
-- 
2.16.2

[PATCH 3/9] um/drivers/vector_user: Adjust an error message in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 13:53:08 +0100

* Adjust an error message at the end of this function.

* Delete the local variable "err" which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 037cd85ee424..9cc4651990e3 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -305,7 +305,6 @@ bool uml_tap_enable_vnet_headers(int fd)
 
 static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
 {
-   int err = -ENOMEM;
int fd = -1, gairet;
struct addrinfo srchints;
struct addrinfo dsthints;
@@ -419,7 +418,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
 cleanup:
if (gairesult != NULL)
freeaddrinfo(gairesult);
-   printk(UM_KERN_ERR "user_init_socket: init failed, error %d", err);
+
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
@@ -427,6 +426,8 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
kfree(result->remote_addr);
kfree(result);
}
+
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
 }
 
-- 
2.16.2

[PATCH 3/9] um/drivers/vector_user: Adjust an error message in user_init_socket_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 13:53:08 +0100

* Adjust an error message at the end of this function.

* Delete the local variable "err" which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 037cd85ee424..9cc4651990e3 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -305,7 +305,6 @@ bool uml_tap_enable_vnet_headers(int fd)
 
 static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
 {
-   int err = -ENOMEM;
int fd = -1, gairet;
struct addrinfo srchints;
struct addrinfo dsthints;
@@ -419,7 +418,7 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
 cleanup:
if (gairesult != NULL)
freeaddrinfo(gairesult);
-   printk(UM_KERN_ERR "user_init_socket: init failed, error %d", err);
+
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
@@ -427,6 +426,8 @@ static struct vector_fds *user_init_socket_fds(struct 
arglist *ifspec, int id)
kfree(result->remote_addr);
kfree(result);
}
+
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
 }
 
-- 
2.16.2

[PATCH 2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 11:40:14 +0100

Up to two checks could be repeated by the user_init_raw_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Delete an initialisation for the local variable "rxfd"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 61 ---
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index d6a6207d4061..037cd85ee424 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -207,54 +207,46 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
 static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
 {
struct ifreq ifr;
-   int rxfd = -1, txfd = -1;
+   int rxfd, txfd = -1;
struct sockaddr_ll sock;
-   int err = -ENOMEM;
-   char *iface;
struct vector_fds *result;
int optval = 1;
+   int err;
+   char *iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
 
-
-   iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
-   if (iface == NULL)
-   goto cleanup;
+   if (!iface) {
+   err = -ENOMEM;
+   goto report_failure;
+   }
 
rxfd = socket(AF_PACKET, SOCK_RAW, ETH_P_ALL);
-   if (rxfd == -1) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (rxfd == -1)
+   goto set_error_code;
+
txfd = socket(AF_PACKET, SOCK_RAW, 0); /* Turn off RX on this fd */
-   if (txfd == -1) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (txfd == -1)
+   goto set_error_code;
+
memset(, 0, sizeof(ifr));
strncpy((char *)_name, iface, sizeof(ifr.ifr_name) - 1);
-   if (ioctl(rxfd, SIOCGIFINDEX, (void *) ) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (ioctl(rxfd, SIOCGIFINDEX, (void *) ) < 0)
+   goto set_error_code;
 
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_ALL);
sock.sll_ifindex = ifr.ifr_ifindex;
 
-   if (bind(rxfd,
-   (struct sockaddr *) , sizeof(struct sockaddr_ll)) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (bind(rxfd, (struct sockaddr *), sizeof(struct sockaddr_ll))
+   < 0)
+   goto set_error_code;
 
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_IP);
sock.sll_ifindex = ifr.ifr_ifindex;
 
-   if (bind(txfd,
-   (struct sockaddr *) , sizeof(struct sockaddr_ll)) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (bind(txfd, (struct sockaddr *), sizeof(struct sockaddr_ll))
+   < 0)
+   goto set_error_code;
 
if (setsockopt(txfd,
SOL_PACKET, PACKET_QDISC_BYPASS,
@@ -270,12 +262,15 @@ static struct vector_fds *user_init_raw_fds(struct 
arglist *ifspec)
result->remote_addr_size = 0;
}
return result;
-cleanup:
-   printk(UM_KERN_ERR "user_init_raw: init failed, error %d", err);
-   if (rxfd >= 0)
-   os_close_file(rxfd);
+
+set_error_code:
+   err = -errno;
+   os_close_file(rxfd);
+
if (txfd >= 0)
os_close_file(txfd);
+report_failure:
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
 
-- 
2.16.2

[PATCH 2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 11:40:14 +0100

Up to two checks could be repeated by the user_init_raw_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Adjust jump targets so that an extra check can be omitted at the end.

* Delete an initialisation for the local variable "rxfd"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 61 ---
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index d6a6207d4061..037cd85ee424 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -207,54 +207,46 @@ static struct vector_fds *user_init_tap_fds(struct 
arglist *ifspec)
 static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
 {
struct ifreq ifr;
-   int rxfd = -1, txfd = -1;
+   int rxfd, txfd = -1;
struct sockaddr_ll sock;
-   int err = -ENOMEM;
-   char *iface;
struct vector_fds *result;
int optval = 1;
+   int err;
+   char *iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
 
-
-   iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
-   if (iface == NULL)
-   goto cleanup;
+   if (!iface) {
+   err = -ENOMEM;
+   goto report_failure;
+   }
 
rxfd = socket(AF_PACKET, SOCK_RAW, ETH_P_ALL);
-   if (rxfd == -1) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (rxfd == -1)
+   goto set_error_code;
+
txfd = socket(AF_PACKET, SOCK_RAW, 0); /* Turn off RX on this fd */
-   if (txfd == -1) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (txfd == -1)
+   goto set_error_code;
+
memset(, 0, sizeof(ifr));
strncpy((char *)_name, iface, sizeof(ifr.ifr_name) - 1);
-   if (ioctl(rxfd, SIOCGIFINDEX, (void *) ) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (ioctl(rxfd, SIOCGIFINDEX, (void *) ) < 0)
+   goto set_error_code;
 
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_ALL);
sock.sll_ifindex = ifr.ifr_ifindex;
 
-   if (bind(rxfd,
-   (struct sockaddr *) , sizeof(struct sockaddr_ll)) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (bind(rxfd, (struct sockaddr *), sizeof(struct sockaddr_ll))
+   < 0)
+   goto set_error_code;
 
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_IP);
sock.sll_ifindex = ifr.ifr_ifindex;
 
-   if (bind(txfd,
-   (struct sockaddr *) , sizeof(struct sockaddr_ll)) < 0) {
-   err = -errno;
-   goto cleanup;
-   }
+   if (bind(txfd, (struct sockaddr *), sizeof(struct sockaddr_ll))
+   < 0)
+   goto set_error_code;
 
if (setsockopt(txfd,
SOL_PACKET, PACKET_QDISC_BYPASS,
@@ -270,12 +262,15 @@ static struct vector_fds *user_init_raw_fds(struct 
arglist *ifspec)
result->remote_addr_size = 0;
}
return result;
-cleanup:
-   printk(UM_KERN_ERR "user_init_raw: init failed, error %d", err);
-   if (rxfd >= 0)
-   os_close_file(rxfd);
+
+set_error_code:
+   err = -errno;
+   os_close_file(rxfd);
+
if (txfd >= 0)
os_close_file(txfd);
+report_failure:
+   printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
 }
 
-- 
2.16.2

[PATCH 1/9] um/drivers/vector_user: Delete unnecessary code in user_init_raw_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 11:36:18 +0100

* One condition check could never be reached with a non-null pointer
  at the end of this function. Thus remove the corresponding statement.

* Delete an initialisation for the local variable "result"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4291f1a5d342..d6a6207d4061 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist 
*ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM;
char *iface;
-   struct vector_fds *result = NULL;
+   struct vector_fds *result;
int optval = 1;
 
 
@@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist 
*ifspec)
os_close_file(rxfd);
if (txfd >= 0)
os_close_file(txfd);
-   if (result != NULL)
-   kfree(result);
return NULL;
 }
 
-- 
2.16.2

[PATCH 1/9] um/drivers/vector_user: Delete unnecessary code in user_init_raw_fds()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 11:36:18 +0100

* One condition check could never be reached with a non-null pointer
  at the end of this function. Thus remove the corresponding statement.

* Delete an initialisation for the local variable "result"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/um/drivers/vector_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4291f1a5d342..d6a6207d4061 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist 
*ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM;
char *iface;
-   struct vector_fds *result = NULL;
+   struct vector_fds *result;
int optval = 1;
 
 
@@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist 
*ifspec)
os_close_file(rxfd);
if (txfd >= 0)
os_close_file(txfd);
-   if (result != NULL)
-   kfree(result);
return NULL;
 }
 
-- 
2.16.2

[PATCH 0/9] UML vector network driver: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 16:06:16 +0100

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

Markus Elfring (9):
  Delete unnecessary code in user_init_raw_fds()
  Less checks in user_init_raw_fds() after error detection
  Adjust an error message in user_init_socket_fds()
  Delete an unnecessary check before kfree() in user_init_socket_fds()
  Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
  Less checks in user_init_socket_fds() after error detection
  Adjust an error message in user_init_tap_fds()
  Less checks in user_init_tap_fds() after error detection
  Delete an unnecessary variable initialisation in user_init_tap_fds()

 arch/um/drivers/vector_user.c | 133 +++---
 1 file changed, 60 insertions(+), 73 deletions(-)

-- 
2.16.2

[PATCH 0/9] UML vector network driver: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 16:06:16 +0100

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

Markus Elfring (9):
  Delete unnecessary code in user_init_raw_fds()
  Less checks in user_init_raw_fds() after error detection
  Adjust an error message in user_init_socket_fds()
  Delete an unnecessary check before kfree() in user_init_socket_fds()
  Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
  Less checks in user_init_socket_fds() after error detection
  Adjust an error message in user_init_tap_fds()
  Less checks in user_init_tap_fds() after error detection
  Delete an unnecessary variable initialisation in user_init_tap_fds()

 arch/um/drivers/vector_user.c | 133 +++---
 1 file changed, 60 insertions(+), 73 deletions(-)

-- 
2.16.2

[PATCH] powerpc: Use common error handling code in setup_new_fdt()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c 
b/arch/powerpc/kernel/machine_kexec_file_64.c
index e4395f937d63..90c6004c2eec 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -302,18 +302,14 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
ret = fdt_setprop_u64(fdt, chosen_node,
  "linux,initrd-start",
  initrd_load_addr);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
 
/* initrd-end is the first address after the initrd image. */
ret = fdt_setprop_u64(fdt, chosen_node, "linux,initrd-end",
  initrd_load_addr + initrd_len);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
 
ret = fdt_add_mem_rsv(fdt, initrd_load_addr, initrd_len);
if (ret) {
@@ -325,10 +321,8 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
 
if (cmdline != NULL) {
ret = fdt_setprop_string(fdt, chosen_node, "bootargs", cmdline);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
} else {
ret = fdt_delprop(fdt, chosen_node, "bootargs");
if (ret && ret != -FDT_ERR_NOTFOUND) {
@@ -344,10 +338,12 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
}
 
ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0);
-   if (ret) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret)
+   goto report_setup_failure;
 
return 0;
+
+report_setup_failure:
+   pr_err("Error setting up the new device tree.\n");
+   return -EINVAL;
 }
-- 
2.16.2

[PATCH] powerpc: Use common error handling code in setup_new_fdt()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c 
b/arch/powerpc/kernel/machine_kexec_file_64.c
index e4395f937d63..90c6004c2eec 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -302,18 +302,14 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
ret = fdt_setprop_u64(fdt, chosen_node,
  "linux,initrd-start",
  initrd_load_addr);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
 
/* initrd-end is the first address after the initrd image. */
ret = fdt_setprop_u64(fdt, chosen_node, "linux,initrd-end",
  initrd_load_addr + initrd_len);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
 
ret = fdt_add_mem_rsv(fdt, initrd_load_addr, initrd_len);
if (ret) {
@@ -325,10 +321,8 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
 
if (cmdline != NULL) {
ret = fdt_setprop_string(fdt, chosen_node, "bootargs", cmdline);
-   if (ret < 0) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret < 0)
+   goto report_setup_failure;
} else {
ret = fdt_delprop(fdt, chosen_node, "bootargs");
if (ret && ret != -FDT_ERR_NOTFOUND) {
@@ -344,10 +338,12 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
}
 
ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0);
-   if (ret) {
-   pr_err("Error setting up the new device tree.\n");
-   return -EINVAL;
-   }
+   if (ret)
+   goto report_setup_failure;
 
return 0;
+
+report_setup_failure:
+   pr_err("Error setting up the new device tree.\n");
+   return -EINVAL;
 }
-- 
2.16.2