diff mbox series

[14/15] iio: health: max30102: do not use internal iio_dev lock

Message ID 20220920112821.975359-15-nuno.sa@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Make 'mlock' really private | expand

Commit Message

Nuno Sa Sept. 20, 2022, 11:28 a.m. UTC
The pattern used in this device does not quite fit in the
iio_device_claim_direct_mode() typical usage. In this case,
iio_buffer_enabled() was being used not to prevent the raw access
but to decide whether or not the device needs to be powered on before.
If buffering, then the device is already on. To guarantee the same
behavior, a combination of locks is being used:

1. Use iio_device_claim_direct_mode() to check if direct mode can be
claimed and if we can, then we keep it until the reading is done (which
also means the device will be powered on and off);
2. If already buffering, we need to make sure that buffering is not
disabled (and hence, powering off the device) while doing a raw read. For
that, we can make use of the local lock that already exists.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/health/max30102.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Jonathan Cameron Sept. 24, 2022, 3:54 p.m. UTC | #1
On Tue, 20 Sep 2022 13:28:20 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The pattern used in this device does not quite fit in the
> iio_device_claim_direct_mode() typical usage. In this case,
> iio_buffer_enabled() was being used not to prevent the raw access
> but to decide whether or not the device needs to be powered on before.
> If buffering, then the device is already on. To guarantee the same
> behavior, a combination of locks is being used:
> 
> 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> claimed and if we can, then we keep it until the reading is done (which
> also means the device will be powered on and off);
> 2. If already buffering, we need to make sure that buffering is not
> disabled (and hence, powering off the device) while doing a raw read. For
> that, we can make use of the local lock that already exists.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Obviously same dance in here as the related previous patch. So same solution
needs adopting.  I just thought I'd reply to make sure we didn't forget to
cover them both :)

Jonathan


> ---
>  drivers/iio/health/max30102.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index abbcef563807..e984c78b99f6 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -227,9 +227,20 @@ static int max30102_buffer_postenable(struct iio_dev *indio_dev)
>  static int max30102_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct max30102_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	/*
> +	 * As stated in the comment in the read_raw() function, temperature
> +	 * can only be acquired if the engine is running. As such the mutex
> +	 * is used to make sure we do not power down while doing a temperature
> +	 * reading.
> +	 */
> +	mutex_lock(&data->lock);
> +	ret = max30102_set_powermode(data, MAX30102_REG_MODE_CONFIG_MODE_NONE,
> +				     false);
> +	mutex_unlock(&data->lock);
>  
> -	return max30102_set_powermode(data, MAX30102_REG_MODE_CONFIG_MODE_NONE,
> -				      false);
> +	return ret;
>  }
>  
>  static const struct iio_buffer_setup_ops max30102_buffer_setup_ops = {
> @@ -477,12 +488,14 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>  		 * Temperature reading can only be acquired when not in
>  		 * shutdown; leave shutdown briefly when buffer not running
>  		 */
> -		mutex_lock(&indio_dev->mlock);
> -		if (!iio_buffer_enabled(indio_dev))
> +		if (!iio_device_claim_direct_mode(indio_dev)) {
>  			ret = max30102_get_temp(data, val, true);
> -		else
> +			iio_device_release_direct_mode(indio_dev);
> +		} else {
> +			mutex_lock(&data->lock);
>  			ret = max30102_get_temp(data, val, false);
> -		mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&data->lock);
> +		}
>  		if (ret)
>  			return ret;
>
Nuno Sá Sept. 30, 2022, 10:04 a.m. UTC | #2
On Sat, 2022-09-24 at 16:54 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 13:28:20 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The pattern used in this device does not quite fit in the
> > iio_device_claim_direct_mode() typical usage. In this case,
> > iio_buffer_enabled() was being used not to prevent the raw access
> > but to decide whether or not the device needs to be powered on
> > before.
> > If buffering, then the device is already on. To guarantee the same
> > behavior, a combination of locks is being used:
> > 
> > 1. Use iio_device_claim_direct_mode() to check if direct mode can
> > be
> > claimed and if we can, then we keep it until the reading is done
> > (which
> > also means the device will be powered on and off);
> > 2. If already buffering, we need to make sure that buffering is not
> > disabled (and hence, powering off the device) while doing a raw
> > read. For
> > that, we can make use of the local lock that already exists.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Obviously same dance in here as the related previous patch. So same
> solution
> needs adopting.  I just thought I'd reply to make sure we didn't
> forget to
> cover them both :)
> 
> 
Hi Jonathan,

So I was working on v2 in the morning and went with your
iio_device_claim_buffer_mode() approach... And bah, well it works like
a charm in the previous patch, it fails in this one:

-               mutex_lock(&indio_dev->mlock);
-               if (!iio_buffer_enabled(indio_dev))
+               if (iio_device_claim_buffer_mode(indio_dev)) {
                        ret = max30102_get_temp(data, val, true);
-               else
+               } else {
                        ret = max30102_get_temp(data, val, false);
-               mutex_unlock(&indio_dev->mlock);
-               if (ret)
+                       iio_device_release_buffer_mode(indio_dev);
+               }
+               if(ret)
                        return ret;


Note that if we are not in buffered mode we won't get mlock and call
max30102_get_temp(data, val, true) without any lock. While it's very
unlikely for someone, in the meantime, to enable the buffer and then
disable it, it's still racy and possible (at least in theory).

So, I'm thinking again on the flag approach... Just check my comment
(in the previous patch) about it being refcounted. I mean, I might be
missing something, but if we need a refcount, I would say things would
be already (potentially) broken, right?

With this step back, I'm planning on a v2 in the beginning of the week
after we have this sorted out (and I guess we need to settle things
also in the ad799x patch)

- Nuno Sá
Jonathan Cameron Oct. 2, 2022, 11:08 a.m. UTC | #3
On Fri, 30 Sep 2022 12:04:39 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2022-09-24 at 16:54 +0100, Jonathan Cameron wrote:
> > On Tue, 20 Sep 2022 13:28:20 +0200
> > Nuno Sá <nuno.sa@analog.com> wrote:
> >   
> > > The pattern used in this device does not quite fit in the
> > > iio_device_claim_direct_mode() typical usage. In this case,
> > > iio_buffer_enabled() was being used not to prevent the raw access
> > > but to decide whether or not the device needs to be powered on
> > > before.
> > > If buffering, then the device is already on. To guarantee the same
> > > behavior, a combination of locks is being used:
> > > 
> > > 1. Use iio_device_claim_direct_mode() to check if direct mode can
> > > be
> > > claimed and if we can, then we keep it until the reading is done
> > > (which
> > > also means the device will be powered on and off);
> > > 2. If already buffering, we need to make sure that buffering is not
> > > disabled (and hence, powering off the device) while doing a raw
> > > read. For
> > > that, we can make use of the local lock that already exists.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > Obviously same dance in here as the related previous patch. So same
> > solution
> > needs adopting.  I just thought I'd reply to make sure we didn't
> > forget to
> > cover them both :)
> > 
> >   
> Hi Jonathan,
> 
> So I was working on v2 in the morning and went with your
> iio_device_claim_buffer_mode() approach... And bah, well it works like
> a charm in the previous patch, it fails in this one:
> 
> -               mutex_lock(&indio_dev->mlock);
> -               if (!iio_buffer_enabled(indio_dev))
> +               if (iio_device_claim_buffer_mode(indio_dev)) {
>                         ret = max30102_get_temp(data, val, true);
> -               else
> +               } else {
>                         ret = max30102_get_temp(data, val, false);
> -               mutex_unlock(&indio_dev->mlock);
> -               if (ret)
> +                       iio_device_release_buffer_mode(indio_dev);
> +               }
> +               if(ret)
>                         return ret;
> 
> 
> Note that if we are not in buffered mode we won't get mlock and call
> max30102_get_temp(data, val, true) without any lock. While it's very
> unlikely for someone, in the meantime, to enable the buffer and then
> disable it, it's still racy and possible (at least in theory).

Ah. That's indeed tedious. I'd close the race by claiming direct mode
for the else branch.  If that fails, pah, just fail the call with a suitable
error return (-EAGAIN probably). 
Or put a retry look around the whole thing to make it even less likely
we'll hit the gap in the locking.

Otherwise, we could do iio_device_claim_current_mode() that locks on one
or the other but that just seems weird.

> 
> So, I'm thinking again on the flag approach... Just check my comment
> (in the previous patch) about it being refcounted. I mean, I might be
> missing something, but if we need a refcount, I would say things would
> be already (potentially) broken, right?

I'm not 100% sure on the refcount necessity, as I've not looked at
the code again, but these things tend to be symmetric as described in the
reply to the previous.   So you need to cover the case that this call
races with the buffer being disabled.

> 
> With this step back, I'm planning on a v2 in the beginning of the week
> after we have this sorted out (and I guess we need to settle things
> also in the ad799x patch)
> 
> - Nuno Sá
>
Nuno Sá Oct. 4, 2022, 7:53 a.m. UTC | #4
On Sun, 2022-10-02 at 12:08 +0100, Jonathan Cameron wrote:
> On Fri, 30 Sep 2022 12:04:39 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sat, 2022-09-24 at 16:54 +0100, Jonathan Cameron wrote:
> > > On Tue, 20 Sep 2022 13:28:20 +0200
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >   
> > > > The pattern used in this device does not quite fit in the
> > > > iio_device_claim_direct_mode() typical usage. In this case,
> > > > iio_buffer_enabled() was being used not to prevent the raw
> > > > access
> > > > but to decide whether or not the device needs to be powered on
> > > > before.
> > > > If buffering, then the device is already on. To guarantee the
> > > > same
> > > > behavior, a combination of locks is being used:
> > > > 
> > > > 1. Use iio_device_claim_direct_mode() to check if direct mode
> > > > can
> > > > be
> > > > claimed and if we can, then we keep it until the reading is
> > > > done
> > > > (which
> > > > also means the device will be powered on and off);
> > > > 2. If already buffering, we need to make sure that buffering is
> > > > not
> > > > disabled (and hence, powering off the device) while doing a raw
> > > > read. For
> > > > that, we can make use of the local lock that already exists.
> > > > 
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > > Obviously same dance in here as the related previous patch. So
> > > same
> > > solution
> > > needs adopting.  I just thought I'd reply to make sure we didn't
> > > forget to
> > > cover them both :)
> > > 
> > >   
> > Hi Jonathan,
> > 
> > So I was working on v2 in the morning and went with your
> > iio_device_claim_buffer_mode() approach... And bah, well it works
> > like
> > a charm in the previous patch, it fails in this one:
> > 
> > -               mutex_lock(&indio_dev->mlock);
> > -               if (!iio_buffer_enabled(indio_dev))
> > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> >                         ret = max30102_get_temp(data, val, true);
> > -               else
> > +               } else {
> >                         ret = max30102_get_temp(data, val, false);
> > -               mutex_unlock(&indio_dev->mlock);
> > -               if (ret)
> > +                       iio_device_release_buffer_mode(indio_dev);
> > +               }
> > +               if(ret)
> >                         return ret;
> > 
> > 
> > Note that if we are not in buffered mode we won't get mlock and
> > call
> > max30102_get_temp(data, val, true) without any lock. While it's
> > very
> > unlikely for someone, in the meantime, to enable the buffer and
> > then
> > disable it, it's still racy and possible (at least in theory).
> 
> Ah. That's indeed tedious. I'd close the race by claiming direct mode
> for the else branch.  If that fails, pah, just fail the call with a
> suitable
> error return (-EAGAIN probably). 
> Or put a retry look around the whole thing to make it even less
> likely
> we'll hit the gap in the locking.
> 

Hmm I did thought about that but it looked very "dirty"... Anyways, I
can do it for v2 just so we have a look on how it looks like.

- Nuno Sá
diff mbox series

Patch

diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index abbcef563807..e984c78b99f6 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -227,9 +227,20 @@  static int max30102_buffer_postenable(struct iio_dev *indio_dev)
 static int max30102_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct max30102_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * As stated in the comment in the read_raw() function, temperature
+	 * can only be acquired if the engine is running. As such the mutex
+	 * is used to make sure we do not power down while doing a temperature
+	 * reading.
+	 */
+	mutex_lock(&data->lock);
+	ret = max30102_set_powermode(data, MAX30102_REG_MODE_CONFIG_MODE_NONE,
+				     false);
+	mutex_unlock(&data->lock);
 
-	return max30102_set_powermode(data, MAX30102_REG_MODE_CONFIG_MODE_NONE,
-				      false);
+	return ret;
 }
 
 static const struct iio_buffer_setup_ops max30102_buffer_setup_ops = {
@@ -477,12 +488,14 @@  static int max30102_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired when not in
 		 * shutdown; leave shutdown briefly when buffer not running
 		 */
-		mutex_lock(&indio_dev->mlock);
-		if (!iio_buffer_enabled(indio_dev))
+		if (!iio_device_claim_direct_mode(indio_dev)) {
 			ret = max30102_get_temp(data, val, true);
-		else
+			iio_device_release_direct_mode(indio_dev);
+		} else {
+			mutex_lock(&data->lock);
 			ret = max30102_get_temp(data, val, false);
-		mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&data->lock);
+		}
 		if (ret)
 			return ret;