Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAHcu+VbYrbWStNNaidDygY=d4f6F5tXUxWtsuH0x3oVeELGWVA@mail.gmail.com>
References: <20200724183954.1.I2e29ae25368ba8a72a9e44121cfbc36ead8ecc6b@changeid> <20200728151258.1222876-1-campello@chromium.org> <20200728091057.10.Ibe84fae61cd914c116e6d59ffeb644f1cbecd601@changeid> <159596523705.1360974.7169089810545711557@swboyd.mtv.corp.google.com> <CAHcu+VbYrbWStNNaidDygY=d4f6F5tXUxWtsuH0x3oVeELGWVA@mail.gmail.com>
Subject: Re: [PATCH 10/15] iio: sx9310: Simplify error return handling
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     LKML <devicetree@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Enrico Granata <egranata@chromium.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        linux-iio <linux-iio@vger.kernel.org>
To:     Daniel Campello <campello@chromium.org>
Date:   Tue, 28 Jul 2020 14:32:28 -0700
Message-ID: <159597194837.1360974.9212489704079396891@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Daniel Campello (2020-07-28 14:23:29)
> On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Daniel Campello (2020-07-28 08:12:53)
> > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310=
_data *data)
> > >  static int sx9310_read_proximity(struct sx9310_data *data,
> > >                                  const struct iio_chan_spec *chan, in=
t *val)
> > >  {
> > > -       int ret =3D 0;
> > > +       int ret;
> > >         __be16 rawval;
> > >
> > >         mutex_lock(&data->mutex);
> > >
> > >         ret =3D sx9310_get_read_channel(data, chan->channel);
> > > -       if (ret < 0)
> > > +       if (ret)
> > >                 goto out;
> > >
> > >         if (data->client->irq) {
> > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_=
data *data,
> > >
> > >         mutex_lock(&data->mutex);
> > >
> > > -       if (ret < 0)
> > > +       if (ret)
> > >                 goto out_disable_irq;
> >
> > Why is this condition checked after grabbing the mutex? Shouldn't it be
> > checked before grabbing the mutex? Or is that supposed to be a
> > mutex_unlock()?
> We acquire the lock before jumping to out_disable_irq which is before
> a mutex_unlock()

Does this function need to hold the mutex lock around get/put_read_channel?
It drops the lock while waiting and then regrabs it which seems to
imply that another reader could come in and try to get the channel again
during the wait. So put another way, it may be simpler to shorten the
lock area and then bail out of this function to a place where the lock
isn't held already on the return path.