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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

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