[PATCH] iio/accel/stk8312: Improve unlocking of a mutex in two functions

[PATCH] iio/accel/stk8312: Improve unlocking of a mutex in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 20:46:18 +0200

* Adjust jump targets so that a call of the function "mutex_unlock"
  is mostly stored at the end of these function implementations.

* Replace four calls 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/stk8312.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index cacc0da2f874..5275ab886e39 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -344,24 +344,24 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
 			return -EBUSY;
 		mutex_lock(&data->lock);
 		ret = stk8312_set_mode(data, data->mode | STK8312_MODE_ACTIVE);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
-			return ret;
-		}
+		if (ret)
+			goto unlock;
+
 		ret = stk8312_read_accel(data, chan->address);
 		if (ret < 0) {
 			stk8312_set_mode(data,
 					 data->mode & (~STK8312_MODE_ACTIVE));
-			mutex_unlock(&data->lock);
-			return ret;
+			goto unlock;
 		}
 		*val = sign_extend32(ret, 7);
 		ret = stk8312_set_mode(data,
 				       data->mode & (~STK8312_MODE_ACTIVE));
+unlock:
 		mutex_unlock(&data->lock);
-		if (ret < 0)
-			return ret;
-		return IIO_VAL_INT;
+		if (!ret)
+			ret = IIO_VAL_INT;
+
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		*val = stk8312_scale_table[data->range - 1][0];
 		*val2 = stk8312_scale_table[data->range - 1][1];
@@ -444,17 +444,15 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 						    data->buffer);
 		if (ret < STK8312_ALL_CHANNEL_SIZE) {
 			dev_err(&data->client->dev, "register read failed\n");
-			mutex_unlock(&data->lock);
-			goto err;
+			goto unlock_after_failure;
 		}
 	} else {
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
 				 indio_dev->masklength) {
 			ret = stk8312_read_accel(data, bit);
-			if (ret < 0) {
-				mutex_unlock(&data->lock);
-				goto err;
-			}
+			if (ret < 0)
+				goto unlock_after_failure;
+
 			data->buffer[i++] = ret;
 		}
 	}
@@ -462,10 +460,13 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
 					   pf->timestamp);
-err:
+notify_trigger:
 	iio_trigger_notify_done(indio_dev->trig);
-
 	return IRQ_HANDLED;
+
+unlock_after_failure:
+	mutex_unlock(&data->lock);
+	goto notify_trigger;
 }
 
 static irqreturn_t stk8312_data_rdy_trig_poll(int irq, void *private)
-- 
2.14.3

Re: [PATCH] iio/accel/stk8312: Improve unlocking of a mutex in two functions

[Jonathan Cameron]

On Wed, 25 Oct 2017 20:55:23 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 25 Oct 2017 20:46:18 +0200
> 
> * Adjust jump targets so that a call of the function "mutex_unlock"
>   is mostly stored at the end of these function implementations.
> 
> * Replace four calls by goto statements.
> 
> * Adjust condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Hi Markus,

Thanks for this.  One small change would make the first case
easier to read in my mind.   We are already fairly deeply
indented here and having labels within switch blocks can be
difficult to read.

As such I would factor out this block as a separate read function.
That will make it a little more readable.

In the second case, the jump backwards just makes the code harder
to read than it currently is.

There is no firm rule about error handling in one place. If it
leads to more complex flow as here, don't do it.

Jonathan

> ---
>  drivers/iio/accel/stk8312.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index cacc0da2f874..5275ab886e39 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -344,24 +344,24 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
>  			return -EBUSY;
>  		mutex_lock(&data->lock);
>  		ret = stk8312_set_mode(data, data->mode | STK8312_MODE_ACTIVE);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			return ret;
> -		}
> +		if (ret)
> +			goto unlock;
> +
>  		ret = stk8312_read_accel(data, chan->address);
>  		if (ret < 0) {
>  			stk8312_set_mode(data,
>  					 data->mode & (~STK8312_MODE_ACTIVE));

Hmm. I'd like to factor out the set mode as well, but then we risk eating the
first error - so I suppose this is the best we can do.
> -			mutex_unlock(&data->lock);
> -			return ret;
> +			goto unlock;
>  		}
>  		*val = sign_extend32(ret, 7);
>  		ret = stk8312_set_mode(data,
>  				       data->mode & (~STK8312_MODE_ACTIVE));
> +unlock:
>  		mutex_unlock(&data->lock);
> -		if (ret < 0)
> -			return ret;
> -		return IIO_VAL_INT;
> +		if (!ret)
> +			ret = IIO_VAL_INT;

> +
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = stk8312_scale_table[data->range - 1][0];
>  		*val2 = stk8312_scale_table[data->range - 1][1];
> @@ -444,17 +444,15 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
>  						    data->buffer);
>  		if (ret < STK8312_ALL_CHANNEL_SIZE) {
>  			dev_err(&data->client->dev, "register read failed\n");
> -			mutex_unlock(&data->lock);
> -			goto err;
> +			goto unlock_after_failure;
>  		}
>  	} else {
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
>  				 indio_dev->masklength) {
>  			ret = stk8312_read_accel(data, bit);
> -			if (ret < 0) {
> -				mutex_unlock(&data->lock);
> -				goto err;
> -			}
> +			if (ret < 0)
> +				goto unlock_after_failure;
> +
>  			data->buffer[i++] = ret;
>  		}
>  	}
> @@ -462,10 +460,13 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>  					   pf->timestamp);
> -err:
> +notify_trigger:
>  	iio_trigger_notify_done(indio_dev->trig);
> -
>  	return IRQ_HANDLED;
> +
> +unlock_after_failure:
> +	mutex_unlock(&data->lock);

No. This construction fails the easy to read case.  The original code
was cleaner.  The reason to bring mutex_unlocks into one place is to simplify
the code.  That isn't the case here.

> +	goto notify_trigger;
>  }
>  
>  static irqreturn_t stk8312_data_rdy_trig_poll(int irq, void *private)

Re: iio/accel/stk8312: Improve unlocking of a mutex in two functions

[SF Markus Elfring]

> In the second case, the jump backwards just makes the code harder
> to read than it currently is.

Maybe …

But I proposed an other source code layout for useful reasons.


> There is no firm rule about error handling in one place.

There are some design options available.


> If it leads to more complex flow as here, don't do it.

I would appreciate to clarify such a view a bit more.
How would you like to achieve a complete and efficient
exception handling in shown places?

Regards,
Markus

Re: iio/accel/stk8312: Improve unlocking of a mutex in two functions

[Peter Meerwald-Stadler]

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

Hello,

> Maybe …
> But I proposed an other source code layout for useful reasons.

I think there is a (hidden) cost of having pure cleanup patches:
they make backporting fixes harder (across the cleanup)

stylistic changes must have a clear benefit, readability is subjective, 
consistency per se doesn't buy anything

the discussion how code should be written in the first place is separate
from the discussion what is worth fixing up lateron (IMHO)

> > There is no firm rule about error handling in one place.
> 
> There are some design options available.
> 
> 
> > If it leads to more complex flow as here, don't do it.
> 
> I would appreciate to clarify such a view a bit more.
> How would you like to achieve a complete and efficient
> exception handling in shown places?

regards, p.

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: iio/accel/stk8312: Improve unlocking of a mutex in two functions

[SF Markus Elfring]

>> But I proposed an other source code layout for useful reasons.
> 
> I think there is a (hidden) cost of having pure cleanup patches:
> they make backporting fixes harder (across the cleanup)

There the usual software development consequences to consider
if you dare to change the source code at all.


> stylistic changes must have a clear benefit, readability is subjective, 
> consistency per se doesn't buy anything

There are also different opinions involved.


> the discussion how code should be written in the first place is separate
> from the discussion what is worth fixing up lateron (IMHO)

Are you looking for a corresponding agreement?

Regards,
Markus