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

[Rohit Sarkar <rohitsarkar5398@gmail.com>]

On Sun, Mar 15, 2020 at 09:46:04AM +0000, Jonathan Cameron wrote:
> On Tue, 10 Mar 2020 00:01:28 +0530
> Rohit Sarkar <rohitsarkar5398@gmail.com> 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@gmail.com>
> 
> 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.

So just to check if I understand correctly, let's say we did not use any
lock in this case. If an execution thread reached the
iio_buffer_enabled() check and found the buffer to be enabled, but
the buffer got disabled simultaneously, the temperature readings that we
get will be corrupted. Does this make sense?

> 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.
Agreed

> 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.
The code already mentions that the "Temperature can only be acquired
while engine is running.", should I add something like "mlock is
acquired to protect the buffer state..." to the same comment.

> 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.
Understood.

> Hmm.  This is really hammering home that perhaps all the remaining
> mlock cases are 'hard'.
A nice sideffect of me investigating these is that I am getting some
good insight into how iio works. I will see if I can investigate a
couple more cases
> Thanks,
> 
> Jonathan
> 
> >  
Thanks,
Rohit
> >  		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 */
>