lkml.org 
[lkml]   [2020]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] iio: health: max30100: remove mlock usage
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 */
>

\
 
 \ /
  Last update: 2020-03-15 11:57    [W:0.098 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site