Re: [PATCH] iio: health: max30100: remove mlock usage

[Jonathan Cameron]

On Mon, 16 Mar 2020 01:29:28 -0700
Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> wrote:

> On Mon, Mar 16, 2020 at 1:21 AM Matt Ranostay
> <matt.ranostay@xxxxxxxxxxxx> wrote:
> >
> > On Sun, Mar 15, 2020 at 2:46 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:  
> > >
> > > On Tue, 10 Mar 2020 00:01:28 +0530
> > > Rohit Sarkar <rohitsarkar5398@xxxxxxxxx> wrote:
> > >  
> > > > Use local lock instead of indio_dev's mlock.
> > > > The mlock was being used to protect local driver state thus using the
> > > > local lock is a better option here.
> > > >
> > > > Signed-off-by: Rohit Sarkar <rohitsarkar5398@xxxxxxxxx>  
> > >
> > > Matt.  Definitely need your input on this.
> > >  
> > > > ---
> > > >  drivers/iio/health/max30100.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> > > > index 84010501762d..8ddc4649547d 100644
> > > > --- a/drivers/iio/health/max30100.c
> > > > +++ b/drivers/iio/health/max30100.c
> > > > @@ -388,7 +388,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
> > > >                * Temperature reading can only be acquired while engine
> > > >                * is running
> > > >                */
> > > > -             mutex_lock(&indio_dev->mlock);
> > > > +             mutex_lock(&data->lock);  
> > >
> > > Hmm.. It's another complex one.  What is actually being protected here is
> > > the buffer state, but not to take it exclusively like claim_direct does.
> > >
> > > Here we need the inverse, we want to ensure we are 'not' in the direct
> > > mode because this hardware requires the buffer to be running to read the
> > > temperature.
> > >
> > > That is the sort of interface that is going to get userspace very
> > > confused.
> > >
> > > Matt, normally what I'd suggest here is that the temperature read should:
> > >
> > > 1) Claim direct mode, if it fails then do the dance you have here
> > > (with more comments to explain why you are taking an internal lock)
> > > 2) Start up capture as if we were in buffered mode
> > > 3) Grab that temp
> > > 4) stop capture to return to non buffered mode.
> > > 5) Release direct mode.
> > >
> > > I guess we decided it wasn't worth the hassle.
> > >
> > > So Rohit.  This one probably needs a comment rather than any change.
> > > We 'could' add a 'hold_buffered_mode' function that takes the mlock,
> > > verifies we are in buffered mode and continues to hold the lock
> > > until the 'release_buffered_mode'.  However, I'm not sure any other
> > > drivers do this particular dance, so clear commenting in the driver
> > > might be enough.   Should we ever change how mlock is used in the
> > > core, we'd have to fix this driver up as well.
> > >
> > > Hmm.  This is really hammering home that perhaps all the remaining
> > > mlock cases are 'hard'.  
> >
> > Heh really had to look this over what I was doing since it has been
> > almost half a decade now :).
> >
> > Think locking that mutex was only to prevent another read during the
> > temp reading, and not really
> > not sure how effective that is actually. Especially since the I2C
> > subsystem should handle those reads
> > in a queue like fashion.
> >
> > - Matt
> >  
> 
> So to be clear I think we can just remove the lock period since the
> odds of this actually being requested (or disabled) at the
> exact time so very remote. Along with the worse case being a failed read.

I disagree.   What that lock prevents is disabling buffered mode between
the check on whether it is enabled and the read.   That's a clear race
so we should keep the lock.

Jonathan


> 
> - Matt
> 
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >  
> > > >
> > > >               if (!iio_buffer_enabled(indio_dev))
> > > >                       ret = -EAGAIN;
> > > > @@ -399,7 +399,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
> > > >
> > > >               }
> > > >
> > > > -             mutex_unlock(&indio_dev->mlock);
> > > > +             mutex_unlock(&data->lock);
> > > >               break;
> > > >       case IIO_CHAN_INFO_SCALE:
> > > >               *val = 1;  /* 0.0625 */  
> > >