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

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 16:26:29 +0200

Add a jump target so that a call of the function "mutex_unlock" is mostly
stored at the end of these function implementations.
Replace five 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/bmc150-accel-core.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 870f92ef61c2..f2a85a11a5e4 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -554,18 +554,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
 
 	mutex_lock(&data->mutex);
 	ret = bmc150_accel_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock_after_failure;
 
 	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
 			       &raw_val, sizeof(raw_val));
 	if (ret < 0) {
 		dev_err(dev, "Error reading axis %d\n", axis);
 		bmc150_accel_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock_after_failure;
 	}
 	*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
 			     chan->scan_type.realbits - 1);
@@ -575,6 +572,10 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
 		return ret;
 
 	return IIO_VAL_INT;
+
+unlock_after_failure:
+	mutex_unlock(&data->mutex);
+	return ret;
 }
 
 static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
@@ -1170,28 +1171,23 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
 	mutex_lock(&data->mutex);
 
 	if (t->enabled == state) {
-		mutex_unlock(&data->mutex);
-		return 0;
+		ret = 0;
+		goto unlock;
 	}
 
 	if (t->setup) {
 		ret = t->setup(t, state);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			return ret;
-		}
+		if (ret < 0)
+			goto unlock;
 	}
 
 	ret = bmc150_accel_set_interrupt(data, t->intr, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	t->enabled = state;
-
+unlock:
 	mutex_unlock(&data->mutex);
-
 	return ret;
 }
 
-- 
2.14.3

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Oct 2017 16:26:29 +0200

Add a jump target so that a call of the function "mutex_unlock" is mostly
stored at the end of these function implementations.
Replace five 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/bmc150-accel-core.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 870f92ef61c2..f2a85a11a5e4 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -554,18 +554,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
 
 	mutex_lock(&data->mutex);
 	ret = bmc150_accel_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock_after_failure;
 
 	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
 			       &raw_val, sizeof(raw_val));
 	if (ret < 0) {
 		dev_err(dev, "Error reading axis %d\n", axis);
 		bmc150_accel_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		goto unlock_after_failure;
 	}
 	*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
 			     chan->scan_type.realbits - 1);
@@ -575,6 +572,10 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
 		return ret;
 
 	return IIO_VAL_INT;
+
+unlock_after_failure:
+	mutex_unlock(&data->mutex);
+	return ret;
 }
 
 static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
@@ -1170,28 +1171,23 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
 	mutex_lock(&data->mutex);
 
 	if (t->enabled = state) {
-		mutex_unlock(&data->mutex);
-		return 0;
+		ret = 0;
+		goto unlock;
 	}
 
 	if (t->setup) {
 		ret = t->setup(t, state);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			return ret;
-		}
+		if (ret < 0)
+			goto unlock;
 	}
 
 	ret = bmc150_accel_set_interrupt(data, t->intr, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	t->enabled = state;
-
+unlock:
 	mutex_unlock(&data->mutex);
-
 	return ret;
 }
 
-- 
2.14.3

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

[Hans de Goede]

Hi,

On 25-10-17 16:33, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 25 Oct 2017 16:26:29 +0200
> 
> Add a jump target so that a call of the function "mutex_unlock" is mostly
> stored at the end of these function implementations.
> Replace five 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/bmc150-accel-core.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 870f92ef61c2..f2a85a11a5e4 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -554,18 +554,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>   
>   	mutex_lock(&data->mutex);
>   	ret = bmc150_accel_set_power_state(data, true);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock_after_failure;
>   
>   	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
>   			       &raw_val, sizeof(raw_val));
>   	if (ret < 0) {
>   		dev_err(dev, "Error reading axis %d\n", axis);
>   		bmc150_accel_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock_after_failure;
>   	}
>   	*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
>   			     chan->scan_type.realbits - 1);
> @@ -575,6 +572,10 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>   		return ret;
>   
>   	return IIO_VAL_INT;
> +
> +unlock_after_failure:
> +	mutex_unlock(&data->mutex);
> +	return ret;
>   }
>   
>   static int bmc150_accel_read_raw(struct iio_dev *indio_dev,

IMHO, if you do this, you should rework the function so that there is a single unlock call
at the end, not a separate one in in error label.

Could e.g. change this:

         ret = bmc150_accel_set_power_state(data, false);
         mutex_unlock(&data->mutex);
         if (ret < 0)
                 return ret;

         return IIO_VAL_INT;
}

To:

         ret = bmc150_accel_set_power_state(data, false);
         if (ret < 0)
                 goto unlock;

	ret = IIO_VAL_INT;
unlock:
         mutex_unlock(&data->mutex);

         return ret;
}

And also use the unlock label in the other cases, this is actually
quite a normal pattern. I see little use in a patch like this if there
are still 2 unlock paths after the patch.

Regards,

Hans





> @@ -1170,28 +1171,23 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
>   	mutex_lock(&data->mutex);
>   
>   	if (t->enabled == state) {
> -		mutex_unlock(&data->mutex);
> -		return 0;
> +		ret = 0;
> +		goto unlock;
>   	}
>   
>   	if (t->setup) {
>   		ret = t->setup(t, state);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto unlock;
>   	}
>   
>   	ret = bmc150_accel_set_interrupt(data, t->intr, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>   
>   	t->enabled = state;
> -
> +unlock:
>   	mutex_unlock(&data->mutex);
> -
>   	return ret;
>   }
>   
> 

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

[Hans de Goede]

Hi,

On 25-10-17 16:33, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 25 Oct 2017 16:26:29 +0200
> 
> Add a jump target so that a call of the function "mutex_unlock" is mostly
> stored at the end of these function implementations.
> Replace five 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/bmc150-accel-core.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 870f92ef61c2..f2a85a11a5e4 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -554,18 +554,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>   
>   	mutex_lock(&data->mutex);
>   	ret = bmc150_accel_set_power_state(data, true);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock_after_failure;
>   
>   	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
>   			       &raw_val, sizeof(raw_val));
>   	if (ret < 0) {
>   		dev_err(dev, "Error reading axis %d\n", axis);
>   		bmc150_accel_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		goto unlock_after_failure;
>   	}
>   	*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
>   			     chan->scan_type.realbits - 1);
> @@ -575,6 +572,10 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>   		return ret;
>   
>   	return IIO_VAL_INT;
> +
> +unlock_after_failure:
> +	mutex_unlock(&data->mutex);
> +	return ret;
>   }
>   
>   static int bmc150_accel_read_raw(struct iio_dev *indio_dev,

IMHO, if you do this, you should rework the function so that there is a single unlock call
at the end, not a separate one in in error label.

Could e.g. change this:

         ret = bmc150_accel_set_power_state(data, false);
         mutex_unlock(&data->mutex);
         if (ret < 0)
                 return ret;

         return IIO_VAL_INT;
}

To:

         ret = bmc150_accel_set_power_state(data, false);
         if (ret < 0)
                 goto unlock;

	ret = IIO_VAL_INT;
unlock:
         mutex_unlock(&data->mutex);

         return ret;
}

And also use the unlock label in the other cases, this is actually
quite a normal pattern. I see little use in a patch like this if there
are still 2 unlock paths after the patch.

Regards,

Hans





> @@ -1170,28 +1171,23 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
>   	mutex_lock(&data->mutex);
>   
>   	if (t->enabled = state) {
> -		mutex_unlock(&data->mutex);
> -		return 0;
> +		ret = 0;
> +		goto unlock;
>   	}
>   
>   	if (t->setup) {
>   		ret = t->setup(t, state);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto unlock;
>   	}
>   
>   	ret = bmc150_accel_set_interrupt(data, t->intr, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>   
>   	t->enabled = state;
> -
> +unlock:
>   	mutex_unlock(&data->mutex);
> -
>   	return ret;
>   }
>   
> 

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

[SF Markus Elfring]

> IMHO, if you do this, you should rework the function so that there is a single unlock call
> at the end, not a separate one in in error label.

Thanks for your update suggestion.

Does it indicate that I may propose similar source code adjustments
in this software area?


> Could e.g. change this:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         mutex_unlock(&data->mutex);
>         if (ret < 0)
>                 return ret;
> 
>         return IIO_VAL_INT;
> }
> 
> To:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         if (ret < 0)
>                 goto unlock;
> 
>     ret = IIO_VAL_INT;

How do you think about to use the following code variant then?

	if (!ret)
		ret = IIO_VAL_INT;


> unlock:
>         mutex_unlock(&data->mutex);
> 
>         return ret;
> }
> 
> And also use the unlock label in the other cases, this is actually
> quite a normal pattern. I see little use in a patch like this if there
> are still 2 unlock paths after the patch.

How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?

Regards,
Markus

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

[SF Markus Elfring]

> IMHO, if you do this, you should rework the function so that there is a single unlock call
> at the end, not a separate one in in error label.

Thanks for your update suggestion.

Does it indicate that I may propose similar source code adjustments
in this software area?


> Could e.g. change this:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         mutex_unlock(&data->mutex);
>         if (ret < 0)
>                 return ret;
> 
>         return IIO_VAL_INT;
> }
> 
> To:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         if (ret < 0)
>                 goto unlock;
> 
>     ret = IIO_VAL_INT;

How do you think about to use the following code variant then?

	if (!ret)
		ret = IIO_VAL_INT;


> unlock:
>         mutex_unlock(&data->mutex);
> 
>         return ret;
> }
> 
> And also use the unlock label in the other cases, this is actually
> quite a normal pattern. I see little use in a patch like this if there
> are still 2 unlock paths after the patch.

How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?

Regards,
Markus

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

[Hans de Goede]

Hi,

On 25-10-17 18:15, SF Markus Elfring wrote:
>> IMHO, if you do this, you should rework the function so that there is a single unlock call
>> at the end, not a separate one in in error label.
> 
> Thanks for your update suggestion.
> 
> Does it indicate that I may propose similar source code adjustments
> in this software area?
> 
> 
>> Could e.g. change this:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          mutex_unlock(&data->mutex);
>>          if (ret < 0)
>>                  return ret;
>>
>>          return IIO_VAL_INT;
>> }
>>
>> To:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          if (ret < 0)
>>                  goto unlock;
>>
>>      ret = IIO_VAL_INT;

If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is even
better.


> How do you think about to use the following code variant then?
> 
> 	if (!ret)
> 		ret = IIO_VAL_INT;


I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better, because that
way the error handling is consistent between all steps and if
another step is later added at the end, the last step will
not require modification.

>> unlock:
>>          mutex_unlock(&data->mutex);
>>
>>          return ret;
>> }
>>
>> And also use the unlock label in the other cases, this is actually
>> quite a normal pattern. I see little use in a patch like this if there
>> are still 2 unlock paths after the patch.
> 
> How long should I wait for corresponding feedback before another small
> source code adjustment will be appropriate?

That is hard to say. I usually just do a new version when I've time,
seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :)  There is really no clear rule here.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 25-10-17 18:15, SF Markus Elfring wrote:
>> IMHO, if you do this, you should rework the function so that there is a single unlock call
>> at the end, not a separate one in in error label.
> 
> Thanks for your update suggestion.
> 
> Does it indicate that I may propose similar source code adjustments
> in this software area?
> 
> 
>> Could e.g. change this:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          mutex_unlock(&data->mutex);
>>          if (ret < 0)
>>                  return ret;
>>
>>          return IIO_VAL_INT;
>> }
>>
>> To:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          if (ret < 0)
>>                  goto unlock;
>>
>>      ret = IIO_VAL_INT;

If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is even
better.


> How do you think about to use the following code variant then?
> 
> 	if (!ret)
> 		ret = IIO_VAL_INT;


I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better, because that
way the error handling is consistent between all steps and if
another step is later added at the end, the last step will
not require modification.

>> unlock:
>>          mutex_unlock(&data->mutex);
>>
>>          return ret;
>> }
>>
>> And also use the unlock label in the other cases, this is actually
>> quite a normal pattern. I see little use in a patch like this if there
>> are still 2 unlock paths after the patch.
> 
> How long should I wait for corresponding feedback before another small
> source code adjustment will be appropriate?

That is hard to say. I usually just do a new version when I've time,
seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :)  There is really no clear rule here.

Regards,

Hans

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

[SF Markus Elfring]

> If that is the only unlock in the function, then it is probably
> best to keep things as is. In general gotos are considered
> better then multiple unlocks, but not having either is
> even better.

Thanks for your quick feedback.


>> How do you think about to use the following code variant then?
>>
>>     if (!ret)
>>         ret = IIO_VAL_INT;
> 
> 
> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> directly above the unlock label variant is better,

I would prefer the approach above so that an extra goto statement
could also be avoided before.


> because that way the error handling is consistent between all steps
> and if another step is later added at the end, the last step will
> not require modification.

Do any more contributors insist on such a code layout?


>> How long should I wait for corresponding feedback before another small
>> source code adjustment will be appropriate?
> 
> That is hard to say.

I am just curious on how we can achieve progress here.


> I usually just do a new version when I've time,

This is generally fine.


> seldomly someone complains I should have waited longer for feedback
> (when I'm quite quick) but usually sending out a new version as soon
> as you've time to work on a new version is best, since if you wait
> you may then not have time for the entire next week or so, at least
> that is my experience :)  There is really no clear rule here.

I asked also because other well-known contributors showed strong
reactions for this change pattern (with the help of a script
for the semantic patch language).
Would you care for similar updates in source files like the following?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304

Regards,
Markus

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

[SF Markus Elfring]

> If that is the only unlock in the function, then it is probably
> best to keep things as is. In general gotos are considered
> better then multiple unlocks, but not having either is
> even better.

Thanks for your quick feedback.


>> How do you think about to use the following code variant then?
>>
>>     if (!ret)
>>         ret = IIO_VAL_INT;
> 
> 
> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> directly above the unlock label variant is better,

I would prefer the approach above so that an extra goto statement
could also be avoided before.


> because that way the error handling is consistent between all steps
> and if another step is later added at the end, the last step will
> not require modification.

Do any more contributors insist on such a code layout?


>> How long should I wait for corresponding feedback before another small
>> source code adjustment will be appropriate?
> 
> That is hard to say.

I am just curious on how we can achieve progress here.


> I usually just do a new version when I've time,

This is generally fine.


> seldomly someone complains I should have waited longer for feedback
> (when I'm quite quick) but usually sending out a new version as soon
> as you've time to work on a new version is best, since if you wait
> you may then not have time for the entire next week or so, at least
> that is my experience :)  There is really no clear rule here.

I asked also because other well-known contributors showed strong
reactions for this change pattern (with the help of a script
for the semantic patch language).
Would you care for similar updates in source files like the following?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id\x1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id6ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id6ef71cae353f88fd6e095e2aaa3e5953af1685d#n304

Regards,
Markus

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

[Hans de Goede]

Hi,

On 25-10-17 18:58, SF Markus Elfring wrote:
>> If that is the only unlock in the function, then it is probably
>> best to keep things as is. In general gotos are considered
>> better then multiple unlocks, but not having either is
>> even better.
> 
> Thanks for your quick feedback.
> 
> 
>>> How do you think about to use the following code variant then?
>>>
>>>      if (!ret)
>>>          ret = IIO_VAL_INT;
>>
>>
>> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
>> directly above the unlock label variant is better,
> 
> I would prefer the approach above so that an extra goto statement
> could also be avoided before.

Usually code in a function follows a pattern of:

	err = step1()
	if (err)
		handle-err


	err = step2()
	if (err)
		handle-err


	err = step3()
	if (err)
		handle-err

What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.

>> because that way the error handling is consistent between all steps
>> and if another step is later added at the end, the last step will
>> not require modification.
> 
> Do any more contributors insist on such a code layout?

There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.

>>> How long should I wait for corresponding feedback before another small
>>> source code adjustment will be appropriate?
>>
>> That is hard to say.
> 
> I am just curious on how we can achieve progress here.
> 
> 
>> I usually just do a new version when I've time,
> 
> This is generally fine.
> 
> 
>> seldomly someone complains I should have waited longer for feedback
>> (when I'm quite quick) but usually sending out a new version as soon
>> as you've time to work on a new version is best, since if you wait
>> you may then not have time for the entire next week or so, at least
>> that is my experience :)  There is really no clear rule here.
> 
> I asked also because other well-known contributors showed strong
> reactions for this change pattern (with the help of a script
> for the semantic patch language).
> Would you care for similar updates in source files like the following?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760

So I just checked this one, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask == IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.

To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).

Basically I believe there is no one-size fits all solution
here and refactoring like this may introduce bugs, so one
needs to weight the amount of work + regression risk vs
the benefits of the code being cleaner going forward.

Regards,

Hans



> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304
> 
> Regards,
> Markus
> 

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

[Hans de Goede]

Hi,

On 25-10-17 18:58, SF Markus Elfring wrote:
>> If that is the only unlock in the function, then it is probably
>> best to keep things as is. In general gotos are considered
>> better then multiple unlocks, but not having either is
>> even better.
> 
> Thanks for your quick feedback.
> 
> 
>>> How do you think about to use the following code variant then?
>>>
>>>      if (!ret)
>>>          ret = IIO_VAL_INT;
>>
>>
>> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
>> directly above the unlock label variant is better,
> 
> I would prefer the approach above so that an extra goto statement
> could also be avoided before.

Usually code in a function follows a pattern of:

	err = step1()
	if (err)
		handle-err


	err = step2()
	if (err)
		handle-err


	err = step3()
	if (err)
		handle-err

What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.

>> because that way the error handling is consistent between all steps
>> and if another step is later added at the end, the last step will
>> not require modification.
> 
> Do any more contributors insist on such a code layout?

There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.

>>> How long should I wait for corresponding feedback before another small
>>> source code adjustment will be appropriate?
>>
>> That is hard to say.
> 
> I am just curious on how we can achieve progress here.
> 
> 
>> I usually just do a new version when I've time,
> 
> This is generally fine.
> 
> 
>> seldomly someone complains I should have waited longer for feedback
>> (when I'm quite quick) but usually sending out a new version as soon
>> as you've time to work on a new version is best, since if you wait
>> you may then not have time for the entire next week or so, at least
>> that is my experience :)  There is really no clear rule here.
> 
> I asked also because other well-known contributors showed strong
> reactions for this change pattern (with the help of a script
> for the semantic patch language).
> Would you care for similar updates in source files like the following?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id\x1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760

So I just checked this one, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask = IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.

To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).

Basically I believe there is no one-size fits all solution
here and refactoring like this may introduce bugs, so one
needs to weight the amount of work + regression risk vs
the benefits of the code being cleaner going forward.

Regards,

Hans



> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id6ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id6ef71cae353f88fd6e095e2aaa3e5953af1685d#n304
> 
> Regards,
> Markus
> 

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

[SF Markus Elfring]

> What you are suggesting breaks this pattern

I might be looking for an other balance between involved implementation
details after your constructive feedback for my first approach
in this software module.


> (not using a goto in the last if (err) case)

I would find it nice when a bit more code reduction is feasible.


> which makes the code harder to read and makes things harder
> (and potentially leads to introducing bugs) when
> a step4() gets added.

There is a choice between conservative adjustments and progressive
software refactoring where both directions can lead to similar
development risks.


>>> because that way the error handling is consistent between all steps
>>> and if another step is later added at the end, the last step will
>>> not require modification.

Such a view might express a change resistance.


>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
> 
> So I just checked this one,

Thanks for your interest.


> this one is tricky because the lock is taking inside
> a switch-case and doing gotos inside the case is not pretty.

I imagine that I would like to use scoped lock objects
in affected source files. (Other programming languages support
such synchronisation constructs directly.)


> Basically I believe there is no one-size fits all solution
> here and refactoring like this may introduce bugs, so one
> needs to weight the amount of work + regression risk vs
> the benefits of the code being cleaner going forward.

It seems that our software development discussion can be
continued in a constructive way then.

Regards,
Markus

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

[SF Markus Elfring]

> What you are suggesting breaks this pattern

I might be looking for an other balance between involved implementation
details after your constructive feedback for my first approach
in this software module.


> (not using a goto in the last if (err) case)

I would find it nice when a bit more code reduction is feasible.


> which makes the code harder to read and makes things harder
> (and potentially leads to introducing bugs) when
> a step4() gets added.

There is a choice between conservative adjustments and progressive
software refactoring where both directions can lead to similar
development risks.


>>> because that way the error handling is consistent between all steps
>>> and if another step is later added at the end, the last step will
>>> not require modification.

Such a view might express a change resistance.


>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id\x1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
> 
> So I just checked this one,

Thanks for your interest.


> this one is tricky because the lock is taking inside
> a switch-case and doing gotos inside the case is not pretty.

I imagine that I would like to use scoped lock objects
in affected source files. (Other programming languages support
such synchronisation constructs directly.)


> Basically I believe there is no one-size fits all solution
> here and refactoring like this may introduce bugs, so one
> needs to weight the amount of work + regression risk vs
> the benefits of the code being cleaner going forward.

It seems that our software development discussion can be
continued in a constructive way then.

Regards,
Markus

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

[Jonathan Cameron]

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

> > What you are suggesting breaks this pattern  
> 
> I might be looking for an other balance between involved implementation
> details after your constructive feedback for my first approach
> in this software module.
> 
> 
> > (not using a goto in the last if (err) case)  
> 
> I would find it nice when a bit more code reduction is feasible.
> 
> 
> > which makes the code harder to read and makes things harder
> > (and potentially leads to introducing bugs) when
> > a step4() gets added.  
> 
> There is a choice between conservative adjustments and progressive
> software refactoring where both directions can lead to similar
> development risks.
> 
> 
> >>> because that way the error handling is consistent between all steps
> >>> and if another step is later added at the end, the last step will
> >>> not require modification.  
> 
> Such a view might express a change resistance.
> 
> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760  
> > 
> > So I just checked this one,  
> 
> Thanks for your interest.
> 
> 
> > this one is tricky because the lock is taking inside
> > a switch-case and doing gotos inside the case is not pretty.  
> 
> I imagine that I would like to use scoped lock objects
> in affected source files. (Other programming languages support
> such synchronisation constructs directly.)

I'd keep it simple for now. Also, I'd actually take a different
approach to tidy up this case we are talking about here.

Factor out the whole case IIO_CHAN_INFO_RAW block as a 
utility function.  Then nice clean and simple lock handling
can be done in the error paths without the readability problems
that you get doing it deeply nested.

Btw. There is another issue in that code that needs fixing
which is that it will race with the buffer being enabled.
It should be using the iio_claim_direct infrastructure to
prevent that cleanly. 

That example is definitely more ugly that it needs to be
so would be nice to clean it up if you have time.

Thanks,

Jonathan
> 
> 
> > Basically I believe there is no one-size fits all solution
> > here and refactoring like this may introduce bugs, so one
> > needs to weight the amount of work + regression risk vs
> > the benefits of the code being cleaner going forward.  
> 
> It seems that our software development discussion can be
> continued in a constructive way then.
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Cameron]

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

> > What you are suggesting breaks this pattern  
> 
> I might be looking for an other balance between involved implementation
> details after your constructive feedback for my first approach
> in this software module.
> 
> 
> > (not using a goto in the last if (err) case)  
> 
> I would find it nice when a bit more code reduction is feasible.
> 
> 
> > which makes the code harder to read and makes things harder
> > (and potentially leads to introducing bugs) when
> > a step4() gets added.  
> 
> There is a choice between conservative adjustments and progressive
> software refactoring where both directions can lead to similar
> development risks.
> 
> 
> >>> because that way the error handling is consistent between all steps
> >>> and if another step is later added at the end, the last step will
> >>> not require modification.  
> 
> Such a view might express a change resistance.
> 
> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id\x1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760  
> > 
> > So I just checked this one,  
> 
> Thanks for your interest.
> 
> 
> > this one is tricky because the lock is taking inside
> > a switch-case and doing gotos inside the case is not pretty.  
> 
> I imagine that I would like to use scoped lock objects
> in affected source files. (Other programming languages support
> such synchronisation constructs directly.)

I'd keep it simple for now. Also, I'd actually take a different
approach to tidy up this case we are talking about here.

Factor out the whole case IIO_CHAN_INFO_RAW block as a 
utility function.  Then nice clean and simple lock handling
can be done in the error paths without the readability problems
that you get doing it deeply nested.

Btw. There is another issue in that code that needs fixing
which is that it will race with the buffer being enabled.
It should be using the iio_claim_direct infrastructure to
prevent that cleanly. 

That example is definitely more ugly that it needs to be
so would be nice to clean it up if you have time.

Thanks,

Jonathan
> 
> 
> > Basically I believe there is no one-size fits all solution
> > here and refactoring like this may introduce bugs, so one
> > needs to weight the amount of work + regression risk vs
> > the benefits of the code being cleaner going forward.  
> 
> It seems that our software development discussion can be
> continued in a constructive way then.
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Cameron]

On Wed, 25 Oct 2017 18:22:02 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 25-10-17 18:15, SF Markus Elfring wrote:
> >> IMHO, if you do this, you should rework the function so that there is a single unlock call
> >> at the end, not a separate one in in error label.  
> > 
> > Thanks for your update suggestion.
> > 
> > Does it indicate that I may propose similar source code adjustments
> > in this software area?
> > 
> >   
> >> Could e.g. change this:
> >>
> >>          ret = bmc150_accel_set_power_state(data, false);
> >>          mutex_unlock(&data->mutex);
> >>          if (ret < 0)
> >>                  return ret;
> >>
> >>          return IIO_VAL_INT;
> >> }
> >>
> >> To:
> >>
> >>          ret = bmc150_accel_set_power_state(data, false);
> >>          if (ret < 0)
> >>                  goto unlock;
> >>
> >>      ret = IIO_VAL_INT;  
> 
> If that is the only unlock in the function, then it is probably
> best to keep things as is. In general gotos are considered
> better then multiple unlocks, but not having either is even
> better.
> 
> 
> > How do you think about to use the following code variant then?
> > 
> > 	if (!ret)
> > 		ret = IIO_VAL_INT;  
> 
> 
> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> directly above the unlock label variant is better, because that
> way the error handling is consistent between all steps and if
> another step is later added at the end, the last step will
> not require modification.
I agree, setting ret = IIO_VAL_INT in the good path unconditionally
is good.

However, it is not just the unlocking that would be nice to
unify here.  The call to:

bmc150_accel_set_power_state(data, false);

occurs in both the final two error paths and the good path.  An
additional label and appropriate gotos would clean that up
as well.

This driver also suffers from issues with racing against
the buffer enable check and buffers being enabled like
I mentioned in the other email.   Clearly more cases of
that around than I realised!  Patches welcome or I'll suggest
it as an outreachy cleanup task.

Jonathan

> 
> >> unlock:
> >>          mutex_unlock(&data->mutex);
> >>
> >>          return ret;
> >> }
> >>
> >> And also use the unlock label in the other cases, this is actually
> >> quite a normal pattern. I see little use in a patch like this if there
> >> are still 2 unlock paths after the patch.  
> > 
> > How long should I wait for corresponding feedback before another small
> > source code adjustment will be appropriate?  
> 
> That is hard to say. I usually just do a new version when I've time,
> seldomly someone complains I should have waited longer for feedback
> (when I'm quite quick) but usually sending out a new version as soon
> as you've time to work on a new version is best, since if you wait
> you may then not have time for the entire next week or so, at least
> that is my experience :)  There is really no clear rule here.
> 
> Regards,
> 
> Hans
> 

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

[Jonathan Cameron]

On Wed, 25 Oct 2017 18:22:02 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 25-10-17 18:15, SF Markus Elfring wrote:
> >> IMHO, if you do this, you should rework the function so that there is a single unlock call
> >> at the end, not a separate one in in error label.  
> > 
> > Thanks for your update suggestion.
> > 
> > Does it indicate that I may propose similar source code adjustments
> > in this software area?
> > 
> >   
> >> Could e.g. change this:
> >>
> >>          ret = bmc150_accel_set_power_state(data, false);
> >>          mutex_unlock(&data->mutex);
> >>          if (ret < 0)
> >>                  return ret;
> >>
> >>          return IIO_VAL_INT;
> >> }
> >>
> >> To:
> >>
> >>          ret = bmc150_accel_set_power_state(data, false);
> >>          if (ret < 0)
> >>                  goto unlock;
> >>
> >>      ret = IIO_VAL_INT;  
> 
> If that is the only unlock in the function, then it is probably
> best to keep things as is. In general gotos are considered
> better then multiple unlocks, but not having either is even
> better.
> 
> 
> > How do you think about to use the following code variant then?
> > 
> > 	if (!ret)
> > 		ret = IIO_VAL_INT;  
> 
> 
> I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> directly above the unlock label variant is better, because that
> way the error handling is consistent between all steps and if
> another step is later added at the end, the last step will
> not require modification.
I agree, setting ret = IIO_VAL_INT in the good path unconditionally
is good.

However, it is not just the unlocking that would be nice to
unify here.  The call to:

bmc150_accel_set_power_state(data, false);

occurs in both the final two error paths and the good path.  An
additional label and appropriate gotos would clean that up
as well.

This driver also suffers from issues with racing against
the buffer enable check and buffers being enabled like
I mentioned in the other email.   Clearly more cases of
that around than I realised!  Patches welcome or I'll suggest
it as an outreachy cleanup task.

Jonathan

> 
> >> unlock:
> >>          mutex_unlock(&data->mutex);
> >>
> >>          return ret;
> >> }
> >>
> >> And also use the unlock label in the other cases, this is actually
> >> quite a normal pattern. I see little use in a patch like this if there
> >> are still 2 unlock paths after the patch.  
> > 
> > How long should I wait for corresponding feedback before another small
> > source code adjustment will be appropriate?  
> 
> That is hard to say. I usually just do a new version when I've time,
> seldomly someone complains I should have waited longer for feedback
> (when I'm quite quick) but usually sending out a new version as soon
> as you've time to work on a new version is best, since if you wait
> you may then not have time for the entire next week or so, at least
> that is my experience :)  There is really no clear rule here.
> 
> Regards,
> 
> Hans
> 

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

[Jonathan Cameron]

On Thu, 26 Oct 2017 16:51:13 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 25 Oct 2017 18:22:02 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > Hi,
> > 
> > On 25-10-17 18:15, SF Markus Elfring wrote:  
> > >> IMHO, if you do this, you should rework the function so that there is a single unlock call
> > >> at the end, not a separate one in in error label.    
> > > 
> > > Thanks for your update suggestion.
> > > 
> > > Does it indicate that I may propose similar source code adjustments
> > > in this software area?
> > > 
> > >     
> > >> Could e.g. change this:
> > >>
> > >>          ret = bmc150_accel_set_power_state(data, false);
> > >>          mutex_unlock(&data->mutex);
> > >>          if (ret < 0)
> > >>                  return ret;
> > >>
> > >>          return IIO_VAL_INT;
> > >> }
> > >>
> > >> To:
> > >>
> > >>          ret = bmc150_accel_set_power_state(data, false);
> > >>          if (ret < 0)
> > >>                  goto unlock;
> > >>
> > >>      ret = IIO_VAL_INT;    
> > 
> > If that is the only unlock in the function, then it is probably
> > best to keep things as is. In general gotos are considered
> > better then multiple unlocks, but not having either is even
> > better.
> > 
> >   
> > > How do you think about to use the following code variant then?
> > > 
> > > 	if (!ret)
> > > 		ret = IIO_VAL_INT;    
> > 
> > 
> > I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> > directly above the unlock label variant is better, because that
> > way the error handling is consistent between all steps and if
> > another step is later added at the end, the last step will
> > not require modification.  
> I agree, setting ret = IIO_VAL_INT in the good path unconditionally
> is good.
> 
> However, it is not just the unlocking that would be nice to
> unify here.  The call to:
> 
> bmc150_accel_set_power_state(data, false);
> 
> occurs in both the final two error paths and the good path.  An
> additional label and appropriate gotos would clean that up
> as well.

Ah my mistake, that would involve 'eating' the first error so
isn't a good idea.  Ignore this one!

Jonathan

> 
> This driver also suffers from issues with racing against
> the buffer enable check and buffers being enabled like
> I mentioned in the other email.   Clearly more cases of
> that around than I realised!  Patches welcome or I'll suggest
> it as an outreachy cleanup task.
> 
> Jonathan
> 
> >   
> > >> unlock:
> > >>          mutex_unlock(&data->mutex);
> > >>
> > >>          return ret;
> > >> }
> > >>
> > >> And also use the unlock label in the other cases, this is actually
> > >> quite a normal pattern. I see little use in a patch like this if there
> > >> are still 2 unlock paths after the patch.    
> > > 
> > > How long should I wait for corresponding feedback before another small
> > > source code adjustment will be appropriate?    
> > 
> > That is hard to say. I usually just do a new version when I've time,
> > seldomly someone complains I should have waited longer for feedback
> > (when I'm quite quick) but usually sending out a new version as soon
> > as you've time to work on a new version is best, since if you wait
> > you may then not have time for the entire next week or so, at least
> > that is my experience :)  There is really no clear rule here.
> > 
> > Regards,
> > 
> > Hans
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Cameron]

On Thu, 26 Oct 2017 16:51:13 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 25 Oct 2017 18:22:02 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > Hi,
> > 
> > On 25-10-17 18:15, SF Markus Elfring wrote:  
> > >> IMHO, if you do this, you should rework the function so that there is a single unlock call
> > >> at the end, not a separate one in in error label.    
> > > 
> > > Thanks for your update suggestion.
> > > 
> > > Does it indicate that I may propose similar source code adjustments
> > > in this software area?
> > > 
> > >     
> > >> Could e.g. change this:
> > >>
> > >>          ret = bmc150_accel_set_power_state(data, false);
> > >>          mutex_unlock(&data->mutex);
> > >>          if (ret < 0)
> > >>                  return ret;
> > >>
> > >>          return IIO_VAL_INT;
> > >> }
> > >>
> > >> To:
> > >>
> > >>          ret = bmc150_accel_set_power_state(data, false);
> > >>          if (ret < 0)
> > >>                  goto unlock;
> > >>
> > >>      ret = IIO_VAL_INT;    
> > 
> > If that is the only unlock in the function, then it is probably
> > best to keep things as is. In general gotos are considered
> > better then multiple unlocks, but not having either is even
> > better.
> > 
> >   
> > > How do you think about to use the following code variant then?
> > > 
> > > 	if (!ret)
> > > 		ret = IIO_VAL_INT;    
> > 
> > 
> > I believe the goto unlock variant and setting ret = IIO_VAL_INT;
> > directly above the unlock label variant is better, because that
> > way the error handling is consistent between all steps and if
> > another step is later added at the end, the last step will
> > not require modification.  
> I agree, setting ret = IIO_VAL_INT in the good path unconditionally
> is good.
> 
> However, it is not just the unlocking that would be nice to
> unify here.  The call to:
> 
> bmc150_accel_set_power_state(data, false);
> 
> occurs in both the final two error paths and the good path.  An
> additional label and appropriate gotos would clean that up
> as well.

Ah my mistake, that would involve 'eating' the first error so
isn't a good idea.  Ignore this one!

Jonathan

> 
> This driver also suffers from issues with racing against
> the buffer enable check and buffers being enabled like
> I mentioned in the other email.   Clearly more cases of
> that around than I realised!  Patches welcome or I'll suggest
> it as an outreachy cleanup task.
> 
> Jonathan
> 
> >   
> > >> unlock:
> > >>          mutex_unlock(&data->mutex);
> > >>
> > >>          return ret;
> > >> }
> > >>
> > >> And also use the unlock label in the other cases, this is actually
> > >> quite a normal pattern. I see little use in a patch like this if there
> > >> are still 2 unlock paths after the patch.    
> > > 
> > > How long should I wait for corresponding feedback before another small
> > > source code adjustment will be appropriate?    
> > 
> > That is hard to say. I usually just do a new version when I've time,
> > seldomly someone complains I should have waited longer for feedback
> > (when I'm quite quick) but usually sending out a new version as soon
> > as you've time to work on a new version is best, since if you wait
> > you may then not have time for the entire next week or so, at least
> > that is my experience :)  There is really no clear rule here.
> > 
> > Regards,
> > 
> > Hans
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html