[PATCH 0/2] iio/accel/stk8ba50: Fine-tuning for two function implementations

[PATCH 0/2] iio/accel/stk8ba50: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 22:10:12 +0200

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

Markus Elfring (2):
  Use common error handling code in stk8ba50_probe()
  Improve unlocking of a mutex in stk8ba50_read_raw()

 drivers/iio/accel/stk8ba50.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

-- 
2.14.3

[PATCH 1/2] iio/accel/stk8ba50: Use common error handling code in stk8ba50_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 21:36:03 +0200

* Add a jump target so that a specific error message is stored only once
  at the end of this function implementation.

* Replace two calls of the function "dev_err" by goto statements.

* Adjust condition checks.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/accel/stk8ba50.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c
index 576b6b140f08..afe90567ad37 100644
--- a/drivers/iio/accel/stk8ba50.c
+++ b/drivers/iio/accel/stk8ba50.c
@@ -426,16 +426,13 @@ static int stk8ba50_probe(struct i2c_client *client,
 	/* Set up interrupts */
 	ret = i2c_smbus_write_byte_data(client,
 			STK8BA50_REG_INTEN2, STK8BA50_DREADY_INT_MASK);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to set up interrupts\n");
-		goto err_power_off;
-	}
+	if (ret)
+		goto report_failure;
+
 	ret = i2c_smbus_write_byte_data(client,
 			STK8BA50_REG_INTMAP2, STK8BA50_DREADY_INT_MAP);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to set up interrupts\n");
-		goto err_power_off;
-	}
+	if (ret)
+		goto report_failure;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -495,6 +492,10 @@ static int stk8ba50_probe(struct i2c_client *client,
 err_power_off:
 	stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
 	return ret;
+
+report_failure:
+	dev_err(&client->dev, "failed to set up interrupts\n");
+	goto err_power_off;
 }
 
 static int stk8ba50_remove(struct i2c_client *client)
-- 
2.14.3

[PATCH 2/2] iio/accel/stk8ba50: Improve unlocking of a mutex in stk8ba50_read_raw()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 22:00:42 +0200

Adjust jump targets so that a call of the function "mutex_unlock" is stored
only once in a case branch of this function implementation.
Replace two calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/accel/stk8ba50.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c
index afe90567ad37..ffc26b5d1fad 100644
--- a/drivers/iio/accel/stk8ba50.c
+++ b/drivers/iio/accel/stk8ba50.c
@@ -221,20 +221,24 @@ static int stk8ba50_read_raw(struct iio_dev *indio_dev,
 			return -EBUSY;
 		mutex_lock(&data->lock);
 		ret = stk8ba50_set_power(data, STK8BA50_MODE_NORMAL);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
-			return -EINVAL;
-		}
+		if (ret < 0)
+			goto e_inval;
+
 		ret = stk8ba50_read_accel(data, chan->address);
 		if (ret < 0) {
 			stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
-			mutex_unlock(&data->lock);
-			return -EINVAL;
+e_inval:
+			ret = -EINVAL;
+			goto unlock;
 		}
 		*val = sign_extend32(ret >> STK8BA50_DATA_SHIFT, 9);
-		stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
+		ret = stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
+unlock:
 		mutex_unlock(&data->lock);
-		return IIO_VAL_INT;
+		if (ret >= 0)
+			ret = IIO_VAL_INT;
+
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
 		*val2 = stk8ba50_scale_table[data->range].scale_val;
-- 
2.14.3

Re: [PATCH 1/2] iio/accel/stk8ba50: Use common error handling code in stk8ba50_probe()

[Jonathan Cameron]

On Wed, 25 Oct 2017 22:16:31 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 25 Oct 2017 21:36:03 +0200
> 
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
> 
> * Replace two calls of the function "dev_err" by goto statements.
> 
> * Adjust condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Again readability is hurt by the more complex code flow.  As such
this is never going to be an acceptable change in the kernel.

Sorry Markus, but all patches have to pass a test on whether
any advantages to outweight complexity  This is definitely not
true here.

Again, a general kernel development rule is to float only one
patch of a given type until you have had feedback on it.  These
backwards goto cases are sufficiently different from your earlier
patch that got reviews that, at most, you should have sent one to the
list and then given time for it to be properly reviewed (up to a week
on IIO list typically).  That would have saved your time and mine.

As I said in one of the other patches, I always reply to all
patches I am rejecting so that anyone coming across them later
on their own from an archive or similar can immediately see the
reasons why they are a bad idea without having to know the mailing
list context.

Jonathan

> ---
>  drivers/iio/accel/stk8ba50.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c
> index 576b6b140f08..afe90567ad37 100644
> --- a/drivers/iio/accel/stk8ba50.c
> +++ b/drivers/iio/accel/stk8ba50.c
> @@ -426,16 +426,13 @@ static int stk8ba50_probe(struct i2c_client *client,
>  	/* Set up interrupts */
>  	ret = i2c_smbus_write_byte_data(client,
>  			STK8BA50_REG_INTEN2, STK8BA50_DREADY_INT_MASK);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "failed to set up interrupts\n");
> -		goto err_power_off;
> -	}
> +	if (ret)
> +		goto report_failure;
> +
>  	ret = i2c_smbus_write_byte_data(client,
>  			STK8BA50_REG_INTMAP2, STK8BA50_DREADY_INT_MAP);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "failed to set up interrupts\n");
> -		goto err_power_off;
> -	}
> +	if (ret)
> +		goto report_failure;
>  
>  	if (client->irq > 0) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -495,6 +492,10 @@ static int stk8ba50_probe(struct i2c_client *client,
>  err_power_off:
>  	stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
>  	return ret;
> +
> +report_failure:
> +	dev_err(&client->dev, "failed to set up interrupts\n");
> +	goto err_power_off;

Simple code flow is replaced by more complex flow for little gain.
Not a good idea.

>  }
>  
>  static int stk8ba50_remove(struct i2c_client *client)

Re: iio/accel/stk8ba50: Use common error handling code in stk8ba50_probe()

[SF Markus Elfring]

> Again, a general kernel development rule is to float only one
> patch of a given type until you have had feedback on it.

Some update suggestions according also to this general source code
transformation pattern were already integrated in other software areas.


> These backwards goto cases are sufficiently different from your earlier
> patch that got reviews that,

They are required if you would ever like to support software refactorings
in directions which I propose occasionally.


> at most, you should have sent one to the list and then given time
> for it to be properly reviewed (up to a week on IIO list typically).

Did I present my ideas a bit too quick in this case?

Regards,
Markus

Re: iio/…: Use common error handling code

[SF Markus Elfring]

> Again, a general kernel development rule is to float only one
> patch of a given type until you have had feedback on it.

A few weeks later …


> As I said in one of the other patches, I always reply to all
> patches I am rejecting so that anyone coming across them later
> on their own from an archive or similar can immediately see the
> reasons why they are a bad idea without having to know the mailing
> list context.

Can you get into the mood to clarify any remaining change possibilities
a bit more?

Regards,
Markus

Re: iio/…: Use common error handling code

[Jonathan Cameron]

On Sun, 10 Dec 2017 13:33:01 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Again, a general kernel development rule is to float only one
> > patch of a given type until you have had feedback on it.  
> 
> A few weeks later …
> 
> 
> > As I said in one of the other patches, I always reply to all
> > patches I am rejecting so that anyone coming across them later
> > on their own from an archive or similar can immediately see the
> > reasons why they are a bad idea without having to know the mailing
> > list context.  
> 
> Can you get into the mood to clarify any remaining change possibilities
> a bit more?

Hi Markus, I've accepted the ones that I think made an improvement
outweighing the inherent small costs of making any change.

So in short, any changes around common error handling need to
improve the maintainability of the code.  So if we are unifying error
paths they need to be simple and obviously the same.

We also need to avoid code constructs that are unusual in error handling
such as backwards gotos.

Note however that most of the changes made so far are only minor improvements.
I am not saying I don't appreciate them, but rather than that they are of
of low importance.

Thanks,

Jonathan

Re: iio/…: Use common error handling code

[SF Markus Elfring]

> Hi Markus, I've accepted the ones that I think made an improvement
> outweighing the inherent small costs of making any change.

Does such a kind of feedback mean that you reconsidered any places
where you expressed a rejection initially?


> We also need to avoid code constructs that are unusual in error handling
> such as backwards gotos.

Why would you like to exclude this approach if anything useful could be achieved
in the shown software design direction?


> Note however that most of the changes made so far are only minor improvements.

I agree that corresponding effects are small just because the discussed
source code adjustments affected specific function implementations.


> I am not saying I don't appreciate them,

Thanks.


> but rather than that they are of of low importance.

A lot of details are competing also for our software development attention.

Regards,
Markus

Re: iio/…: Use common error handling code

[Jonathan Cameron]

On Sun, 10 Dec 2017 17:43:34 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Hi Markus, I've accepted the ones that I think made an improvement
> > outweighing the inherent small costs of making any change.  
> 
> Does such a kind of feedback mean that you reconsidered any places
> where you expressed a rejection initially?
No.  Once I have expressed strong reservations about a patch it
would require some change in the facts to make me reevaluate.
> 
> 
> > We also need to avoid code constructs that are unusual in error handling
> > such as backwards gotos.  
> 
> Why would you like to exclude this approach if anything useful could be achieved
> in the shown software design direction?
Yes - exclude this. It trades of ease of review against briefness of code.
Ease of review and hence verification of correctness is more important in these
cases.

> 
> 
> > Note however that most of the changes made so far are only minor improvements.  
> 
> I agree that corresponding effects are small just because the discussed
> source code adjustments affected specific function implementations.
> 
> 
> > I am not saying I don't appreciate them,  
> 
> Thanks.
> 
> 
> > but rather than that they are of of low importance.  
> 
> A lot of details are competing also for our software development attention.
>
Exactly.

Jonathan
 
> Regards,
> Markus