[13/15] iio: health: max30100: do not use internal iio_dev lock

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
allow it. Hence to get rid of the 'mlock' we need to:

1. Use iio_device_claim_direct_mode() to check if direct mode can be
claimed and if we can return -EINVAL (as the original code);
2. Make sure that buffering is not disabled while doing a raw read. For
that, we can make use of the local lock that already exists.

While at it, fixed a minor coding style complain...

Signed-off-by: Nuno Sá <[e-mail]>
---
 drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

Comments

Miquel Raynal Sept. 20, 2022, 12:23 p.m. UTC | #1

Hi Nuno,

[e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:

> 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
> allow it. Hence to get rid of the 'mlock' we need to:
> 
> 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> claimed and if we can return -EINVAL (as the original code);
>
> 2. Make sure that buffering is not disabled while doing a raw read. For
> that, we can make use of the local lock that already exists.
> 
> While at it, fixed a minor coding style complain...
> 
> Signed-off-by: Nuno Sá <[e-mail]>
> ---
>  drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index ad5717965223..aa494cad5df0 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -185,8 +185,19 @@ static int max30100_buffer_postenable(struct iio_dev *indio_dev)
>  static int max30100_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct max30100_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 = max30100_set_powermode(data, false);
> +	mutex_unlock(&data->lock);
>  
> -	return max30100_set_powermode(data, false);
> +	return ret;
>  }
>  
>  static const struct iio_buffer_setup_ops max30100_buffer_setup_ops = {
> @@ -387,18 +398,17 @@ 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);
> -
> -		if (!iio_buffer_enabled(indio_dev))
> +		if (!iio_device_claim_direct_mode(indio_dev)) {

I wonder if this line change here is really needed. I agree the whole
construction looks like what iio_device_claim_direct_mode() does but in
practice I don't see the point of acquiring any lock here if we just
release it no matter what happens right after.

Unless of course if there is a hidden goal like "stop exporting
iio_buffer_enabled()" or something like that.

At least I would separate this from the main change which targets the
removal of mlock because I don't see how it is directly related.

>  			ret = -EAGAIN;
> -		else {
> +			iio_device_release_direct_mode(indio_dev);
> +		} else {
> +			mutex_lock(&data->lock);
>  			ret = max30100_get_temp(data, val);
>  			if (!ret)
>  				ret = IIO_VAL_INT;
> -
> +			mutex_unlock(&data->lock);
>  		}
>  
> -		mutex_unlock(&indio_dev->mlock);
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 1;  /* 0.0625 */

In any case, nice series, thanks for writing it!

Thanks,
Miquèl

Nuno Sa Sept. 20, 2022, 12:44 p.m. UTC | #2

> -----Original Message-----
> From: Miquel Raynal <[e-mail]>
> Sent: Tuesday, September 20, 2022 2:23 PM
> To: Sa, Nuno <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Chunyan Zhang <[e-mail]>; Hennerich,
> Michael <[e-mail]>; Martin Blumenstingl
> <[e-mail]>; Sascha Hauer
> <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; Alexandru Ardelean
> <[e-mail]>; Fabio Estevam <[e-mail]>; Andriy
> Tryshnivskyy <[e-mail]>; Haibo Chen
> <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> Goede <[e-mail]>; Jerome Brunet <[e-mail]>;
> Heiko Stuebner <[e-mail]>; Florian Boor
> <[e-mail]>; Regus, Ciprian
> <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Andy
> Shevchenko <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> Wang <[e-mail]>; Jyoti Bhayana
> <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> <[e-mail]>
> Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> lock
> 
> [External]
> 
> Hi Nuno,
> 

Hi Miquel,

Thanks for reviewing...

> [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> 
> > 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
> > allow it. Hence to get rid of the 'mlock' we need to:
> >
> > 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> > claimed and if we can return -EINVAL (as the original code);
> >
> > 2. Make sure that buffering is not disabled while doing a raw read. For
> > that, we can make use of the local lock that already exists.
> >
> > While at it, fixed a minor coding style complain...
> >
> > Signed-off-by: Nuno Sá <[e-mail]>
> > ---
> >  drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> > index ad5717965223..aa494cad5df0 100644
> > --- a/drivers/iio/health/max30100.c
> > +++ b/drivers/iio/health/max30100.c
> > @@ -185,8 +185,19 @@ static int max30100_buffer_postenable(struct
> iio_dev *indio_dev)
> >  static int max30100_buffer_predisable(struct iio_dev *indio_dev)
> >  {
> >  	struct max30100_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 = max30100_set_powermode(data, false);
> > +	mutex_unlock(&data->lock);
> >
> > -	return max30100_set_powermode(data, false);
> > +	return ret;
> >  }
> >
> >  static const struct iio_buffer_setup_ops max30100_buffer_setup_ops = {
> > @@ -387,18 +398,17 @@ 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);
> > -
> > -		if (!iio_buffer_enabled(indio_dev))
> > +		if (!iio_device_claim_direct_mode(indio_dev)) {
> 
> I wonder if this line change here is really needed. I agree the whole
> construction looks like what iio_device_claim_direct_mode() does but in
> practice I don't see the point of acquiring any lock here if we just
> release it no matter what happens right after.
> 

I can see that this is odd (at the very least) but AFAIK, this is the only way
to safely infer if buffering is enabled or not. iio_buffer_enabled() has no
protection against someone concurrently enabling/disabling the buffer.
So the call is needed to make sure 'mlock' is internally grabbed before
calling iio_buffer_enabled().

> Unless of course if there is a hidden goal like "stop exporting
> iio_buffer_enabled()" or something like that.
> 
> At least I would separate this from the main change which targets the
> removal of mlock because I don't see how it is directly related.

In a sense both changes are needed to ultimately get rid of mlock. I'm 
also not sure how could I do the separation... Do you have something
in mind?

- Nuno Sá

Miquel Raynal Sept. 20, 2022, 12:55 p.m. UTC | #3

Hi Nuno,

[e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:

> > -----Original Message-----
> > From: Miquel Raynal <[e-mail]>
> > Sent: Tuesday, September 20, 2022 2:23 PM
> > To: Sa, Nuno <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; Chunyan Zhang <[e-mail]>; Hennerich,
> > Michael <[e-mail]>; Martin Blumenstingl
> > <[e-mail]>; Sascha Hauer
> > <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; Alexandru Ardelean
> > <[e-mail]>; Fabio Estevam <[e-mail]>; Andriy
> > Tryshnivskyy <[e-mail]>; Haibo Chen
> > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > Goede <[e-mail]>; Jerome Brunet <[e-mail]>;
> > Heiko Stuebner <[e-mail]>; Florian Boor
> > <[e-mail]>; Regus, Ciprian
> > <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Andy
> > Shevchenko <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> > Wang <[e-mail]>; Jyoti Bhayana
> > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > <[e-mail]>
> > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> > lock
> > 
> > [External]
> > 
> > Hi Nuno,
> >   
> 
> Hi Miquel,
> 
> Thanks for reviewing...
> 
> > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> >   
> > > 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
> > > allow it. Hence to get rid of the 'mlock' we need to:
> > >
> > > 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> > > claimed and if we can return -EINVAL (as the original code);
> > >
> > > 2. Make sure that buffering is not disabled while doing a raw read. For
> > > that, we can make use of the local lock that already exists.
> > >
> > > While at it, fixed a minor coding style complain...
> > >
> > > Signed-off-by: Nuno Sá <[e-mail]>
> > > ---
> > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> > > index ad5717965223..aa494cad5df0 100644
> > > --- a/drivers/iio/health/max30100.c
> > > +++ b/drivers/iio/health/max30100.c
> > > @@ -185,8 +185,19 @@ static int max30100_buffer_postenable(struct  
> > iio_dev *indio_dev)  
> > >  static int max30100_buffer_predisable(struct iio_dev *indio_dev)
> > >  {
> > >  	struct max30100_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 = max30100_set_powermode(data, false);
> > > +	mutex_unlock(&data->lock);
> > >
> > > -	return max30100_set_powermode(data, false);
> > > +	return ret;
> > >  }
> > >
> > >  static const struct iio_buffer_setup_ops max30100_buffer_setup_ops = {
> > > @@ -387,18 +398,17 @@ 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);
> > > -
> > > -		if (!iio_buffer_enabled(indio_dev))
> > > +		if (!iio_device_claim_direct_mode(indio_dev)) {  
> > 
> > I wonder if this line change here is really needed. I agree the whole
> > construction looks like what iio_device_claim_direct_mode() does but in
> > practice I don't see the point of acquiring any lock here if we just
> > release it no matter what happens right after.
> >   
> 
> I can see that this is odd (at the very least) but AFAIK, this is the only way
> to safely infer if buffering is enabled or not. iio_buffer_enabled() has no
> protection against someone concurrently enabling/disabling the buffer.

Yes, but this is only relevant if you want to infer that the "buffers
are enabled" and be sure that it cannot be otherwise during the next
lines until you release the lock. Acquiring a lock, doing the if and
then unconditionally releasing the lock, IMHO, does not make any sense
(but I'm not a locking guru) because when you enter the else clause,
you are not protected anyway, so in both cases all this is completely
racy.

> So the call is needed to make sure 'mlock' is internally grabbed before
> calling iio_buffer_enabled().
> 
> > Unless of course if there is a hidden goal like "stop exporting
> > iio_buffer_enabled()" or something like that.
> > 
> > At least I would separate this from the main change which targets the
> > removal of mlock because I don't see how it is directly related.  
> 
> In a sense both changes are needed to ultimately get rid of mlock. I'm 
> also not sure how could I do the separation... Do you have something
> in mind?
> 
> - Nuno Sá


Thanks,
Miquèl

Nuno Sa Sept. 20, 2022, 1:15 p.m. UTC | #4

> -----Original Message-----
> From: Miquel Raynal <[e-mail]>
> Sent: Tuesday, September 20, 2022 2:56 PM
> To: Sa, Nuno <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Chunyan Zhang <[e-mail]>; Hennerich,
> Michael <[e-mail]>; Martin Blumenstingl
> <[e-mail]>; Sascha Hauer
> <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; Alexandru Ardelean
> <[e-mail]>; Fabio Estevam <[e-mail]>; Andriy
> Tryshnivskyy <[e-mail]>; Haibo Chen
> <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> Goede <[e-mail]>; Jerome Brunet <[e-mail]>;
> Heiko Stuebner <[e-mail]>; Florian Boor
> <[e-mail]>; Regus, Ciprian
> <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Andy
> Shevchenko <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> Wang <[e-mail]>; Jyoti Bhayana
> <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> <[e-mail]>
> Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> lock
> 
> [External]
> 
> Hi Nuno,
> 
> [e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> 
> > > -----Original Message-----
> > > From: Miquel Raynal <[e-mail]>
> > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > To: Sa, Nuno <[e-mail]>
> > > Cc: [e-mail]; linux-
> [e-mail];
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]; Chunyan Zhang <[e-mail]>;
> Hennerich,
> > > Michael <[e-mail]>; Martin Blumenstingl
> > > <[e-mail]>; Sascha Hauer
> > > <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> > > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> Ardelean
> > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> Andriy
> > > Tryshnivskyy <[e-mail]>; Haibo Chen
> > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > > Goede <[e-mail]>; Jerome Brunet
> <[e-mail]>;
> > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > <[e-mail]>; Regus, Ciprian
> > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> Andy
> > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> > > Wang <[e-mail]>; Jyoti Bhayana
> > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > > <[e-mail]>
> > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal
> iio_dev
> > > lock
> > >
> > > [External]
> > >
> > > Hi Nuno,
> > >
> >
> > Hi Miquel,
> >
> > Thanks for reviewing...
> >
> > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > >
> > > > 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
> > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > >
> > > > 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> > > > claimed and if we can return -EINVAL (as the original code);
> > > >
> > > > 2. Make sure that buffering is not disabled while doing a raw read. For
> > > > that, we can make use of the local lock that already exists.
> > > >
> > > > While at it, fixed a minor coding style complain...
> > > >
> > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > ---
> > > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
> > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/health/max30100.c
> b/drivers/iio/health/max30100.c
> > > > index ad5717965223..aa494cad5df0 100644
> > > > --- a/drivers/iio/health/max30100.c
> > > > +++ b/drivers/iio/health/max30100.c
> > > > @@ -185,8 +185,19 @@ static int max30100_buffer_postenable(struct
> > > iio_dev *indio_dev)
> > > >  static int max30100_buffer_predisable(struct iio_dev *indio_dev)
> > > >  {
> > > >  	struct max30100_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 = max30100_set_powermode(data, false);
> > > > +	mutex_unlock(&data->lock);
> > > >
> > > > -	return max30100_set_powermode(data, false);
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static const struct iio_buffer_setup_ops max30100_buffer_setup_ops
> = {
> > > > @@ -387,18 +398,17 @@ 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);
> > > > -
> > > > -		if (!iio_buffer_enabled(indio_dev))
> > > > +		if (!iio_device_claim_direct_mode(indio_dev)) {
> > >
> > > I wonder if this line change here is really needed. I agree the whole
> > > construction looks like what iio_device_claim_direct_mode() does but in
> > > practice I don't see the point of acquiring any lock here if we just
> > > release it no matter what happens right after.
> > >
> >
> > I can see that this is odd (at the very least) but AFAIK, this is the only way
> > to safely infer if buffering is enabled or not. iio_buffer_enabled() has no
> > protection against someone concurrently enabling/disabling the buffer.
> 
> Yes, but this is only relevant if you want to infer that the "buffers
> are enabled" and be sure that it cannot be otherwise during the next
> lines until you release the lock. Acquiring a lock, doing the if and
> then unconditionally releasing the lock, IMHO, does not make any sense
> (but I'm not a locking guru) because when you enter the else clause,
> you are not protected anyway, so in both cases all this is completely
> racy.
> 

Ahh crap, yes you are right... It is still racy since we can still try to read
the temperature with the device powered off. I'm not really sure how to
address this. One way could be to just use an internal control variable
to reflect the device power state (set/clear on the buffer callbacks) and
only use the local lock (completely ditching the call to
iio_device_claim_direct_mode())...

Other options would be to have helpers for acquiring/releasing the lock
(I think this would defeat the idea of not abusing this lock at all) or have
A version  iio_device_claim_direct_mode() that does not release the 
lock in case buffering is enabled. Any preference?

- Nuno Sá

Miquel Raynal Sept. 20, 2022, 1:53 p.m. UTC | #5

Hi Nuno,

[e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:

> > -----Original Message-----
> > From: Miquel Raynal <[e-mail]>
> > Sent: Tuesday, September 20, 2022 2:56 PM
> > To: Sa, Nuno <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; Chunyan Zhang <[e-mail]>; Hennerich,
> > Michael <[e-mail]>; Martin Blumenstingl
> > <[e-mail]>; Sascha Hauer
> > <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; Alexandru Ardelean
> > <[e-mail]>; Fabio Estevam <[e-mail]>; Andriy
> > Tryshnivskyy <[e-mail]>; Haibo Chen
> > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > Goede <[e-mail]>; Jerome Brunet <[e-mail]>;
> > Heiko Stuebner <[e-mail]>; Florian Boor
> > <[e-mail]>; Regus, Ciprian
> > <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Andy
> > Shevchenko <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> > Wang <[e-mail]>; Jyoti Bhayana
> > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > <[e-mail]>
> > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> > lock
> > 
> > [External]
> > 
> > Hi Nuno,
> > 
> > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> >   
> > > > -----Original Message-----
> > > > From: Miquel Raynal <[e-mail]>
> > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > To: Sa, Nuno <[e-mail]>
> > > > Cc: [e-mail]; linux-  
> > [e-mail];  
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]; Chunyan Zhang <[e-mail]>;  
> > Hennerich,  
> > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > <[e-mail]>; Sascha Hauer
> > > > <[e-mail]>; Cixi Geng <[e-mail]>; Kevin
> > > > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > > > Pengutronix Kernel Team <[e-mail]>; Alexandru  
> > Ardelean  
> > > > <[e-mail]>; Fabio Estevam <[e-mail]>;  
> > Andriy  
> > > > Tryshnivskyy <[e-mail]>; Haibo Chen
> > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > > > Goede <[e-mail]>; Jerome Brunet  
> > <[e-mail]>;  
> > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > <[e-mail]>; Regus, Ciprian
> > > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;  
> > Andy  
> > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > <[e-mail]>; Neil Armstrong <[e-mail]>; Baolin
> > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > > > <[e-mail]>
> > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal  
> > iio_dev  
> > > > lock
> > > >
> > > > [External]
> > > >
> > > > Hi Nuno,
> > > >  
> > >
> > > Hi Miquel,
> > >
> > > Thanks for reviewing...
> > >  
> > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > > >  
> > > > > 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  
> > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > >
> > > > > 1. Use iio_device_claim_direct_mode() to check if direct mode can be
> > > > > claimed and if we can return -EINVAL (as the original code);
> > > > >
> > > > > 2. Make sure that buffering is not disabled while doing a raw read. For
> > > > > that, we can make use of the local lock that already exists.
> > > > >
> > > > > While at it, fixed a minor coding style complain...
> > > > >
> > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > ---
> > > > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++-------
> > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/health/max30100.c  
> > b/drivers/iio/health/max30100.c  
> > > > > index ad5717965223..aa494cad5df0 100644
> > > > > --- a/drivers/iio/health/max30100.c
> > > > > +++ b/drivers/iio/health/max30100.c
> > > > > @@ -185,8 +185,19 @@ static int max30100_buffer_postenable(struct  
> > > > iio_dev *indio_dev)  
> > > > >  static int max30100_buffer_predisable(struct iio_dev *indio_dev)
> > > > >  {
> > > > >  	struct max30100_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 = max30100_set_powermode(data, false);
> > > > > +	mutex_unlock(&data->lock);
> > > > >
> > > > > -	return max30100_set_powermode(data, false);
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static const struct iio_buffer_setup_ops max30100_buffer_setup_ops  
> > = {  
> > > > > @@ -387,18 +398,17 @@ 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);
> > > > > -
> > > > > -		if (!iio_buffer_enabled(indio_dev))
> > > > > +		if (!iio_device_claim_direct_mode(indio_dev)) {  
> > > >
> > > > I wonder if this line change here is really needed. I agree the whole
> > > > construction looks like what iio_device_claim_direct_mode() does but in
> > > > practice I don't see the point of acquiring any lock here if we just
> > > > release it no matter what happens right after.
> > > >  
> > >
> > > I can see that this is odd (at the very least) but AFAIK, this is the only way
> > > to safely infer if buffering is enabled or not. iio_buffer_enabled() has no
> > > protection against someone concurrently enabling/disabling the buffer.  
> > 
> > Yes, but this is only relevant if you want to infer that the "buffers
> > are enabled" and be sure that it cannot be otherwise during the next
> > lines until you release the lock. Acquiring a lock, doing the if and
> > then unconditionally releasing the lock, IMHO, does not make any sense
> > (but I'm not a locking guru) because when you enter the else clause,
> > you are not protected anyway, so in both cases all this is completely
> > racy.
> >   
> 
> Ahh crap, yes you are right... It is still racy since we can still try to read
> the temperature with the device powered off. I'm not really sure how to
> address this. One way could be to just use an internal control variable
> to reflect the device power state (set/clear on the buffer callbacks) and
> only use the local lock (completely ditching the call to
> iio_device_claim_direct_mode())...

I tend to prefer this option than the one below.

I guess your implementation already prevents buffer_predisable() to run
thanks to the local lock being held during the operation. Maybe we
should just verify that buffers are enabled from within the local lock
being held instead of just acquiring it for the get_temp() measure. It
would probably solve the situation here.

> Other options would be to have helpers for acquiring/releasing the lock
> (I think this would defeat the idea of not abusing this lock at all) or have
> A version  iio_device_claim_direct_mode() that does not release the 
> lock in case buffering is enabled. Any preference?
> 
> - Nuno Sá


Thanks,
Miquèl

Nuno Sá Sept. 20, 2022, 2:56 p.m. UTC | #6

On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:
> Hi Nuno,
> 
> [e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> 
> > > -----Original Message-----
> > > From: Miquel Raynal <[e-mail]>
> > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > To: Sa, Nuno <[e-mail]>
> > > Cc: [e-mail];
> > > [e-mail];
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]; Chunyan Zhang <[e-mail]>;
> > > Hennerich,
> > > Michael <[e-mail]>; Martin Blumenstingl
> > > <[e-mail]>; Sascha Hauer
> > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > Kevin
> > > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> > > Ardelean
> > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> > > Andriy
> > > Tryshnivskyy <[e-mail]>; Haibo Chen
> > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > > Goede <[e-mail]>; Jerome Brunet
> > > <[e-mail]>;
> > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > <[e-mail]>; Regus, Ciprian
> > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> > > Andy
> > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > Baolin
> > > Wang <[e-mail]>; Jyoti Bhayana
> > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > > <[e-mail]>
> > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > internal iio_dev
> > > lock
> > > 
> > > [External]
> > > 
> > > Hi Nuno,
> > > 
> > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > >   
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <[e-mail]>
> > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > To: Sa, Nuno <[e-mail]>
> > > > > Cc: [e-mail]; linux-  
> > > [e-mail];  
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]; Chunyan Zhang <[e-mail]>;  
> > > Hennerich,  
> > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > <[e-mail]>; Sascha Hauer
> > > > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > > > Kevin
> > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > <[e-mail]>;
> > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru  
> > > Ardelean  
> > > > > <[e-mail]>; Fabio Estevam <[e-mail]>; 
> > > Andriy  
> > > > > Tryshnivskyy <[e-mail]>; Haibo
> > > > > Chen
> > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans
> > > > > de
> > > > > Goede <[e-mail]>; Jerome Brunet  
> > > <[e-mail]>;  
> > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > <[e-mail]>; Regus, Ciprian
> > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > <[e-mail]>;  
> > > Andy  
> > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > > > Baolin
> > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson
> > > > > Zhai
> > > > > <[e-mail]>
> > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > internal  
> > > iio_dev  
> > > > > lock
> > > > > 
> > > > > [External]
> > > > > 
> > > > > Hi Nuno,
> > > > >  
> > > > 
> > > > Hi Miquel,
> > > > 
> > > > Thanks for reviewing...
> > > >  
> > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > > > >  
> > > > > > 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  
> > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > 
> > > > > > 1. Use iio_device_claim_direct_mode() to check if direct
> > > > > > mode can be
> > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > code);
> > > > > > 
> > > > > > 2. Make sure that buffering is not disabled while doing a
> > > > > > raw read. For
> > > > > > that, we can make use of the local lock that already
> > > > > > exists.
> > > > > > 
> > > > > > While at it, fixed a minor coding style complain...
> > > > > > 
> > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > ---
> > > > > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++------
> > > > > > -
> > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/health/max30100.c  
> > > b/drivers/iio/health/max30100.c  
> > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > max30100_buffer_postenable(struct  
> > > > > iio_dev *indio_dev)  
> > > > > >  static int max30100_buffer_predisable(struct iio_dev
> > > > > > *indio_dev)
> > > > > >  {
> > > > > >         struct max30100_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 = max30100_set_powermode(data, false);
> > > > > > +       mutex_unlock(&data->lock);
> > > > > > 
> > > > > > -       return max30100_set_powermode(data, false);
> > > > > > +       return ret;
> > > > > >  }
> > > > > > 
> > > > > >  static const struct iio_buffer_setup_ops
> > > > > > max30100_buffer_setup_ops  
> > > = {  
> > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > -
> > > > > > -               if (!iio_buffer_enabled(indio_dev))
> > > > > > +               if
> > > > > > (!iio_device_claim_direct_mode(indio_dev)) {  
> > > > > 
> > > > > I wonder if this line change here is really needed. I agree
> > > > > the whole
> > > > > construction looks like what iio_device_claim_direct_mode()
> > > > > does but in
> > > > > practice I don't see the point of acquiring any lock here if
> > > > > we just
> > > > > release it no matter what happens right after.
> > > > >  
> > > > 
> > > > I can see that this is odd (at the very least) but AFAIK, this
> > > > is the only way
> > > > to safely infer if buffering is enabled or not.
> > > > iio_buffer_enabled() has no
> > > > protection against someone concurrently enabling/disabling the
> > > > buffer.  
> > > 
> > > Yes, but this is only relevant if you want to infer that the
> > > "buffers
> > > are enabled" and be sure that it cannot be otherwise during the
> > > next
> > > lines until you release the lock. Acquiring a lock, doing the if
> > > and
> > > then unconditionally releasing the lock, IMHO, does not make any
> > > sense
> > > (but I'm not a locking guru) because when you enter the else
> > > clause,
> > > you are not protected anyway, so in both cases all this is
> > > completely
> > > racy.
> > >   
> > 
> > Ahh crap, yes you are right... It is still racy since we can still
> > try to read
> > the temperature with the device powered off. I'm not really sure
> > how to
> > address this. One way could be to just use an internal control
> > variable
> > to reflect the device power state (set/clear on the buffer
> > callbacks) and
> > only use the local lock (completely ditching the call to
> > iio_device_claim_direct_mode())...
> 
> I tend to prefer this option than the one below.
> 
> I guess your implementation already prevents buffer_predisable() to
> run
> thanks to the local lock being held during the operation. Maybe we
> should just verify that buffers are enabled from within the local
> lock
> being held instead of just acquiring it for the get_temp() measure.
> It
> would probably solve the situation here.
> > 
Not sure if I understood... You mean something like:

mutex_lock(&data->lock);
if (!iio_buffer_enabled(indio_dev)) {
	ret = -EINVAL;
} else {
 	ret = max30100_get_temp(data, val);
 	if (!ret)
 		ret = IIO_VAL_INT;

}
mutex_unlock(&data->lock);

If so, I think this is still racy since we release the lock after the
predisable which means we could still detect the buffers as enabled (in
the above block) and try to get_temp on a powered down device.

Since we pretty much only care about the power state of the device (and
we are using the buffering state to infer that AFAIU), I was thinking
in something like:


mutex_lock(&data->lock);
if (!data->powered) {
	ret = -EINVAL;
} else {
 	ret = max30100_get_temp(data, val);
 	if (!ret)
 		ret = IIO_VAL_INT;

}
mutex_unlock(&data->lock);

Then, in the predisable, something like I have but setting the flag to
false and the opposite on the postenable... Naturally we could also
just read the registers (and I actually tend to prefer it) instead of a
new flag but I guess the flag is enough in this case.

- Nuno Sá
>

Miquel Raynal Sept. 20, 2022, 3:10 p.m. UTC | #7

Hi Nuno,

[e-mail] wrote on Tue, 20 Sep 2022 16:56:01 +0200:

> On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:
> > Hi Nuno,
> > 
> > [e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> >   
> > > > -----Original Message-----
> > > > From: Miquel Raynal <[e-mail]>
> > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > To: Sa, Nuno <[e-mail]>
> > > > Cc: [e-mail];
> > > > [e-mail];
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]; Chunyan Zhang <[e-mail]>;
> > > > Hennerich,
> > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > <[e-mail]>; Sascha Hauer
> > > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > > Kevin
> > > > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> > > > Ardelean
> > > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> > > > Andriy
> > > > Tryshnivskyy <[e-mail]>; Haibo Chen
> > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > > > Goede <[e-mail]>; Jerome Brunet
> > > > <[e-mail]>;
> > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > <[e-mail]>; Regus, Ciprian
> > > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> > > > Andy
> > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > > Baolin
> > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > > > <[e-mail]>
> > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > internal iio_dev
> > > > lock
> > > > 
> > > > [External]
> > > > 
> > > > Hi Nuno,
> > > > 
> > > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > Cc: [e-mail]; linux-    
> > > > [e-mail];    
> > > > > > [e-mail]; [e-mail]; linux-
> > > > > > [e-mail]; Chunyan Zhang <[e-mail]>;    
> > > > Hennerich,    
> > > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > > <[e-mail]>; Sascha Hauer
> > > > > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > > > > Kevin
> > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > <[e-mail]>;
> > > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru    
> > > > Ardelean    
> > > > > > <[e-mail]>; Fabio Estevam <[e-mail]>;   
> > > > Andriy    
> > > > > > Tryshnivskyy <[e-mail]>; Haibo
> > > > > > Chen
> > > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans
> > > > > > de
> > > > > > Goede <[e-mail]>; Jerome Brunet    
> > > > <[e-mail]>;    
> > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > <[e-mail]>;    
> > > > Andy    
> > > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > > > > Baolin
> > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson
> > > > > > Zhai
> > > > > > <[e-mail]>
> > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > > internal    
> > > > iio_dev    
> > > > > > lock
> > > > > > 
> > > > > > [External]
> > > > > > 
> > > > > > Hi Nuno,
> > > > > >    
> > > > > 
> > > > > Hi Miquel,
> > > > > 
> > > > > Thanks for reviewing...
> > > > >    
> > > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > > > > >    
> > > > > > > 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    
> > > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > > 
> > > > > > > 1. Use iio_device_claim_direct_mode() to check if direct
> > > > > > > mode can be
> > > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > > code);
> > > > > > > 
> > > > > > > 2. Make sure that buffering is not disabled while doing a
> > > > > > > raw read. For
> > > > > > > that, we can make use of the local lock that already
> > > > > > > exists.
> > > > > > > 
> > > > > > > While at it, fixed a minor coding style complain...
> > > > > > > 
> > > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++------
> > > > > > > -
> > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/health/max30100.c    
> > > > b/drivers/iio/health/max30100.c    
> > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > max30100_buffer_postenable(struct    
> > > > > > iio_dev *indio_dev)    
> > > > > > >  static int max30100_buffer_predisable(struct iio_dev
> > > > > > > *indio_dev)
> > > > > > >  {
> > > > > > >         struct max30100_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 = max30100_set_powermode(data, false);
> > > > > > > +       mutex_unlock(&data->lock);
> > > > > > > 
> > > > > > > -       return max30100_set_powermode(data, false);
> > > > > > > +       return ret;
> > > > > > >  }
> > > > > > > 
> > > > > > >  static const struct iio_buffer_setup_ops
> > > > > > > max30100_buffer_setup_ops    
> > > > = {    
> > > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > > -
> > > > > > > -               if (!iio_buffer_enabled(indio_dev))
> > > > > > > +               if
> > > > > > > (!iio_device_claim_direct_mode(indio_dev)) {    
> > > > > > 
> > > > > > I wonder if this line change here is really needed. I agree
> > > > > > the whole
> > > > > > construction looks like what iio_device_claim_direct_mode()
> > > > > > does but in
> > > > > > practice I don't see the point of acquiring any lock here if
> > > > > > we just
> > > > > > release it no matter what happens right after.
> > > > > >    
> > > > > 
> > > > > I can see that this is odd (at the very least) but AFAIK, this
> > > > > is the only way
> > > > > to safely infer if buffering is enabled or not.
> > > > > iio_buffer_enabled() has no
> > > > > protection against someone concurrently enabling/disabling the
> > > > > buffer.    
> > > > 
> > > > Yes, but this is only relevant if you want to infer that the
> > > > "buffers
> > > > are enabled" and be sure that it cannot be otherwise during the
> > > > next
> > > > lines until you release the lock. Acquiring a lock, doing the if
> > > > and
> > > > then unconditionally releasing the lock, IMHO, does not make any
> > > > sense
> > > > (but I'm not a locking guru) because when you enter the else
> > > > clause,
> > > > you are not protected anyway, so in both cases all this is
> > > > completely
> > > > racy.
> > > >     
> > > 
> > > Ahh crap, yes you are right... It is still racy since we can still
> > > try to read
> > > the temperature with the device powered off. I'm not really sure
> > > how to
> > > address this. One way could be to just use an internal control
> > > variable
> > > to reflect the device power state (set/clear on the buffer
> > > callbacks) and
> > > only use the local lock (completely ditching the call to
> > > iio_device_claim_direct_mode())...  
> > 
> > I tend to prefer this option than the one below.
> > 
> > I guess your implementation already prevents buffer_predisable() to
> > run
> > thanks to the local lock being held during the operation. Maybe we
> > should just verify that buffers are enabled from within the local
> > lock
> > being held instead of just acquiring it for the get_temp() measure.
> > It
> > would probably solve the situation here.  
> > >   
> Not sure if I understood... You mean something like:
> 
> mutex_lock(&data->lock);
> if (!iio_buffer_enabled(indio_dev)) {
> 	ret = -EINVAL;
> } else {
>  	ret = max30100_get_temp(data, val);
>  	if (!ret)
>  		ret = IIO_VAL_INT;
> 
> }
> mutex_unlock(&data->lock);
> 
> If so, I think this is still racy since we release the lock after the
> predisable which means we could still detect the buffers as enabled (in
> the above block) and try to get_temp on a powered down device.

True.

> 
> Since we pretty much only care about the power state of the device (and
> we are using the buffering state to infer that AFAIU), I was thinking
> in something like:
> 
> 
> mutex_lock(&data->lock);
> if (!data->powered) {
> 	ret = -EINVAL;
> } else {
>  	ret = max30100_get_temp(data, val);
>  	if (!ret)
>  		ret = IIO_VAL_INT;
> 
> }
> mutex_unlock(&data->lock);

LGTM.

> 
> Then, in the predisable, something like I have but setting the flag to
> false and the opposite on the postenable... Naturally we could also
> just read the registers (and I actually tend to prefer it) instead of a
> new flag but I guess the flag is enough in this case.
> 
> - Nuno Sá
> >   


Thanks,
Miquèl

Jonathan Cameron Sept. 24, 2022, 3:53 p.m. UTC | #8

On Tue, 20 Sep 2022 17:10:33 +0200
Miquel Raynal <[e-mail]> wrote:

> Hi Nuno,
> 
> [e-mail] wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> 
> > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:  
> > > Hi Nuno,
> > > 
> > > [e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > >     
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <[e-mail]>
> > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > To: Sa, Nuno <[e-mail]>
> > > > > Cc: [e-mail];
> > > > > [e-mail];
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]; Chunyan Zhang <[e-mail]>;
> > > > > Hennerich,
> > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > <[e-mail]>; Sascha Hauer
> > > > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > > > Kevin
> > > > > Hilman <[e-mail]>; Vladimir Zapolskiy <[e-mail]>;
> > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> > > > > Ardelean
> > > > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> > > > > Andriy
> > > > > Tryshnivskyy <[e-mail]>; Haibo Chen
> > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans de
> > > > > Goede <[e-mail]>; Jerome Brunet
> > > > > <[e-mail]>;
> > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > <[e-mail]>; Regus, Ciprian
> > > > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> > > > > Andy
> > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > > > Baolin
> > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson Zhai
> > > > > <[e-mail]>
> > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > internal iio_dev
> > > > > lock
> > > > > 
> > > > > [External]
> > > > > 
> > > > > Hi Nuno,
> > > > > 
> > > > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > > > >       
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > Cc: [e-mail]; linux-      
> > > > > [e-mail];      
> > > > > > > [e-mail]; [e-mail]; linux-
> > > > > > > [e-mail]; Chunyan Zhang <[e-mail]>;      
> > > > > Hennerich,      
> > > > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > <[e-mail]>; Cixi Geng <[e-mail]>;
> > > > > > > Kevin
> > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > <[e-mail]>;
> > > > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru      
> > > > > Ardelean      
> > > > > > > <[e-mail]>; Fabio Estevam <[e-mail]>;     
> > > > > Andriy      
> > > > > > > Tryshnivskyy <[e-mail]>; Haibo
> > > > > > > Chen
> > > > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans
> > > > > > > de
> > > > > > > Goede <[e-mail]>; Jerome Brunet      
> > > > > <[e-mail]>;      
> > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > <[e-mail]>;      
> > > > > Andy      
> > > > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > > > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > > > > > > Baolin
> > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson
> > > > > > > Zhai
> > > > > > > <[e-mail]>
> > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > > > internal      
> > > > > iio_dev      
> > > > > > > lock
> > > > > > > 
> > > > > > > [External]
> > > > > > > 
> > > > > > > Hi Nuno,
> > > > > > >      
> > > > > > 
> > > > > > Hi Miquel,
> > > > > > 
> > > > > > Thanks for reviewing...
> > > > > >      
> > > > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > > > > > >      
> > > > > > > > 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      
> > > > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > > > 
> > > > > > > > 1. Use iio_device_claim_direct_mode() to check if direct
> > > > > > > > mode can be
> > > > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > > > code);
> > > > > > > > 
> > > > > > > > 2. Make sure that buffering is not disabled while doing a
> > > > > > > > raw read. For
> > > > > > > > that, we can make use of the local lock that already
> > > > > > > > exists.
> > > > > > > > 
> > > > > > > > While at it, fixed a minor coding style complain...
> > > > > > > > 
> > > > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/iio/health/max30100.c | 24 +++++++++++++++++------
> > > > > > > > -
> > > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/iio/health/max30100.c      
> > > > > b/drivers/iio/health/max30100.c      
> > > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > > max30100_buffer_postenable(struct      
> > > > > > > iio_dev *indio_dev)      
> > > > > > > >  static int max30100_buffer_predisable(struct iio_dev
> > > > > > > > *indio_dev)
> > > > > > > >  {
> > > > > > > >         struct max30100_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 = max30100_set_powermode(data, false);
> > > > > > > > +       mutex_unlock(&data->lock);
> > > > > > > > 
> > > > > > > > -       return max30100_set_powermode(data, false);
> > > > > > > > +       return ret;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > >  static const struct iio_buffer_setup_ops
> > > > > > > > max30100_buffer_setup_ops      
> > > > > = {      
> > > > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > > > -
> > > > > > > > -               if (!iio_buffer_enabled(indio_dev))
> > > > > > > > +               if
> > > > > > > > (!iio_device_claim_direct_mode(indio_dev)) {      
> > > > > > > 
> > > > > > > I wonder if this line change here is really needed. I agree
> > > > > > > the whole
> > > > > > > construction looks like what iio_device_claim_direct_mode()
> > > > > > > does but in
> > > > > > > practice I don't see the point of acquiring any lock here if
> > > > > > > we just
> > > > > > > release it no matter what happens right after.
> > > > > > >      
> > > > > > 
> > > > > > I can see that this is odd (at the very least) but AFAIK, this
> > > > > > is the only way
> > > > > > to safely infer if buffering is enabled or not.
> > > > > > iio_buffer_enabled() has no
> > > > > > protection against someone concurrently enabling/disabling the
> > > > > > buffer.      
> > > > > 
> > > > > Yes, but this is only relevant if you want to infer that the
> > > > > "buffers
> > > > > are enabled" and be sure that it cannot be otherwise during the
> > > > > next
> > > > > lines until you release the lock. Acquiring a lock, doing the if
> > > > > and
> > > > > then unconditionally releasing the lock, IMHO, does not make any
> > > > > sense
> > > > > (but I'm not a locking guru) because when you enter the else
> > > > > clause,
> > > > > you are not protected anyway, so in both cases all this is
> > > > > completely
> > > > > racy.
> > > > >       
> > > > 
> > > > Ahh crap, yes you are right... It is still racy since we can still
> > > > try to read
> > > > the temperature with the device powered off. I'm not really sure
> > > > how to
> > > > address this. One way could be to just use an internal control
> > > > variable
> > > > to reflect the device power state (set/clear on the buffer
> > > > callbacks) and
> > > > only use the local lock (completely ditching the call to
> > > > iio_device_claim_direct_mode())...    
> > > 
> > > I tend to prefer this option than the one below.
> > > 
> > > I guess your implementation already prevents buffer_predisable() to
> > > run
> > > thanks to the local lock being held during the operation. Maybe we
> > > should just verify that buffers are enabled from within the local
> > > lock
> > > being held instead of just acquiring it for the get_temp() measure.
> > > It
> > > would probably solve the situation here.    
> > > >     
> > Not sure if I understood... You mean something like:
> > 
> > mutex_lock(&data->lock);
> > if (!iio_buffer_enabled(indio_dev)) {
> > 	ret = -EINVAL;
> > } else {
> >  	ret = max30100_get_temp(data, val);
> >  	if (!ret)
> >  		ret = IIO_VAL_INT;
> > 
> > }
> > mutex_unlock(&data->lock);
> > 
> > If so, I think this is still racy since we release the lock after the
> > predisable which means we could still detect the buffers as enabled (in
> > the above block) and try to get_temp on a powered down device.  
> 
> True.
> 
> > 
> > Since we pretty much only care about the power state of the device (and
> > we are using the buffering state to infer that AFAIU), I was thinking
> > in something like:
> > 
> > 
> > mutex_lock(&data->lock);
> > if (!data->powered) {
> > 	ret = -EINVAL;
> > } else {
> >  	ret = max30100_get_temp(data, val);
> >  	if (!ret)
> >  		ret = IIO_VAL_INT;
> > 
> > }
> > mutex_unlock(&data->lock);  
> 
> LGTM.

A reference counted power up flag would probably work as we'd want to disable
power only when the reference count goes to 0.  Note all checks of that flag
would need to be done under the lock as well.

As an alternative...
 
Whilst it is a serious oddity, how about flipping the logic and having
an iio_device_claim_buffered_mode() that takes mlock and holds it only
if we are in buffered mode - then holds it until matching release?

Now, I've only done a superficial audit of the buffer removal paths
to check they hold the lock before we call predisable() but it looks
like they do - so this should work.

Just wanted to muddy the waters :)

> 
> > 
> > Then, in the predisable, something like I have but setting the flag to
> > false and the opposite on the postenable... Naturally we could also
> > just read the registers (and I actually tend to prefer it) instead of a
> > new flag but I guess the flag is enough in this case.
> > 
> > - Nuno Sá  
> > >     
> 
> 
> Thanks,
> Miquèl

Nuno Sá Sept. 26, 2022, 10:06 a.m. UTC | #9

On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 17:10:33 +0200
> Miquel Raynal <[e-mail]> wrote:
> 
> > Hi Nuno,
> > 
> > [e-mail] wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > 
> > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:  
> > > > Hi Nuno,
> > > > 
> > > > [e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > Cc: [e-mail];
> > > > > > [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > linux-
> > > > > > [e-mail]; Chunyan Zhang <[e-mail]>;
> > > > > > Hennerich,
> > > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > > <[e-mail]>; Sascha Hauer
> > > > > > <[e-mail]>; Cixi Geng
> > > > > > <[e-mail]>;
> > > > > > Kevin
> > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > <[e-mail]>;
> > > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> > > > > > Ardelean
> > > > > > <[e-mail]>; Fabio Estevam
> > > > > > <[e-mail]>;
> > > > > > Andriy
> > > > > > Tryshnivskyy <[e-mail]>; Haibo
> > > > > > Chen
> > > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans
> > > > > > de
> > > > > > Goede <[e-mail]>; Jerome Brunet
> > > > > > <[e-mail]>;
> > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > <[e-mail]>;
> > > > > > Andy
> > > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > > <[e-mail]>; Neil Armstrong
> > > > > > <[e-mail]>;
> > > > > > Baolin
> > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson
> > > > > > Zhai
> > > > > > <[e-mail]>
> > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not
> > > > > > use
> > > > > > internal iio_dev
> > > > > > lock
> > > > > > 
> > > > > > [External]
> > > > > > 
> > > > > > Hi Nuno,
> > > > > > 
> > > > > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > +0000:
> > > > > >       
> > > > > > > > -----Original Message-----
> > > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > > Cc: [e-mail]; linux-      
> > > > > > [e-mail];      
> > > > > > > > [e-mail]; [e-mail];
> > > > > > > > linux-
> > > > > > > > [e-mail]; Chunyan Zhang
> > > > > > > > <[e-mail]>;      
> > > > > > Hennerich,      
> > > > > > > > Michael <[e-mail]>; Martin
> > > > > > > > Blumenstingl
> > > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > > <[e-mail]>; Cixi Geng
> > > > > > > > <[e-mail]>;
> > > > > > > > Kevin
> > > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > > <[e-mail]>;
> > > > > > > > Pengutronix Kernel Team <[e-mail]>;
> > > > > > > > Alexandru      
> > > > > > Ardelean      
> > > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > > <[e-mail]>;     
> > > > > > Andriy      
> > > > > > > > Tryshnivskyy <[e-mail]>;
> > > > > > > > Haibo
> > > > > > > > Chen
> > > > > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > > > > Hans
> > > > > > > > de
> > > > > > > > Goede <[e-mail]>; Jerome Brunet      
> > > > > > <[e-mail]>;      
> > > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > > <[e-mail]>;      
> > > > > > Andy      
> > > > > > > > Shevchenko <[e-mail]>; Jonathan
> > > > > > > > Cameron
> > > > > > > > <[e-mail]>; Neil Armstrong
> > > > > > > > <[e-mail]>;
> > > > > > > > Baolin
> > > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>;
> > > > > > > > Orson
> > > > > > > > Zhai
> > > > > > > > <[e-mail]>
> > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > not use
> > > > > > > > internal      
> > > > > > iio_dev      
> > > > > > > > lock
> > > > > > > > 
> > > > > > > > [External]
> > > > > > > > 
> > > > > > > > Hi Nuno,
> > > > > > > >      
> > > > > > > 
> > > > > > > Hi Miquel,
> > > > > > > 
> > > > > > > Thanks for reviewing...
> > > > > > >      
> > > > > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19
> > > > > > > > +0200:
> > > > > > > >      
> > > > > > > > > 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      
> > > > > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > > > > 
> > > > > > > > > 1. Use iio_device_claim_direct_mode() to check if
> > > > > > > > > direct
> > > > > > > > > mode can be
> > > > > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > > > > code);
> > > > > > > > > 
> > > > > > > > > 2. Make sure that buffering is not disabled while
> > > > > > > > > doing a
> > > > > > > > > raw read. For
> > > > > > > > > that, we can make use of the local lock that already
> > > > > > > > > exists.
> > > > > > > > > 
> > > > > > > > > While at it, fixed a minor coding style complain...
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  drivers/iio/health/max30100.c | 24
> > > > > > > > > +++++++++++++++++------
> > > > > > > > > -
> > > > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/iio/health/max30100.c      
> > > > > > b/drivers/iio/health/max30100.c      
> > > > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > > > max30100_buffer_postenable(struct      
> > > > > > > > iio_dev *indio_dev)      
> > > > > > > > >  static int max30100_buffer_predisable(struct iio_dev
> > > > > > > > > *indio_dev)
> > > > > > > > >  {
> > > > > > > > >         struct max30100_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 = max30100_set_powermode(data, false);
> > > > > > > > > +       mutex_unlock(&data->lock);
> > > > > > > > > 
> > > > > > > > > -       return max30100_set_powermode(data, false);
> > > > > > > > > +       return ret;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > >  static const struct iio_buffer_setup_ops
> > > > > > > > > max30100_buffer_setup_ops      
> > > > > > = {      
> > > > > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > > > > -
> > > > > > > > > -               if (!iio_buffer_enabled(indio_dev))
> > > > > > > > > +               if
> > > > > > > > > (!iio_device_claim_direct_mode(indio_dev)) {      
> > > > > > > > 
> > > > > > > > I wonder if this line change here is really needed. I
> > > > > > > > agree
> > > > > > > > the whole
> > > > > > > > construction looks like what
> > > > > > > > iio_device_claim_direct_mode()
> > > > > > > > does but in
> > > > > > > > practice I don't see the point of acquiring any lock
> > > > > > > > here if
> > > > > > > > we just
> > > > > > > > release it no matter what happens right after.
> > > > > > > >      
> > > > > > > 
> > > > > > > I can see that this is odd (at the very least) but AFAIK,
> > > > > > > this
> > > > > > > is the only way
> > > > > > > to safely infer if buffering is enabled or not.
> > > > > > > iio_buffer_enabled() has no
> > > > > > > protection against someone concurrently
> > > > > > > enabling/disabling the
> > > > > > > buffer.      
> > > > > > 
> > > > > > Yes, but this is only relevant if you want to infer that
> > > > > > the
> > > > > > "buffers
> > > > > > are enabled" and be sure that it cannot be otherwise during
> > > > > > the
> > > > > > next
> > > > > > lines until you release the lock. Acquiring a lock, doing
> > > > > > the if
> > > > > > and
> > > > > > then unconditionally releasing the lock, IMHO, does not
> > > > > > make any
> > > > > > sense
> > > > > > (but I'm not a locking guru) because when you enter the
> > > > > > else
> > > > > > clause,
> > > > > > you are not protected anyway, so in both cases all this is
> > > > > > completely
> > > > > > racy.
> > > > > >       
> > > > > 
> > > > > Ahh crap, yes you are right... It is still racy since we can
> > > > > still
> > > > > try to read
> > > > > the temperature with the device powered off. I'm not really
> > > > > sure
> > > > > how to
> > > > > address this. One way could be to just use an internal
> > > > > control
> > > > > variable
> > > > > to reflect the device power state (set/clear on the buffer
> > > > > callbacks) and
> > > > > only use the local lock (completely ditching the call to
> > > > > iio_device_claim_direct_mode())...    
> > > > 
> > > > I tend to prefer this option than the one below.
> > > > 
> > > > I guess your implementation already prevents
> > > > buffer_predisable() to
> > > > run
> > > > thanks to the local lock being held during the operation. Maybe
> > > > we
> > > > should just verify that buffers are enabled from within the
> > > > local
> > > > lock
> > > > being held instead of just acquiring it for the get_temp()
> > > > measure.
> > > > It
> > > > would probably solve the situation here.    
> > > > >     
> > > Not sure if I understood... You mean something like:
> > > 
> > > mutex_lock(&data->lock);
> > > if (!iio_buffer_enabled(indio_dev)) {
> > >         ret = -EINVAL;
> > > } else {
> > >         ret = max30100_get_temp(data, val);
> > >         if (!ret)
> > >                 ret = IIO_VAL_INT;
> > > 
> > > }
> > > mutex_unlock(&data->lock);
> > > 
> > > If so, I think this is still racy since we release the lock after
> > > the
> > > predisable which means we could still detect the buffers as
> > > enabled (in
> > > the above block) and try to get_temp on a powered down device.  
> > 
> > True.
> > 
> > > 
> > > Since we pretty much only care about the power state of the
> > > device (and
> > > we are using the buffering state to infer that AFAIU), I was
> > > thinking
> > > in something like:
> > > 
> > > 
> > > mutex_lock(&data->lock);
> > > if (!data->powered) {
> > >         ret = -EINVAL;
> > > } else {
> > >         ret = max30100_get_temp(data, val);
> > >         if (!ret)
> > >                 ret = IIO_VAL_INT;
> > > 
> > > }
> > > mutex_unlock(&data->lock);  
> > 
> > LGTM.
> 
> A reference counted power up flag would probably work as we'd want to
> disable
> power only when the reference count goes to 0.  Note all checks of
> that flag
> would need to be done under the lock as well.
> 

Is there any way to enable a buffer more than once? Otherwise I'm not
sure we really need a refcount... Any ways, your below approach looks
good to me and surely easier.

> As an alternative...
>  
> Whilst it is a serious oddity, how about flipping the logic and
> having
> an iio_device_claim_buffered_mode() that takes mlock and holds it
> only
> if we are in buffered mode - then holds it until matching release?
> 

This goes along with one of my suggestions:

"A version  iio_device_claim_direct_mode() that does not release the 
lock in case buffering is enabled."

You just gave it a name (and one that I would not ever remember)...

> Now, I've only done a superficial audit of the buffer removal paths
> to check they hold the lock before we call predisable() but it looks

Otherwise I guess we would have to fix it :)


- Nuno Sá

Jonathan Cameron Oct. 2, 2022, 11:03 a.m. UTC | #10

On Mon, 26 Sep 2022 12:06:13 +0200
Nuno Sá <[e-mail]> wrote:

> On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> > On Tue, 20 Sep 2022 17:10:33 +0200
> > Miquel Raynal <[e-mail]> wrote:
> >   
> > > Hi Nuno,
> > > 
> > > [e-mail] wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > >   
> > > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:    
> > > > > Hi Nuno,
> > > > > 
> > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > > > >       
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > Cc: [e-mail];
> > > > > > > [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > linux-
> > > > > > > [e-mail]; Chunyan Zhang <[e-mail]>;
> > > > > > > Hennerich,
> > > > > > > Michael <[e-mail]>; Martin Blumenstingl
> > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > <[e-mail]>; Cixi Geng
> > > > > > > <[e-mail]>;
> > > > > > > Kevin
> > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > <[e-mail]>;
> > > > > > > Pengutronix Kernel Team <[e-mail]>; Alexandru
> > > > > > > Ardelean
> > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > <[e-mail]>;
> > > > > > > Andriy
> > > > > > > Tryshnivskyy <[e-mail]>; Haibo
> > > > > > > Chen
> > > > > > > <[e-mail]>; Shawn Guo <[e-mail]>; Hans
> > > > > > > de
> > > > > > > Goede <[e-mail]>; Jerome Brunet
> > > > > > > <[e-mail]>;
> > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > <[e-mail]>;
> > > > > > > Andy
> > > > > > > Shevchenko <[e-mail]>; Jonathan Cameron
> > > > > > > <[e-mail]>; Neil Armstrong
> > > > > > > <[e-mail]>;
> > > > > > > Baolin
> > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Orson
> > > > > > > Zhai
> > > > > > > <[e-mail]>
> > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not
> > > > > > > use
> > > > > > > internal iio_dev
> > > > > > > lock
> > > > > > > 
> > > > > > > [External]
> > > > > > > 
> > > > > > > Hi Nuno,
> > > > > > > 
> > > > > > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > > +0000:
> > > > > > >         
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > > > Cc: [e-mail]; linux-        
> > > > > > > [e-mail];        
> > > > > > > > > [e-mail]; [e-mail];
> > > > > > > > > linux-
> > > > > > > > > [e-mail]; Chunyan Zhang
> > > > > > > > > <[e-mail]>;        
> > > > > > > Hennerich,        
> > > > > > > > > Michael <[e-mail]>; Martin
> > > > > > > > > Blumenstingl
> > > > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > > > <[e-mail]>; Cixi Geng
> > > > > > > > > <[e-mail]>;
> > > > > > > > > Kevin
> > > > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > > > <[e-mail]>;
> > > > > > > > > Pengutronix Kernel Team <[e-mail]>;
> > > > > > > > > Alexandru        
> > > > > > > Ardelean        
> > > > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > > > <[e-mail]>;       
> > > > > > > Andriy        
> > > > > > > > > Tryshnivskyy <[e-mail]>;
> > > > > > > > > Haibo
> > > > > > > > > Chen
> > > > > > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > > > > > Hans
> > > > > > > > > de
> > > > > > > > > Goede <[e-mail]>; Jerome Brunet        
> > > > > > > <[e-mail]>;        
> > > > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > > > <[e-mail]>;        
> > > > > > > Andy        
> > > > > > > > > Shevchenko <[e-mail]>; Jonathan
> > > > > > > > > Cameron
> > > > > > > > > <[e-mail]>; Neil Armstrong
> > > > > > > > > <[e-mail]>;
> > > > > > > > > Baolin
> > > > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>;
> > > > > > > > > Orson
> > > > > > > > > Zhai
> > > > > > > > > <[e-mail]>
> > > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > > not use
> > > > > > > > > internal        
> > > > > > > iio_dev        
> > > > > > > > > lock
> > > > > > > > > 
> > > > > > > > > [External]
> > > > > > > > > 
> > > > > > > > > Hi Nuno,
> > > > > > > > >        
> > > > > > > > 
> > > > > > > > Hi Miquel,
> > > > > > > > 
> > > > > > > > Thanks for reviewing...
> > > > > > > >        
> > > > > > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:28:19
> > > > > > > > > +0200:
> > > > > > > > >        
> > > > > > > > > > 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        
> > > > > > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > > > > > 
> > > > > > > > > > 1. Use iio_device_claim_direct_mode() to check if
> > > > > > > > > > direct
> > > > > > > > > > mode can be
> > > > > > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > > > > > code);
> > > > > > > > > > 
> > > > > > > > > > 2. Make sure that buffering is not disabled while
> > > > > > > > > > doing a
> > > > > > > > > > raw read. For
> > > > > > > > > > that, we can make use of the local lock that already
> > > > > > > > > > exists.
> > > > > > > > > > 
> > > > > > > > > > While at it, fixed a minor coding style complain...
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/iio/health/max30100.c | 24
> > > > > > > > > > +++++++++++++++++------
> > > > > > > > > > -
> > > > > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/iio/health/max30100.c        
> > > > > > > b/drivers/iio/health/max30100.c        
> > > > > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > > > > max30100_buffer_postenable(struct        
> > > > > > > > > iio_dev *indio_dev)        
> > > > > > > > > >  static int max30100_buffer_predisable(struct iio_dev
> > > > > > > > > > *indio_dev)
> > > > > > > > > >  {
> > > > > > > > > >         struct max30100_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 = max30100_set_powermode(data, false);
> > > > > > > > > > +       mutex_unlock(&data->lock);
> > > > > > > > > > 
> > > > > > > > > > -       return max30100_set_powermode(data, false);
> > > > > > > > > > +       return ret;
> > > > > > > > > >  }
> > > > > > > > > > 
> > > > > > > > > >  static const struct iio_buffer_setup_ops
> > > > > > > > > > max30100_buffer_setup_ops        
> > > > > > > = {        
> > > > > > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > > > > > -
> > > > > > > > > > -               if (!iio_buffer_enabled(indio_dev))
> > > > > > > > > > +               if
> > > > > > > > > > (!iio_device_claim_direct_mode(indio_dev)) {        
> > > > > > > > > 
> > > > > > > > > I wonder if this line change here is really needed. I
> > > > > > > > > agree
> > > > > > > > > the whole
> > > > > > > > > construction looks like what
> > > > > > > > > iio_device_claim_direct_mode()
> > > > > > > > > does but in
> > > > > > > > > practice I don't see the point of acquiring any lock
> > > > > > > > > here if
> > > > > > > > > we just
> > > > > > > > > release it no matter what happens right after.
> > > > > > > > >        
> > > > > > > > 
> > > > > > > > I can see that this is odd (at the very least) but AFAIK,
> > > > > > > > this
> > > > > > > > is the only way
> > > > > > > > to safely infer if buffering is enabled or not.
> > > > > > > > iio_buffer_enabled() has no
> > > > > > > > protection against someone concurrently
> > > > > > > > enabling/disabling the
> > > > > > > > buffer.        
> > > > > > > 
> > > > > > > Yes, but this is only relevant if you want to infer that
> > > > > > > the
> > > > > > > "buffers
> > > > > > > are enabled" and be sure that it cannot be otherwise during
> > > > > > > the
> > > > > > > next
> > > > > > > lines until you release the lock. Acquiring a lock, doing
> > > > > > > the if
> > > > > > > and
> > > > > > > then unconditionally releasing the lock, IMHO, does not
> > > > > > > make any
> > > > > > > sense
> > > > > > > (but I'm not a locking guru) because when you enter the
> > > > > > > else
> > > > > > > clause,
> > > > > > > you are not protected anyway, so in both cases all this is
> > > > > > > completely
> > > > > > > racy.
> > > > > > >         
> > > > > > 
> > > > > > Ahh crap, yes you are right... It is still racy since we can
> > > > > > still
> > > > > > try to read
> > > > > > the temperature with the device powered off. I'm not really
> > > > > > sure
> > > > > > how to
> > > > > > address this. One way could be to just use an internal
> > > > > > control
> > > > > > variable
> > > > > > to reflect the device power state (set/clear on the buffer
> > > > > > callbacks) and
> > > > > > only use the local lock (completely ditching the call to
> > > > > > iio_device_claim_direct_mode())...      
> > > > > 
> > > > > I tend to prefer this option than the one below.
> > > > > 
> > > > > I guess your implementation already prevents
> > > > > buffer_predisable() to
> > > > > run
> > > > > thanks to the local lock being held during the operation. Maybe
> > > > > we
> > > > > should just verify that buffers are enabled from within the
> > > > > local
> > > > > lock
> > > > > being held instead of just acquiring it for the get_temp()
> > > > > measure.
> > > > > It
> > > > > would probably solve the situation here.      
> > > > > >       
> > > > Not sure if I understood... You mean something like:
> > > > 
> > > > mutex_lock(&data->lock);
> > > > if (!iio_buffer_enabled(indio_dev)) {
> > > >         ret = -EINVAL;
> > > > } else {
> > > >         ret = max30100_get_temp(data, val);
> > > >         if (!ret)
> > > >                 ret = IIO_VAL_INT;
> > > > 
> > > > }
> > > > mutex_unlock(&data->lock);
> > > > 
> > > > If so, I think this is still racy since we release the lock after
> > > > the
> > > > predisable which means we could still detect the buffers as
> > > > enabled (in
> > > > the above block) and try to get_temp on a powered down device.    
> > > 
> > > True.
> > >   
> > > > 
> > > > Since we pretty much only care about the power state of the
> > > > device (and
> > > > we are using the buffering state to infer that AFAIU), I was
> > > > thinking
> > > > in something like:
> > > > 
> > > > 
> > > > mutex_lock(&data->lock);
> > > > if (!data->powered) {
> > > >         ret = -EINVAL;
> > > > } else {
> > > >         ret = max30100_get_temp(data, val);
> > > >         if (!ret)
> > > >                 ret = IIO_VAL_INT;
> > > > 
> > > > }
> > > > mutex_unlock(&data->lock);    
> > > 
> > > LGTM.  
> > 
> > A reference counted power up flag would probably work as we'd want to
> > disable
> > power only when the reference count goes to 0.  Note all checks of
> > that flag
> > would need to be done under the lock as well.
> >   
> 
> Is there any way to enable a buffer more than once? Otherwise I'm not
> sure we really need a refcount... Any ways, your below approach looks
> good to me and surely easier.

Indeed can only have the buffer enabled once, but there may be a race
with the disable of the buffer and this path. Hence need a ref count
to detect when the count goes to zero, what ever the reason.
A flag only covers one side of the race.

> 
> > As an alternative...
> >  
> > Whilst it is a serious oddity, how about flipping the logic and
> > having
> > an iio_device_claim_buffered_mode() that takes mlock and holds it
> > only
> > if we are in buffered mode - then holds it until matching release?
> >   
> 
> This goes along with one of my suggestions:
> 
> "A version  iio_device_claim_direct_mode() that does not release the 
> lock in case buffering is enabled."

Ah. I missed that suggestion.

> 
> You just gave it a name (and one that I would not ever remember)...

:) It's used in so few paths that I'm not that bothered if it's hard
to remember!

> 
> > Now, I've only done a superficial audit of the buffer removal paths
> > to check they hold the lock before we call predisable() but it looks  
> 
> Otherwise I guess we would have to fix it :)
True, though testing these more obscure devices is a real pain.

Jonathan

> 
> 
> - Nuno Sá

Nuno Sá Oct. 4, 2022, 7:57 a.m. UTC | #11

On Sun, 2022-10-02 at 12:03 +0100, Jonathan Cameron wrote:
> On Mon, 26 Sep 2022 12:06:13 +0200
> Nuno Sá <[e-mail]> wrote:
> 
> > On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> > > On Tue, 20 Sep 2022 17:10:33 +0200
> > > Miquel Raynal <[e-mail]> wrote:
> > >   
> > > > Hi Nuno,
> > > > 
> > > > [e-mail] wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > > >   
> > > > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:    
> > > > > > Hi Nuno,
> > > > > > 
> > > > > > [e-mail] wrote on Tue, 20 Sep 2022 13:15:32
> > > > > > +0000:
> > > > > >       
> > > > > > > > -----Original Message-----
> > > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > > Cc: [e-mail];
> > > > > > > > [e-mail];
> > > > > > > > [e-mail]; [e-mail];
> > > > > > > > linux-
> > > > > > > > [e-mail]; Chunyan Zhang
> > > > > > > > <[e-mail]>;
> > > > > > > > Hennerich,
> > > > > > > > Michael <[e-mail]>; Martin
> > > > > > > > Blumenstingl
> > > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > > <[e-mail]>; Cixi Geng
> > > > > > > > <[e-mail]>;
> > > > > > > > Kevin
> > > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > > <[e-mail]>;
> > > > > > > > Pengutronix Kernel Team <[e-mail]>;
> > > > > > > > Alexandru
> > > > > > > > Ardelean
> > > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > > <[e-mail]>;
> > > > > > > > Andriy
> > > > > > > > Tryshnivskyy <[e-mail]>;
> > > > > > > > Haibo
> > > > > > > > Chen
> > > > > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > > > > Hans
> > > > > > > > de
> > > > > > > > Goede <[e-mail]>; Jerome Brunet
> > > > > > > > <[e-mail]>;
> > > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > > <[e-mail]>;
> > > > > > > > Andy
> > > > > > > > Shevchenko <[e-mail]>; Jonathan
> > > > > > > > Cameron
> > > > > > > > <[e-mail]>; Neil Armstrong
> > > > > > > > <[e-mail]>;
> > > > > > > > Baolin
> > > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > > <[e-mail]>; Chen-Yu Tsai <[e-mail]>;
> > > > > > > > Orson
> > > > > > > > Zhai
> > > > > > > > <[e-mail]>
> > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > not
> > > > > > > > use
> > > > > > > > internal iio_dev
> > > > > > > > lock
> > > > > > > > 
> > > > > > > > [External]
> > > > > > > > 
> > > > > > > > Hi Nuno,
> > > > > > > > 
> > > > > > > > [e-mail] wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > > > +0000:
> > > > > > > >         
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Miquel Raynal <[e-mail]>
> > > > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > > > To: Sa, Nuno <[e-mail]>
> > > > > > > > > > Cc: [e-mail]; linux-
> > > > > > > > > >         
> > > > > > > > [e-mail];        
> > > > > > > > > > [e-mail];
> > > > > > > > > > [e-mail];
> > > > > > > > > > linux-
> > > > > > > > > > [e-mail]; Chunyan Zhang
> > > > > > > > > > <[e-mail]>;        
> > > > > > > > Hennerich,        
> > > > > > > > > > Michael <[e-mail]>; Martin
> > > > > > > > > > Blumenstingl
> > > > > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > > > > <[e-mail]>; Cixi Geng
> > > > > > > > > > <[e-mail]>;
> > > > > > > > > > Kevin
> > > > > > > > > > Hilman <[e-mail]>; Vladimir Zapolskiy
> > > > > > > > > > <[e-mail]>;
> > > > > > > > > > Pengutronix Kernel Team <[e-mail]>;
> > > > > > > > > > Alexandru        
> > > > > > > > Ardelean        
> > > > > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > > > > <[e-mail]>;       
> > > > > > > > Andriy        
> > > > > > > > > > Tryshnivskyy <[e-mail]>;
> > > > > > > > > > Haibo
> > > > > > > > > > Chen
> > > > > > > > > > <[e-mail]>; Shawn Guo
> > > > > > > > > > <[e-mail]>;
> > > > > > > > > > Hans
> > > > > > > > > > de
> > > > > > > > > > Goede <[e-mail]>; Jerome Brunet        
> > > > > > > > <[e-mail]>;        
> > > > > > > > > > Heiko Stuebner <[e-mail]>; Florian Boor
> > > > > > > > > > <[e-mail]>; Regus, Ciprian
> > > > > > > > > > <[e-mail]>; Lars-Peter Clausen
> > > > > > > > > > <[e-mail]>;        
> > > > > > > > Andy        
> > > > > > > > > > Shevchenko <[e-mail]>; Jonathan
> > > > > > > > > > Cameron
> > > > > > > > > > <[e-mail]>; Neil Armstrong
> > > > > > > > > > <[e-mail]>;
> > > > > > > > > > Baolin
> > > > > > > > > > Wang <[e-mail]>; Jyoti Bhayana
> > > > > > > > > > <[e-mail]>; Chen-Yu Tsai
> > > > > > > > > > <[e-mail]>;
> > > > > > > > > > Orson
> > > > > > > > > > Zhai
> > > > > > > > > > <[e-mail]>
> > > > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100:
> > > > > > > > > > do
> > > > > > > > > > not use
> > > > > > > > > > internal        
> > > > > > > > iio_dev        
> > > > > > > > > > lock
> > > > > > > > > > 
> > > > > > > > > > [External]
> > > > > > > > > > 
> > > > > > > > > > Hi Nuno,
> > > > > > > > > >        
> > > > > > > > > 
> > > > > > > > > Hi Miquel,
> > > > > > > > > 
> > > > > > > > > Thanks for reviewing...
> > > > > > > > >        
> > > > > > > > > > [e-mail] wrote on Tue, 20 Sep 2022
> > > > > > > > > > 13:28:19
> > > > > > > > > > +0200:
> > > > > > > > > >        
> > > > > > > > > > > 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        
> > > > > > > > > > > allow it. Hence to get rid of the 'mlock' we need
> > > > > > > > > > > to:
> > > > > > > > > > > 
> > > > > > > > > > > 1. Use iio_device_claim_direct_mode() to check if
> > > > > > > > > > > direct
> > > > > > > > > > > mode can be
> > > > > > > > > > > claimed and if we can return -EINVAL (as the
> > > > > > > > > > > original
> > > > > > > > > > > code);
> > > > > > > > > > > 
> > > > > > > > > > > 2. Make sure that buffering is not disabled while
> > > > > > > > > > > doing a
> > > > > > > > > > > raw read. For
> > > > > > > > > > > that, we can make use of the local lock that
> > > > > > > > > > > already
> > > > > > > > > > > exists.
> > > > > > > > > > > 
> > > > > > > > > > > While at it, fixed a minor coding style
> > > > > > > > > > > complain...
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Nuno Sá <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/iio/health/max30100.c | 24
> > > > > > > > > > > +++++++++++++++++------
> > > > > > > > > > > -
> > > > > > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/iio/health/max30100.c       
> > > > > > > > b/drivers/iio/health/max30100.c        
> > > > > > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > > > > > max30100_buffer_postenable(struct        
> > > > > > > > > > iio_dev *indio_dev)        
> > > > > > > > > > >  static int max30100_buffer_predisable(struct
> > > > > > > > > > > iio_dev
> > > > > > > > > > > *indio_dev)
> > > > > > > > > > >  {
> > > > > > > > > > >         struct max30100_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 = max30100_set_powermode(data,
> > > > > > > > > > > false);
> > > > > > > > > > > +       mutex_unlock(&data->lock);
> > > > > > > > > > > 
> > > > > > > > > > > -       return max30100_set_powermode(data,
> > > > > > > > > > > false);
> > > > > > > > > > > +       return ret;
> > > > > > > > > > >  }
> > > > > > > > > > > 
> > > > > > > > > > >  static const struct iio_buffer_setup_ops
> > > > > > > > > > > max30100_buffer_setup_ops        
> > > > > > > > = {        
> > > > > > > > > > > @@ -387,18 +398,17 @@ 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);
> > > > > > > > > > > -
> > > > > > > > > > > -               if
> > > > > > > > > > > (!iio_buffer_enabled(indio_dev))
> > > > > > > > > > > +               if
> > > > > > > > > > > (!iio_device_claim_direct_mode(indio_dev))
> > > > > > > > > > > {        
> > > > > > > > > > 
> > > > > > > > > > I wonder if this line change here is really needed.
> > > > > > > > > > I
> > > > > > > > > > agree
> > > > > > > > > > the whole
> > > > > > > > > > construction looks like what
> > > > > > > > > > iio_device_claim_direct_mode()
> > > > > > > > > > does but in
> > > > > > > > > > practice I don't see the point of acquiring any
> > > > > > > > > > lock
> > > > > > > > > > here if
> > > > > > > > > > we just
> > > > > > > > > > release it no matter what happens right after.
> > > > > > > > > >        
> > > > > > > > > 
> > > > > > > > > I can see that this is odd (at the very least) but
> > > > > > > > > AFAIK,
> > > > > > > > > this
> > > > > > > > > is the only way
> > > > > > > > > to safely infer if buffering is enabled or not.
> > > > > > > > > iio_buffer_enabled() has no
> > > > > > > > > protection against someone concurrently
> > > > > > > > > enabling/disabling the
> > > > > > > > > buffer.        
> > > > > > > > 
> > > > > > > > Yes, but this is only relevant if you want to infer
> > > > > > > > that
> > > > > > > > the
> > > > > > > > "buffers
> > > > > > > > are enabled" and be sure that it cannot be otherwise
> > > > > > > > during
> > > > > > > > the
> > > > > > > > next
> > > > > > > > lines until you release the lock. Acquiring a lock,
> > > > > > > > doing
> > > > > > > > the if
> > > > > > > > and
> > > > > > > > then unconditionally releasing the lock, IMHO, does not
> > > > > > > > make any
> > > > > > > > sense
> > > > > > > > (but I'm not a locking guru) because when you enter the
> > > > > > > > else
> > > > > > > > clause,
> > > > > > > > you are not protected anyway, so in both cases all this
> > > > > > > > is
> > > > > > > > completely
> > > > > > > > racy.
> > > > > > > >         
> > > > > > > 
> > > > > > > Ahh crap, yes you are right... It is still racy since we
> > > > > > > can
> > > > > > > still
> > > > > > > try to read
> > > > > > > the temperature with the device powered off. I'm not
> > > > > > > really
> > > > > > > sure
> > > > > > > how to
> > > > > > > address this. One way could be to just use an internal
> > > > > > > control
> > > > > > > variable
> > > > > > > to reflect the device power state (set/clear on the
> > > > > > > buffer
> > > > > > > callbacks) and
> > > > > > > only use the local lock (completely ditching the call to
> > > > > > > iio_device_claim_direct_mode())...      
> > > > > > 
> > > > > > I tend to prefer this option than the one below.
> > > > > > 
> > > > > > I guess your implementation already prevents
> > > > > > buffer_predisable() to
> > > > > > run
> > > > > > thanks to the local lock being held during the operation.
> > > > > > Maybe
> > > > > > we
> > > > > > should just verify that buffers are enabled from within the
> > > > > > local
> > > > > > lock
> > > > > > being held instead of just acquiring it for the get_temp()
> > > > > > measure.
> > > > > > It
> > > > > > would probably solve the situation here.      
> > > > > > >       
> > > > > Not sure if I understood... You mean something like:
> > > > > 
> > > > > mutex_lock(&data->lock);
> > > > > if (!iio_buffer_enabled(indio_dev)) {
> > > > >         ret = -EINVAL;
> > > > > } else {
> > > > >         ret = max30100_get_temp(data, val);
> > > > >         if (!ret)
> > > > >                 ret = IIO_VAL_INT;
> > > > > 
> > > > > }
> > > > > mutex_unlock(&data->lock);
> > > > > 
> > > > > If so, I think this is still racy since we release the lock
> > > > > after
> > > > > the
> > > > > predisable which means we could still detect the buffers as
> > > > > enabled (in
> > > > > the above block) and try to get_temp on a powered down
> > > > > device.    
> > > > 
> > > > True.
> > > >   
> > > > > 
> > > > > Since we pretty much only care about the power state of the
> > > > > device (and
> > > > > we are using the buffering state to infer that AFAIU), I was
> > > > > thinking
> > > > > in something like:
> > > > > 
> > > > > 
> > > > > mutex_lock(&data->lock);
> > > > > if (!data->powered) {
> > > > >         ret = -EINVAL;
> > > > > } else {
> > > > >         ret = max30100_get_temp(data, val);
> > > > >         if (!ret)
> > > > >                 ret = IIO_VAL_INT;
> > > > > 
> > > > > }
> > > > > mutex_unlock(&data->lock);    
> > > > 
> > > > LGTM.  
> > > 
> > > A reference counted power up flag would probably work as we'd
> > > want to
> > > disable
> > > power only when the reference count goes to 0.  Note all checks
> > > of
> > > that flag
> > > would need to be done under the lock as well.
> > >   
> > 
> > Is there any way to enable a buffer more than once? Otherwise I'm
> > not
> > sure we really need a refcount... Any ways, your below approach
> > looks
> > good to me and surely easier.
> 
> Indeed can only have the buffer enabled once, but there may be a race
> with the disable of the buffer and this path. Hence need a ref count
> to detect when the count goes to zero, what ever the reason.
> A flag only covers one side of the race.
> 
> > > 
Sure, but I would "secure" it with the same lock on 'data->lock' and
the flag setting would be obviously under that lock (same on the
postenable side of things). IMO, I don't think we need to that far as
using refcounts in here. Anyways, I will give a try to the suggestion
you made in the other patch.

- Nuno Sá
> >

Patch

diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index ad5717965223..aa494cad5df0 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -185,8 +185,19 @@  static int max30100_buffer_postenable(struct iio_dev *indio_dev)
 static int max30100_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct max30100_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 = max30100_set_powermode(data, false);
+	mutex_unlock(&data->lock);
 
-	return max30100_set_powermode(data, false);
+	return ret;
 }
 
 static const struct iio_buffer_setup_ops max30100_buffer_setup_ops = {
@@ -387,18 +398,17 @@  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);
-
-		if (!iio_buffer_enabled(indio_dev))
+		if (!iio_device_claim_direct_mode(indio_dev)) {
 			ret = -EAGAIN;
-		else {
+			iio_device_release_direct_mode(indio_dev);
+		} else {
+			mutex_lock(&data->lock);
 			ret = max30100_get_temp(data, val);
 			if (!ret)
 				ret = IIO_VAL_INT;
-
+			mutex_unlock(&data->lock);
 		}
 
-		mutex_unlock(&indio_dev->mlock);
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1;  /* 0.0625 */