lkml.org 
[lkml]   [2017]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two functions
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

\
 
 \ /
  Last update: 2017-10-26 18:05    [W:0.061 / U:5.988 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site