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@xxxxxxxxxx> 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
>