[PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 21:45:09 +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 <elfring@users.sourceforge.net>
---
 drivers/power/supply/bq2415x_charger.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/bq2415x_charger.c b/drivers/power/supply/bq2415x_charger.c
index c4770a94cc8e..8385e02e1634 100644
--- a/drivers/power/supply/bq2415x_charger.c
+++ b/drivers/power/supply/bq2415x_charger.c
@@ -914,16 +914,12 @@ static void bq2415x_timer_work(struct work_struct *work)
 	}
 
 	boost = bq2415x_exec_command(bq, BQ2415X_BOOST_MODE_STATUS);
-	if (boost < 0) {
-		bq2415x_timer_error(bq, "Unknown error");
-		return;
-	}
+	if (boost < 0)
+		goto report_timer_error;
 
 	error = bq2415x_exec_command(bq, BQ2415X_FAULT_STATUS);
-	if (error < 0) {
-		bq2415x_timer_error(bq, "Unknown error");
-		return;
-	}
+	if (error < 0)
+		goto report_timer_error;
 
 	if (boost) {
 		switch (error) {
@@ -992,6 +988,10 @@ static void bq2415x_timer_work(struct work_struct *work)
 	}
 
 	schedule_delayed_work(&bq->work, BQ2415X_TIMER_TIMEOUT * HZ);
+	return;
+
+report_timer_error:
+	bq2415x_timer_error(bq, "Unknown error");
 }
 
 /**** power supply interface code ****/
-- 
2.14.3

Re: [PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[Pavel Machek]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 30 Oct 2017 21:45:09 +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 <elfring@users.sourceforge.net>

Better fix would be to display separate messages; user is probably
interested in what failed...
								Pavel

> ---
>  drivers/power/supply/bq2415x_charger.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/bq2415x_charger.c b/drivers/power/supply/bq2415x_charger.c
> index c4770a94cc8e..8385e02e1634 100644
> --- a/drivers/power/supply/bq2415x_charger.c
> +++ b/drivers/power/supply/bq2415x_charger.c
> @@ -914,16 +914,12 @@ static void bq2415x_timer_work(struct work_struct *work)
>  	}
>  
>  	boost = bq2415x_exec_command(bq, BQ2415X_BOOST_MODE_STATUS);
> -	if (boost < 0) {
> -		bq2415x_timer_error(bq, "Unknown error");
> -		return;
> -	}
> +	if (boost < 0)
> +		goto report_timer_error;
>  
>  	error = bq2415x_exec_command(bq, BQ2415X_FAULT_STATUS);
> -	if (error < 0) {
> -		bq2415x_timer_error(bq, "Unknown error");
> -		return;
> -	}
> +	if (error < 0)
> +		goto report_timer_error;
>  
>  	if (boost) {
>  		switch (error) {
> @@ -992,6 +988,10 @@ static void bq2415x_timer_work(struct work_struct *work)
>  	}
>  
>  	schedule_delayed_work(&bq->work, BQ2415X_TIMER_TIMEOUT * HZ);
> +	return;
> +
> +report_timer_error:
> +	bq2415x_timer_error(bq, "Unknown error");
>  }
>  
>  /**** power supply interface code ****/

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[SF Markus Elfring]

> Better fix would be to display separate messages; user is probably
> interested in what failed...

Which information (or wording) would you find more appropriate
at these places?

Regards,
Markus

Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[Pali Rohár]

On Thursday 09 November 2017 14:04:19 SF Markus Elfring wrote:
> > Better fix would be to display separate messages; user is probably
> > interested in what failed...
> 
> Which information (or wording) would you find more appropriate
> at these places?

Hi! Basically dropping your patch and instead of the "Unknown error"
return to user reason why BQ2415X_BOOST_MODE_STATUS or
BQ2415X_FAULT_STATUS commands failed. Or at least instead of the
"Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS".

Basically I do not see any value in your patch. Current coding style
pattern in that function is:

do_something;
if failed:
  print error;
  return;

And your patch just changed some, but not *all* parts of code to:

do_something;
if failed:
  goto end_of_function

If you are changing coding style, I would really suggest to change it on
all places to let it consistent. Because your change introduces just
inconsistency.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()

[SF Markus Elfring]

>> Which information (or wording) would you find more appropriate
>> at these places?
> 
> Hi! Basically dropping your patch and instead of the "Unknown error"
> return to user reason why BQ2415X_BOOST_MODE_STATUS or
> BQ2415X_FAULT_STATUS commands failed. Or at least instead of the
> "Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS".

Can the error description become better?

Regards,
Markus