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

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

[SF Markus Elfring]

From: Markus Elfring <elfring@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 <elfring@users.sourceforge.net>
---
 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(&data->mutex);
 	ret = bmg160_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val);
 	if (ret < 0) {
 		dev_err(dev, "Error reading reg_temp\n");
 		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock;
 	}
 
 	*val = sign_extend32(raw_val, 7);
 	ret = bmg160_set_power_state(data, false);
+unlock:
 	mutex_unlock(&data->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(&data->mutex);
 	ret = bmg160_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val,
 			       sizeof(raw_val));
 	if (ret < 0) {
 		dev_err(dev, "Error reading axis %d\n", axis);
 		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock;
 	}
 
 	*val = sign_extend32(le16_to_cpu(raw_val), 15);
 	ret = bmg160_set_power_state(data, false);
+unlock:
 	mutex_unlock(&data->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(&data->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(&data->mutex);
-			return ret;
+			goto unlock;
 		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->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(&data->mutex);
-			return ret;
+			goto unlock;
 		}
 		ret = bmg160_set_filter(data, val);
 		if (ret < 0) {
 			bmg160_set_power_state(data, false);
-			mutex_unlock(&data->mutex);
-			return ret;
+			goto unlock;
 		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->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(&data->mutex);
 		/* Refer to comments above for the suspend mode ops */
 		ret = bmg160_set_power_state(data, true);
-		if (ret < 0) {
-			mutex_unlock(&data->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(&data->mutex);
-			return ret;
+			goto unlock;
 		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+
+		goto set_power_state;
 	default:
 		return -EINVAL;
 	}
 
 	return -EINVAL;
+
+set_power_state:
+	ret = bmg160_set_power_state(data, false);
+unlock:
+	mutex_unlock(&data->mutex);
+	return ret;
 }
 
 static int bmg160_read_event(struct iio_dev *indio_dev,
@@ -763,8 +756,8 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
 
 	if (!state && data->motion_trigger_on) {
 		data->ev_enable_state = 0;
-		mutex_unlock(&data->mutex);
-		return 0;
+		ret = 0;
+		goto unlock;
 	}
 	/*
 	 * We will expect the enable and disable to do operation in
@@ -776,22 +769,19 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
 	 * is always on so sequence doesn't matter
 	 */
 	ret = bmg160_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	ret =  bmg160_setup_any_motion_interrupt(data, state);
 	if (ret < 0) {
 		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock;
 	}
 
 	data->ev_enable_state = state;
+unlock:
 	mutex_unlock(&data->mutex);
-
-	return 0;
+	return ret;
 }
 
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("100 200 400 1000 2000");
@@ -919,8 +909,8 @@ static int bmg160_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(&data->mutex);
-		return 0;
+		ret = 0;
+		goto unlock;
 	}
 
 	/*
@@ -928,27 +918,24 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig,
 	 * enable/disable operation order
 	 */
 	ret = bmg160_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
+
 	if (data->motion_trig == trig)
 		ret =  bmg160_setup_any_motion_interrupt(data, state);
 	else
 		ret = bmg160_setup_new_data_interrupt(data, state);
 	if (ret < 0) {
 		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock;
 	}
 	if (data->motion_trig == trig)
 		data->motion_trigger_on = state;
 	else
 		data->dready_trigger_on = state;
-
+unlock:
 	mutex_unlock(&data->mutex);
-
-	return 0;
+	return ret;
 }
 
 static const struct iio_trigger_ops bmg160_trigger_ops = {
-- 
2.16.2

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

[Jonathan Cameron]

On Wed, 14 Mar 2018 16:15:32 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@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 <elfring@users.sourceforge.net>

Hi Markus,

Some of these are good and sensible changes - others break the code.
Please be careful to fully check all the resulting paths and ensure
we don't change wether the lock is still held in all exit paths.
Note a function that isn't lockdep annotated should not be holding
any locks, that it took, upon exit.

> ---
>  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(&data->mutex);
>  	ret = bmg160_set_power_state(data, true);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>  
>  	ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val);
>  	if (ret < 0) {
>  		dev_err(dev, "Error reading reg_temp\n");
>  		bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock;
>  	}
>  
>  	*val = sign_extend32(raw_val, 7);
>  	ret = bmg160_set_power_state(data, false);
> +unlock:
>  	mutex_unlock(&data->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(&data->mutex);
>  	ret = bmg160_set_power_state(data, true);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>  
>  	ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val,
>  			       sizeof(raw_val));
>  	if (ret < 0) {
>  		dev_err(dev, "Error reading axis %d\n", axis);
>  		bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock;
>  	}
>  
>  	*val = sign_extend32(le16_to_cpu(raw_val), 15);
>  	ret = bmg160_set_power_state(data, false);
> +unlock:
>  	mutex_unlock(&data->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(&data->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(&data->mutex);
> -			return ret;
> +			goto unlock;
>  		}
> -		ret = bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->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(&data->mutex);
> -			return ret;
> +			goto unlock;
>  		}
>  		ret = bmg160_set_filter(data, val);
>  		if (ret < 0) {
>  			bmg160_set_power_state(data, false);
> -			mutex_unlock(&data->mutex);
> -			return ret;
> +			goto unlock;
>  		}
> -		ret = bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->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(&data->mutex);
>  		/* Refer to comments above for the suspend mode ops */
>  		ret = bmg160_set_power_state(data, true);
> -		if (ret < 0) {
> -			mutex_unlock(&data->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(&data->mutex);
> -			return ret;
> +			goto unlock;
>  		}
> -		ret = bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
Please keep the mutex_unlock in the same scope as the
mutex_lock.

I may make sense to take both outside the switch statement but
that needs careful consideration.

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

>  	}
>  
>  	return -EINVAL;
Mutex is still locked here and the return is wrong.
> +
> +set_power_state:
> +	ret = bmg160_set_power_state(data, false);
> +unlock:
> +	mutex_unlock(&data->mutex);
blank line before the return.

> +	return ret;
>  }
>  
>  static int bmg160_read_event(struct iio_dev *indio_dev,
> @@ -763,8 +756,8 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
>  
>  	if (!state && data->motion_trigger_on) {
>  		data->ev_enable_state = 0;
> -		mutex_unlock(&data->mutex);
> -		return 0;
> +		ret = 0;
Put this in as the value at instantiation.
int ret = 0;
> +		goto unlock;
>  	}
>  	/*
>  	 * We will expect the enable and disable to do operation in
> @@ -776,22 +769,19 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
>  	 * is always on so sequence doesn't matter
>  	 */
>  	ret = bmg160_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>  
>  	ret =  bmg160_setup_any_motion_interrupt(data, state);
>  	if (ret < 0) {
>  		bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock;
>  	}
>  
>  	data->ev_enable_state = state;
> +unlock:
>  	mutex_unlock(&data->mutex);
> -
Blank line preferred before the return ret.
> -	return 0;
> +	return ret;
>  }
>  
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("100 200 400 1000 2000");
> @@ -919,8 +909,8 @@ static int bmg160_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(&data->mutex);
> -		return 0;
> +		ret = 0;
Setting ret where it is originally defined to 0 would be tidier.

int ret = 0;

> +		goto unlock;
>  	}
>  
>  	/*
> @@ -928,27 +918,24 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  	 * enable/disable operation order
>  	 */
>  	ret = bmg160_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
> +
>  	if (data->motion_trig == trig)
>  		ret =  bmg160_setup_any_motion_interrupt(data, state);
>  	else
>  		ret = bmg160_setup_new_data_interrupt(data, state);
>  	if (ret < 0) {
>  		bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock;
>  	}
>  	if (data->motion_trig == trig)
>  		data->motion_trigger_on = state;
>  	else
>  		data->dready_trigger_on = state;
> -
> +unlock:
>  	mutex_unlock(&data->mutex);
> -
> -	return 0;
I would prefer a blank line between the mutex_unlock and the return.

> +	return ret;
>  }
>  
>  static const struct iio_trigger_ops bmg160_trigger_ops = {

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 <elfring@users.sourceforge.net> wrote:
> 
>> From: Markus Elfring <elfring@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 <elfring@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: [PATCH] iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[Greg Kroah-Hartman]

On Sat, Mar 17, 2018 at 07:54:22PM +0000, Jonathan Cameron wrote:
> On Wed, 14 Mar 2018 16:15:32 +0100
> SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> 
> > From: Markus Elfring <elfring@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 <elfring@users.sourceforge.net>
> 
> Hi Markus,
> 
> Some of these are good and sensible changes - others break the code.
> Please be careful to fully check all the resulting paths and ensure
> we don't change wether the lock is still held in all exit paths.
> Note a function that isn't lockdep annotated should not be holding
> any locks, that it took, upon exit.

Please note that you are dealing with someone who is on many kernel
maintainer's email-blacklist, including my own.  I've found it's just
not worth the time and energy in responding to these emails, it's a
time-sink you will never find your way out of.

good luck!

greg k-h

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

[Jonathan Cameron]

On Sun, 18 Mar 2018 09:19:47 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> Am 17.03.2018 um 20:54 schrieb Jonathan Cameron:
> > On Wed, 14 Mar 2018 16:15:32 +0100
> > SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> >   
> >> From: Markus Elfring <elfring@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 <elfring@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?
> 
You are correct, this does however reflect the issue with the resulting
lack of balance here.  I saw the mutex was getting unlocked outside
the local scope and so assumed that it was also take outside the local
scope.  That isn't true, so we have hurt readability.

It might make sense to move the lock and unlock outside the switch statement
but we certainly don't want to the the confusion that the lack of balance is
causing here.

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.

Jonathan

> 
> >   
> >>  	}
> >>  
> >>  	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]

>> 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.

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

Regards,
Markus