| Message ID | 66d582a4-a77e-cd78-4215-49587ec2259e@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > > -- 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
> 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 -- 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
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 -- 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
> 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 -- 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
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 > -- 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
> 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 -- 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
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 -- 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
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 > -- 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
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 -- 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
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; }