Quoting Daniel Campello (2020-07-28 14:23:29)
> On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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, int *val)
> > > {
> > > - int ret = 0;
> > > + int ret;
> > > __be16 rawval;
> > >
> > > mutex_lock(&data->mutex);
> > >
> > > ret = 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.
