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

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

[Rohit Sarkar]

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>
---
 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);
 
 		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 */
-- 
2.23.0.385.gbc12974a89

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

[Jonathan Cameron]

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.

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

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 */

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

[Rohit Sarkar]

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 */
> 

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

[Jonathan Cameron]

On Sun, 15 Mar 2020 16:27:24 +0530
Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:

> 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?

Yes, any time after that check the buffer could be disabled and we'd potentially
get a garbage result.

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

I'd wait for Matt to come back with whether he'd like to enable the
more complex solution to allow the buffer to be enabled on demand.

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

Cool. 

Jonathan

> > 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 */  
> >   

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

[Matt Ranostay]

On Sun, Mar 15, 2020 at 2:46 AM Jonathan Cameron <jic23@kernel.org> 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.
>
> 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

>
> 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 */
>

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

[Matt Ranostay]

On Mon, Mar 16, 2020 at 1:21 AM Matt Ranostay
<matt.ranostay@konsulko.com> wrote:
>
> On Sun, Mar 15, 2020 at 2:46 AM Jonathan Cameron <jic23@kernel.org> 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.
> >
> > 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.

- 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 */
> >

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

[Jonathan Cameron]

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

> On Mon, Mar 16, 2020 at 1:21 AM Matt Ranostay
> <matt.ranostay@konsulko.com> wrote:
> >
> > On Sun, Mar 15, 2020 at 2:46 AM Jonathan Cameron <jic23@kernel.org> 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.
> > >
> > > 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 */  
> > >