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

[Jonathan Cameron]

On Sun, 18 Mar 2018 09:19:47 +0100
SF Markus Elfring  wrote:

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

[Jonathan Cameron]

On Sun, 18 Mar 2018 09:19:47 +0100
SF Markus Elfring  wrote:

> 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?
> 
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: [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 +, Jonathan Cameron wrote:
> 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 - 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: [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 +, Jonathan Cameron wrote:
> 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 - 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

[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

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

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  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 - 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(>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 = 

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  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 - 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(>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);

[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,