[PATCH 00/15] Make 'mlock' really private

[PATCH 00/15] Make 'mlock' really private

[Nuno Sá]

This patchset cleans all the drivers directly using the iio_device 'mlock'.
This lock is private and should not be used outside the core (or by using
proper helpers).

Most of the conversions where straight, but there are some that really need
extra looking. Mainly patches [13/15] and [14/15] were a bit hacky since
iio_device_claim_direct_mode() does not fit 100%. The reason is that we
want to check if the device is buffering and do something if it is (in
which case the API return -EBUSY and released the lock. I just used a
combinations of locks to get around this (hopefully I did not messed up).

I was also not sure if patch [1/15] deserves a Fixes: tag but, to me, it
is really fixing a potential race so I added it.

Note that this series was only compiled tested using allyesconfig for
ARM. I ran 'git grep' to make sure there were no more users of 'mlock'.
Hopefully I covered them all...

Nuno Sá (15):
  iio: adc: ad_sigma_delta: do not use internal iio_dev lock
  iio: adc: ad799x: do not use internal iio_dev lock
  iio: adc: axp288_adc: do not use internal iio_dev lock
  iio: adc: imx7d_adc: do not use internal iio_dev lock
  iio: adc: lpc32xx_adc: do not use internal iio_dev lock
  iio: adc: ltc2947-core: do not use internal iio_dev lock
  iio: adc: meson_saradc: do not use internal iio_dev lock
  iio: adc: rockchip_saradc: do not use internal iio_dev lock
  iio: adc: sc27xx_adc: do not use internal iio_dev lock
  iio: adc: vf610_adc: do not use internal iio_dev lock
  iio: common: scmi_iio: do not use internal iio_dev lock
  iio: fyro: itg3200_core: do not use internal iio_dev lock
  iio: health: max30100: do not use internal iio_dev lock
  iio: health: max30102: do not use internal iio_dev lock
  iio: core: move 'mlock' to 'struct iio_dev_opaque'

 drivers/iio/TODO                           |  3 ---
 drivers/iio/adc/ad799x.c                   | 18 ++++++++-----
 drivers/iio/adc/ad_sigma_delta.c           |  8 +++---
 drivers/iio/adc/axp288_adc.c               |  8 ++++--
 drivers/iio/adc/imx7d_adc.c                | 13 ++++++----
 drivers/iio/adc/lpc32xx_adc.c              | 10 +++++---
 drivers/iio/adc/ltc2497-core.c             |  7 +++--
 drivers/iio/adc/ltc2497.h                  |  2 ++
 drivers/iio/adc/meson_saradc.c             | 10 +++++---
 drivers/iio/adc/rockchip_saradc.c          | 14 ++++++----
 drivers/iio/adc/sc27xx_adc.c               | 12 ++++++---
 drivers/iio/adc/vf610_adc.c                | 30 ++++++++++++++--------
 drivers/iio/common/scmi_sensors/scmi_iio.c |  9 +++++--
 drivers/iio/gyro/itg3200_core.c            |  9 ++++---
 drivers/iio/health/max30100.c              | 24 ++++++++++++-----
 drivers/iio/health/max30102.c              | 25 +++++++++++++-----
 drivers/iio/industrialio-buffer.c          | 29 ++++++++++++---------
 drivers/iio/industrialio-core.c            | 18 +++++++------
 drivers/iio/industrialio-event.c           |  4 +--
 drivers/iio/industrialio-trigger.c         | 12 ++++-----
 include/linux/iio/gyro/itg3200.h           |  2 ++
 include/linux/iio/iio-opaque.h             |  2 ++
 include/linux/iio/iio.h                    |  3 ---
 23 files changed, 175 insertions(+), 97 deletions(-)

-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/15] iio: adc: ad_sigma_delta: do not use internal iio_dev lock

[Nuno Sá]

Drop 'mlock' usage by making use of iio_device_claim_direct_mode().
This change actually makes sure we cannot do a single conversion while
buffering is enable. Note there was a potential race in the previous
code since we were only acquiring the lock after checking if the bus is
enabled.

Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 261a9a6b45e1..d8570f620785 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	unsigned int data_reg;
 	int ret = 0;
 
-	if (iio_buffer_enabled(indio_dev))
-		return -EBUSY;
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 
-	mutex_lock(&indio_dev->mlock);
 	ad_sigma_delta_set_channel(sigma_delta, chan->address);
 
 	spi_bus_lock(sigma_delta->spi->master);
@@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->master);
-	mutex_unlock(&indio_dev->mlock);
+	iio_device_release_direct_mode(indio_dev);
 
 	if (ret)
 		return ret;
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/15] iio: adc: ad_sigma_delta: do not use internal iio_dev lock

[Miquel Raynal]

Hi Nuno,

nuno.sa@analog.com wrote on Tue, 20 Sep 2022 13:28:07 +0200:

> Drop 'mlock' usage by making use of iio_device_claim_direct_mode().
> This change actually makes sure we cannot do a single conversion while
> buffering is enable. Note there was a potential race in the previous
> code since we were only acquiring the lock after checking if the bus is
> enabled.
> 
> Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices")

To answer your question in the cover letter, I feel like this Fixes:
tag is deserved.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 261a9a6b45e1..d8570f620785 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  	unsigned int data_reg;
>  	int ret = 0;
>  
> -	if (iio_buffer_enabled(indio_dev))
> -		return -EBUSY;
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
>  
> -	mutex_lock(&indio_dev->mlock);
>  	ad_sigma_delta_set_channel(sigma_delta, chan->address);
>  
>  	spi_bus_lock(sigma_delta->spi->master);
> @@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
>  	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->master);
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  
>  	if (ret)
>  		return ret;


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/15] iio: adc: ad_sigma_delta: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:54:10 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Nuno,
> 
> nuno.sa@analog.com wrote on Tue, 20 Sep 2022 13:28:07 +0200:
> 
> > Drop 'mlock' usage by making use of iio_device_claim_direct_mode().
> > This change actually makes sure we cannot do a single conversion while
> > buffering is enable. Note there was a potential race in the previous
> > code since we were only acquiring the lock after checking if the bus is
> > enabled.
> > 
> > Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices")  
> 
> To answer your question in the cover letter, I feel like this Fixes:
> tag is deserved.
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Agreed to the fixes tag being valid.
There is the slightly fun aspect that the issue predates
the introduction of iio_device_claim_direct_mode() so when this gets
backported we could get failures.  However, I think the oldest kernel
anyone is bothering with in the stable series is 4.9 and that function
was introduced earlier the same year as that kernel so we are probably fine :)

I'm going to pick the easy ones up in this series, though note that these
are 'probably' now being queued up for next cycle.  Linus made some noises
about a possible rc8 though so I might do an extra pull request if that looks
likely - then these might make it in rather sooner.

Jonathan



> 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad_sigma_delta.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > index 261a9a6b45e1..d8570f620785 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
> >  	unsigned int data_reg;
> >  	int ret = 0;
> >  
> > -	if (iio_buffer_enabled(indio_dev))
> > -		return -EBUSY;
> > +	ret = iio_device_claim_direct_mode(indio_dev);
> > +	if (ret)
> > +		return ret;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> >  	ad_sigma_delta_set_channel(sigma_delta, chan->address);
> >  
> >  	spi_bus_lock(sigma_delta->spi->master);
> > @@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
> >  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> >  	sigma_delta->bus_locked = false;
> >  	spi_bus_unlock(sigma_delta->spi->master);
> > -	mutex_unlock(&indio_dev->mlock);
> > +	iio_device_release_direct_mode(indio_dev);
> >  
> >  	if (ret)
> >  		return ret;  
> 
> 
> Thanks,
> Miquèl


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/15] iio: adc: ad799x: do not use internal iio_dev lock

[Nuno Sá]

'mlock' was being grabbed when setting the device frequency. In order to
not introduce any functional change a new lock is added. With that in
mind, the lock also needs to be grabbed in the places where 'mlock' is.

On the other places the lock was being used, we can just drop
it since we are only doing one i2c bus read/write which is already
safe.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ad799x.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 262bd7665b33..838ba8e77de1 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -28,6 +28,7 @@
 #include <linux/types.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/bitops.h>
 
 #include <linux/iio/iio.h>
@@ -125,6 +126,8 @@ struct ad799x_state {
 	const struct ad799x_chip_config	*chip_config;
 	struct regulator		*reg;
 	struct regulator		*vref;
+	/* lock to protect against multiple access to the device */
+	struct mutex			lock;
 	unsigned			id;
 	u16				config;
 
@@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev *indio_dev,
 		ret = iio_device_claim_direct_mode(indio_dev);
 		if (ret)
 			return ret;
+		mutex_lock(&st->lock);
 		ret = ad799x_scan_direct(st, chan->scan_index);
+		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
@@ -351,7 +356,8 @@ static ssize_t ad799x_write_frequency(struct device *dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
+
 	ret = i2c_smbus_read_byte_data(st->client, AD7998_CYCLE_TMR_REG);
 	if (ret < 0)
 		goto error_ret_mutex;
@@ -373,7 +379,7 @@ static ssize_t ad799x_write_frequency(struct device *dev,
 	ret = len;
 
 error_ret_mutex:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 
 	return ret;
 }
@@ -407,6 +413,8 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&st->lock);
+
 	if (state)
 		st->config |= BIT(chan->scan_index) << AD799X_CHANNEL_SHIFT;
 	else
@@ -418,6 +426,7 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
 		st->config &= ~AD7998_ALERT_EN;
 
 	ret = ad799x_write_config(st, st->config);
+	mutex_unlock(&st->lock);
 	iio_device_release_direct_mode(indio_dev);
 	return ret;
 }
@@ -454,11 +463,9 @@ static int ad799x_write_event_value(struct iio_dev *indio_dev,
 	if (val < 0 || val > GENMASK(chan->scan_type.realbits - 1, 0))
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
 	ret = i2c_smbus_write_word_swapped(st->client,
 		ad799x_threshold_reg(chan, dir, info),
 		val << chan->scan_type.shift);
-	mutex_unlock(&indio_dev->mlock);
 
 	return ret;
 }
@@ -473,10 +480,8 @@ static int ad799x_read_event_value(struct iio_dev *indio_dev,
 	int ret;
 	struct ad799x_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	ret = i2c_smbus_read_word_swapped(st->client,
 		ad799x_threshold_reg(chan, dir, info));
-	mutex_unlock(&indio_dev->mlock);
 	if (ret < 0)
 		return ret;
 	*val = (ret >> chan->scan_type.shift) &
@@ -785,6 +790,7 @@ static int ad799x_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+	mutex_init(&st->lock);
 	/* this is only used for device removal purposes */
 	i2c_set_clientdata(client, indio_dev);
 
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/15] iio: adc: ad799x: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:08 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> 'mlock' was being grabbed when setting the device frequency. In order to
> not introduce any functional change a new lock is added. With that in
> mind, the lock also needs to be grabbed in the places where 'mlock' is.

The usage in here is an example of why we originally decided to take mlock
private...  Annoying hard to reason about.  One key thing this description
doesn't mention is protection of st->config vs device state and I think
the original usage of mlock is partly intended to protect that.

Upshot is I'm not confident enough on this one to be happy taking it without
more head scratching or some review from others!

> 
> On the other places the lock was being used, we can just drop
> it since we are only doing one i2c bus read/write which is already
> safe.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

> ---
>  drivers/iio/adc/ad799x.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 262bd7665b33..838ba8e77de1 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -28,6 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/bitops.h>
>  
>  #include <linux/iio/iio.h>
> @@ -125,6 +126,8 @@ struct ad799x_state {
>  	const struct ad799x_chip_config	*chip_config;
>  	struct regulator		*reg;
>  	struct regulator		*vref;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex			lock;
>  	unsigned			id;
>  	u16				config;
>  
> @@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev *indio_dev,
>  		ret = iio_device_claim_direct_mode(indio_dev);
>  		if (ret)
>  			return ret;
> +		mutex_lock(&st->lock);

If we claim direct mode for the frequency writing we'll avoid racing with
buffers being enabled or other sysfs accesses that are claiming direct mode.

That made me think we could drop the lock, but the argument gets tricker
around st->config which is used in ad799x_scan_direct() and modified
in write_event_config() in a fashion that means it could be out of sync.
I'm not sure that matters but it is getting hard to reason about.


>  		ret = ad799x_scan_direct(st, chan->scan_index);
> +		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
>  		if (ret < 0)
> @@ -351,7 +356,8 @@ static ssize_t ad799x_write_frequency(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
> +
>  	ret = i2c_smbus_read_byte_data(st->client, AD7998_CYCLE_TMR_REG);
>  	if (ret < 0)
>  		goto error_ret_mutex;
> @@ -373,7 +379,7 @@ static ssize_t ad799x_write_frequency(struct device *dev,
>  	ret = len;
>  
>  error_ret_mutex:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  
>  	return ret;
>  }
> @@ -407,6 +413,8 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> +	mutex_lock(&st->lock);
> +
I think you do need the lock here and in other places where you want to ensure the
st->config state matches that of the device.

>  	if (state)
>  		st->config |= BIT(chan->scan_index) << AD799X_CHANNEL_SHIFT;
>  	else
> @@ -418,6 +426,7 @@ static int ad799x_write_event_config(struct iio_dev *indio_dev,
>  		st->config &= ~AD7998_ALERT_EN;
>  
>  	ret = ad799x_write_config(st, st->config);
> +	mutex_unlock(&st->lock);
>  	iio_device_release_direct_mode(indio_dev);
>  	return ret;
>  }
> @@ -454,11 +463,9 @@ static int ad799x_write_event_value(struct iio_dev *indio_dev,
>  	if (val < 0 || val > GENMASK(chan->scan_type.realbits - 1, 0))
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
>  	ret = i2c_smbus_write_word_swapped(st->client,
>  		ad799x_threshold_reg(chan, dir, info),
>  		val << chan->scan_type.shift);
> -	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret;
>  }
> @@ -473,10 +480,8 @@ static int ad799x_read_event_value(struct iio_dev *indio_dev,
>  	int ret;
>  	struct ad799x_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
>  	ret = i2c_smbus_read_word_swapped(st->client,
>  		ad799x_threshold_reg(chan, dir, info));
> -	mutex_unlock(&indio_dev->mlock);
>  	if (ret < 0)
>  		return ret;
>  	*val = (ret >> chan->scan_type.shift) &
> @@ -785,6 +790,7 @@ static int ad799x_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	mutex_init(&st->lock);
>  	/* this is only used for device removal purposes */
>  	i2c_set_clientdata(client, indio_dev);
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/15] iio: adc: ad799x: do not use internal iio_dev lock

[Nuno Sá]

On Sat, 2022-09-24 at 15:45 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 13:28:08 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > 'mlock' was being grabbed when setting the device frequency. In
> > order to
> > not introduce any functional change a new lock is added. With that
> > in
> > mind, the lock also needs to be grabbed in the places where 'mlock'
> > is.
> 
> The usage in here is an example of why we originally decided to take
> mlock
> private...  Annoying hard to reason about.  One key thing this
> description
> doesn't mention is protection of st->config vs device state and I
> think
> the original usage of mlock is partly intended to protect that.
> 
> Upshot is I'm not confident enough on this one to be happy taking it
> without
> more head scratching or some review from others!
> 

Yeah, this one is odd enough...

> > 
> > On the other places the lock was being used, we can just drop
> > it since we are only doing one i2c bus read/write which is already
> > safe.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> > ---
> >  drivers/iio/adc/ad799x.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> > index 262bd7665b33..838ba8e77de1 100644
> > --- a/drivers/iio/adc/ad799x.c
> > +++ b/drivers/iio/adc/ad799x.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/types.h>
> >  #include <linux/err.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/bitops.h>
> >  
> >  #include <linux/iio/iio.h>
> > @@ -125,6 +126,8 @@ struct ad799x_state {
> >         const struct ad799x_chip_config *chip_config;
> >         struct regulator                *reg;
> >         struct regulator                *vref;
> > +       /* lock to protect against multiple access to the device */
> > +       struct mutex                    lock;
> >         unsigned                        id;
> >         u16                             config;
> >  
> > @@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev
> > *indio_dev,
> >                 ret = iio_device_claim_direct_mode(indio_dev);
> >                 if (ret)
> >                         return ret;
> > +               mutex_lock(&st->lock);
> 
> If we claim direct mode for the frequency writing we'll avoid racing
> with
> buffers being enabled or other sysfs accesses that are claiming
> direct mode.
> 

As you stated in some other patch, changing the frequency while
buffering is probably not a good idea (possible in some devices though)
but the main reason I haven't used the claim direct approach was
because it would change behavior and could, in theory, break some
userspace apps...

> That made me think we could drop the lock, but the argument gets
> tricker
> around st->config which is used in ad799x_scan_direct() and modified
> in write_event_config() in a fashion that means it could be out of
> sync.
> I'm not sure that matters but it is getting hard to reason about.
> 

The write_event_config() also could use some improvement... Note that
st->config is always written even if ad799x_write_config() fails (which
for some devices is possible). I know that for an i2c write to fail
that probably means we have bigger issues but that does not make it
correct :). We should only update the variable after doing the actual
configuration...

- Nuno Sá
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/axp288_adc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
index 580361bd9849..3bbb96156739 100644
--- a/drivers/iio/adc/axp288_adc.c
+++ b/drivers/iio/adc/axp288_adc.c
@@ -9,6 +9,7 @@
 
 #include <linux/dmi.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
 #include <linux/regmap.h>
@@ -50,6 +51,8 @@ enum axp288_adc_id {
 struct axp288_adc_info {
 	int irq;
 	struct regmap *regmap;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	bool ts_enabled;
 };
 
@@ -161,7 +164,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	struct axp288_adc_info *info = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&info->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
@@ -178,7 +181,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 	default:
 		ret = -EINVAL;
 	}
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&info->lock);
 
 	return ret;
 }
@@ -264,6 +267,7 @@ static int axp288_adc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info = iio_priv(indio_dev);
+	mutex_init(&info->lock);
 	info->irq = platform_get_irq(pdev, 0);
 	if (info->irq < 0)
 		return info->irq;
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.

...

>         info = iio_priv(indio_dev);
> +       mutex_init(&info->lock);
>         info->irq = platform_get_irq(pdev, 0);
>         if (info->irq < 0)
>                 return info->irq;

Consider initializing it as late as possible, like after IRQ retrieval
in this context (maybe even deeper, but no context available). Ditto
for the rest of the series.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, September 20, 2022 3:13 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Jonathan Cameron <jic23@kernel.org>; Neil Armstrong
> <narmstrong@baylibre.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Jyoti Bhayana <jbhayana@google.com>;
> Chen-Yu Tsai <wens@csie.org>; Orson Zhai <orsonzhai@gmail.com>
> Subject: Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev
> lock
> 
> [External]
> 
> On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > The iio_device lock is only meant for internal use. Hence define a
> > device local lock to protect against concurrent accesses.
> 
> ...
> 
> >         info = iio_priv(indio_dev);
> > +       mutex_init(&info->lock);
> >         info->irq = platform_get_irq(pdev, 0);
> >         if (info->irq < 0)
> >                 return info->irq;
> 
> Consider initializing it as late as possible, like after IRQ retrieval
> in this context (maybe even deeper, but no context available). Ditto
> for the rest of the series.

Any special reason for it (maybe related to lockdep :wondering: ) ? Just
curious as I never noticed such a pattern when initializing mutexes.

- Nuno Sá
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > >         info = iio_priv(indio_dev);
> > > +       mutex_init(&info->lock);
> > >         info->irq = platform_get_irq(pdev, 0);
> > >         if (info->irq < 0)
> > >                 return info->irq;
> >
> > Consider initializing it as late as possible, like after IRQ retrieval
> > in this context (maybe even deeper, but no context available). Ditto
> > for the rest of the series.
>
> Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> curious as I never noticed such a pattern when initializing mutexes.

Yes. Micro-optimization based on the rule "don't create a resource in
case of known error".

OTOH, you have to be sure that the mutex (and generally speaking a
locking) should be initialized early enough.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > >         info = iio_priv(indio_dev);
> > > > +       mutex_init(&info->lock);
> > > >         info->irq = platform_get_irq(pdev, 0);
> > > >         if (info->irq < 0)
> > > >                 return info->irq;
> > >
> > > Consider initializing it as late as possible, like after IRQ retrieval
> > > in this context (maybe even deeper, but no context available). Ditto
> > > for the rest of the series.
> >
> > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > curious as I never noticed such a pattern when initializing mutexes.
>
> Yes. Micro-optimization based on the rule "don't create a resource in
> case of known error".
>
> OTOH, you have to be sure that the mutex (and generally speaking a
> locking) should be initialized early enough.

Note that "micro" in the above can be multiplied by 'k', where 'k' is
the amount of deferred probes (probably not the case here, but again,
"generally speaking").

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, September 20, 2022 3:39 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Jonathan Cameron <jic23@kernel.org>; Neil Armstrong
> <narmstrong@baylibre.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Jyoti Bhayana <jbhayana@google.com>;
> Chen-Yu Tsai <wens@csie.org>; Orson Zhai <orsonzhai@gmail.com>
> Subject: Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev
> lock
> 
> [External]
> 
> On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com>
> wrote:
> 
> ...
> 
> > > > >         info = iio_priv(indio_dev);
> > > > > +       mutex_init(&info->lock);
> > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > >         if (info->irq < 0)
> > > > >                 return info->irq;
> > > >
> > > > Consider initializing it as late as possible, like after IRQ retrieval
> > > > in this context (maybe even deeper, but no context available). Ditto
> > > > for the rest of the series.
> > >
> > > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > > curious as I never noticed such a pattern when initializing mutexes.
> >
> > Yes. Micro-optimization based on the rule "don't create a resource in
> > case of known error".
> >
> > OTOH, you have to be sure that the mutex (and generally speaking a
> > locking) should be initialized early enough.
> 

Typically not really needed during probe...

> Note that "micro" in the above can be multiplied by 'k', where 'k' is
> the amount of deferred probes (probably not the case here, but again,
> "generally speaking").
> 

Well, I don't think 'mutex_init()' does that much that really matters in
these patches but ok, that rule is indeed a good practice that sometimes
makes a difference. And since I will definitely need a v2, I can make that
change. Where applicable, the best place is probably before registering the
IIO device...

- Nuno Sá
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Tuesday, September 20, 2022 3:39 PM
> > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com>
> > wrote:
> >
> > ...
> >
> > > > > >         info = iio_priv(indio_dev);
> > > > > > +       mutex_init(&info->lock);
> > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > >         if (info->irq < 0)
> > > > > >                 return info->irq;
> > > > >
> > > > > Consider initializing it as late as possible, like after IRQ retrieval
> > > > > in this context (maybe even deeper, but no context available). Ditto
> > > > > for the rest of the series.
> > > >
> > > > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > > > curious as I never noticed such a pattern when initializing mutexes.
> > >
> > > Yes. Micro-optimization based on the rule "don't create a resource in
> > > case of known error".
> > >
> > > OTOH, you have to be sure that the mutex (and generally speaking a
> > > locking) should be initialized early enough.
>
> Typically not really needed during probe...

Actually as long as you expose the ABI to the user anything can
happen, means that your code should be ready to receive the requests
in any possible callbacks / file ops. Same applies to the IRQ handler.
So it's very important to initialize locking just in time. Hence I can
say that "typically it needs to be carefully placed".

> > Note that "micro" in the above can be multiplied by 'k', where 'k' is
> > the amount of deferred probes (probably not the case here, but again,
> > "generally speaking").
>
> Well, I don't think 'mutex_init()' does that much that really matters in
> these patches but ok, that rule is indeed a good practice that sometimes
> makes a difference. And since I will definitely need a v2, I can make that
> change. Where applicable, the best place is probably before registering the
> IIO device...

Some drivers are pedantic and want to have mutex_destroy() to be
called, it also reduces the surface of returning with an undestroyed
object (let's not discuss the usefulness of such destruction, but in
principle).

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Nuno Sá]

On Tue, 2022-09-20 at 18:12 +0300, Andy Shevchenko wrote:
> On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Tuesday, September 20, 2022 3:39 PM
> > > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com>
> > > > wrote:
> > > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá
> > > > > > <nuno.sa@analog.com>
> > > wrote:
> > > 
> > > ...
> > > 
> > > > > > >         info = iio_priv(indio_dev);
> > > > > > > +       mutex_init(&info->lock);
> > > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > > >         if (info->irq < 0)
> > > > > > >                 return info->irq;
> > > > > > 
> > > > > > Consider initializing it as late as possible, like after
> > > > > > IRQ retrieval
> > > > > > in this context (maybe even deeper, but no context
> > > > > > available). Ditto
> > > > > > for the rest of the series.
> > > > > 
> > > > > Any special reason for it (maybe related to lockdep
> > > > > :wondering: ) ? Just
> > > > > curious as I never noticed such a pattern when initializing
> > > > > mutexes.
> > > > 
> > > > Yes. Micro-optimization based on the rule "don't create a
> > > > resource in
> > > > case of known error".
> > > > 
> > > > OTOH, you have to be sure that the mutex (and generally
> > > > speaking a
> > > > locking) should be initialized early enough.
> > 
> > Typically not really needed during probe...
> 
> Actually as long as you expose the ABI to the user anything can
> happen, means that your code should be ready to receive the requests
> in any possible callbacks / file ops. Same applies to the IRQ
> handler.
> So it's very important to initialize locking just in time. Hence I
> can
> say that "typically it needs to be carefully placed".
> 

Yes, I'm aware of that... For some reason I just thought about code
paths directly on probe. Anyways, hopefully these drivers mostly do the
right thing and register the IIO device as late as possible (ideally
the last thing to be done). The same goes for IRQs and for IIO, when
used as part of triggered buffering, the lock is often only used in the
trigger handler which means it's only reachable after the ABI is
exposed... 

- Nuno Sá
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Wed, 21 Sep 2022 11:07:50 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-09-20 at 18:12 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:  
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Tuesday, September 20, 2022 3:39 PM
> > > > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:  
> > > > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com>
> > > > > wrote:  
> > > > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá
> > > > > > > <nuno.sa@analog.com>  
> > > > wrote:
> > > > 
> > > > ...
> > > >   
> > > > > > > >         info = iio_priv(indio_dev);
> > > > > > > > +       mutex_init(&info->lock);
> > > > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > > > >         if (info->irq < 0)
> > > > > > > >                 return info->irq;  
> > > > > > > 
> > > > > > > Consider initializing it as late as possible, like after
> > > > > > > IRQ retrieval
> > > > > > > in this context (maybe even deeper, but no context
> > > > > > > available). Ditto
> > > > > > > for the rest of the series.  
> > > > > > 
> > > > > > Any special reason for it (maybe related to lockdep
> > > > > > :wondering: ) ? Just
> > > > > > curious as I never noticed such a pattern when initializing
> > > > > > mutexes.  
> > > > > 
> > > > > Yes. Micro-optimization based on the rule "don't create a
> > > > > resource in
> > > > > case of known error".
> > > > > 
> > > > > OTOH, you have to be sure that the mutex (and generally
> > > > > speaking a
> > > > > locking) should be initialized early enough.  
> > > 
> > > Typically not really needed during probe...  
> > 
> > Actually as long as you expose the ABI to the user anything can
> > happen, means that your code should be ready to receive the requests
> > in any possible callbacks / file ops. Same applies to the IRQ
> > handler.
> > So it's very important to initialize locking just in time. Hence I
> > can
> > say that "typically it needs to be carefully placed".
> >   
> 
> Yes, I'm aware of that... For some reason I just thought about code
> paths directly on probe. Anyways, hopefully these drivers mostly do the
> right thing and register the IIO device as late as possible (ideally
> the last thing to be done). The same goes for IRQs and for IIO, when
> used as part of triggered buffering, the lock is often only used in the
> trigger handler which means it's only reachable after the ABI is
> exposed... 

Can't say I feel that strongly about a mutex_init() placement, but
no harm in moving them later - indeed before the iio_device_register()
should be correct - though care needed as might be some unnecessary locks
taken in probe because of code sharing (and them previously being harmless)

Jonathan
> 
> - Nuno Sá
> > >   


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:09 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Looks good. Not sure how we missed cleaning this one up earlier
given how simple it is!  Anyhow, given Andy's feedback I'll wait for v2
to pick this up.

> ---
>  drivers/iio/adc/axp288_adc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 580361bd9849..3bbb96156739 100644
> --- a/drivers/iio/adc/axp288_adc.c
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/dmi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/kernel.h>
>  #include <linux/device.h>
>  #include <linux/regmap.h>
> @@ -50,6 +51,8 @@ enum axp288_adc_id {
>  struct axp288_adc_info {
>  	int irq;
>  	struct regmap *regmap;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
>  	bool ts_enabled;
>  };
>  
> @@ -161,7 +164,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  	struct axp288_adc_info *info = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&info->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
> @@ -178,7 +181,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  	default:
>  		ret = -EINVAL;
>  	}
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&info->lock);
>  
>  	return ret;
>  }
> @@ -264,6 +267,7 @@ static int axp288_adc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	info = iio_priv(indio_dev);
> +	mutex_init(&info->lock);
>  	info->irq = platform_get_irq(pdev, 0);
>  	if (info->irq < 0)
>  		return info->irq;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/15] iio: adc: imx7d_adc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/imx7d_adc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 86caff1d006b..fa19f5e09079 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 
@@ -108,7 +109,8 @@ struct imx7d_adc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *clk;
-
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	u32 vref_uv;
 	u32 value;
 	u32 channel;
@@ -293,7 +295,7 @@ static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&info->lock);
 		reinit_completion(&info->completion);
 
 		channel = chan->channel & 0x03;
@@ -303,16 +305,16 @@ static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
 		ret = wait_for_completion_interruptible_timeout
 				(&info->completion, IMX7D_ADC_TIMEOUT);
 		if (ret == 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
 			return -ETIMEDOUT;
 		}
 		if (ret < 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
 			return ret;
 		}
 
 		*val = info->value;
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&info->lock);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -487,6 +489,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 
 	info = iio_priv(indio_dev);
 	info->dev = dev;
+	mutex_init(&info->lock);
 
 	info->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(info->regs))
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/15] iio: adc: imx7d_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:10 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.

Mention the mutex.h include.  Technically it's an unrelated (good) change.
Definitely not worth a separate patch however so just mention it in the
description.

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/imx7d_adc.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 86caff1d006b..fa19f5e09079 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -108,7 +109,8 @@ struct imx7d_adc {
>  	struct device *dev;
>  	void __iomem *regs;
>  	struct clk *clk;
> -
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
>  	u32 vref_uv;
>  	u32 value;
>  	u32 channel;
> @@ -293,7 +295,7 @@ static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&info->lock);
>  		reinit_completion(&info->completion);
>  
>  		channel = chan->channel & 0x03;
> @@ -303,16 +305,16 @@ static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
>  		ret = wait_for_completion_interruptible_timeout
>  				(&info->completion, IMX7D_ADC_TIMEOUT);
>  		if (ret == 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&info->lock);
>  			return -ETIMEDOUT;
>  		}
>  		if (ret < 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&info->lock);
>  			return ret;
>  		}
>  
>  		*val = info->value;
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&info->lock);
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> @@ -487,6 +489,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  
>  	info = iio_priv(indio_dev);
>  	info->dev = dev;
> +	mutex_init(&info->lock);
>  
>  	info->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(info->regs))


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/15] iio: adc: lpc32xx_adc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/lpc32xx_adc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
index b56ce15255cf..4136cf536d58 100644
--- a/drivers/iio/adc/lpc32xx_adc.c
+++ b/drivers/iio/adc/lpc32xx_adc.c
@@ -15,6 +15,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 
@@ -49,6 +50,8 @@ struct lpc32xx_adc_state {
 	struct clk *clk;
 	struct completion completion;
 	struct regulator *vref;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 
 	u32 value;
 };
@@ -64,10 +67,10 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&st->lock);
 		ret = clk_prepare_enable(st->clk);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&st->lock);
 			return ret;
 		}
 		/* Measurement setup */
@@ -80,7 +83,7 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
 		wait_for_completion(&st->completion); /* set by ISR */
 		clk_disable_unprepare(st->clk);
 		*val = st->value;
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&st->lock);
 
 		return IIO_VAL_INT;
 
@@ -158,6 +161,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	st = iio_priv(iodev);
+	mutex_init(&st->lock);
 
 	st->adc_base = devm_ioremap(&pdev->dev, res->start,
 				    resource_size(res));
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/15] iio: adc: lpc32xx_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:11 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.

As with previous patch, mention the additional include cleanup.

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/lpc32xx_adc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index b56ce15255cf..4136cf536d58 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -15,6 +15,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -49,6 +50,8 @@ struct lpc32xx_adc_state {
>  	struct clk *clk;
>  	struct completion completion;
>  	struct regulator *vref;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
>  
>  	u32 value;
>  };
> @@ -64,10 +67,10 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&st->lock);
>  		ret = clk_prepare_enable(st->clk);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&st->lock);
>  			return ret;
>  		}
>  		/* Measurement setup */
> @@ -80,7 +83,7 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>  		wait_for_completion(&st->completion); /* set by ISR */
>  		clk_disable_unprepare(st->clk);
>  		*val = st->value;
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&st->lock);
>  
>  		return IIO_VAL_INT;
>  
> @@ -158,6 +161,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	st = iio_priv(iodev);
> +	mutex_init(&st->lock);
>  
>  	st->adc_base = devm_ioremap(&pdev->dev, res->start,
>  				    resource_size(res));


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/15] iio: adc: ltc2947-core: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ltc2497-core.c | 7 +++++--
 drivers/iio/adc/ltc2497.h      | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
index f52d37af4d1f..00536501dc52 100644
--- a/drivers/iio/adc/ltc2497-core.c
+++ b/drivers/iio/adc/ltc2497-core.c
@@ -10,6 +10,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/regulator/consumer.h>
 
 #include "ltc2497.h"
@@ -81,9 +82,9 @@ static int ltc2497core_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&ddata->lock);
 		ret = ltc2497core_read(ddata, chan->address, val);
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&ddata->lock);
 		if (ret < 0)
 			return ret;
 
@@ -169,6 +170,8 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
 	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
 	int ret;
 
+	mutex_init(&ddata->lock);
+
 	/*
 	 * Keep using dev_name() for the iio_dev's name on some of the parts,
 	 * since updating it would result in a ABI breakage.
diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
index e023de0d88c4..781519b52475 100644
--- a/drivers/iio/adc/ltc2497.h
+++ b/drivers/iio/adc/ltc2497.h
@@ -12,6 +12,8 @@ struct ltc2497_chip_info {
 struct ltc2497core_driverdata {
 	struct regulator *ref;
 	ktime_t	time_prev;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	const struct ltc2497_chip_info	*chip_info;
 	u8 addr_prev;
 	int (*result_and_measure)(struct ltc2497core_driverdata *ddata,
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/15] iio: adc: meson_saradc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/meson_saradc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 1a68b099d323..45d6e3d4b0ce 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/iio/iio.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
@@ -276,6 +277,8 @@ struct meson_sar_adc_priv {
 	struct clk				*adc_div_clk;
 	struct clk_divider			clk_div;
 	struct completion			done;
+	/* lock to protect against multiple access to the device */
+	struct mutex				lock;
 	int					calibbias;
 	int					calibscale;
 	struct regmap				*tsc_regmap;
@@ -486,7 +489,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev)
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
 	int val, ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&priv->lock);
 
 	if (priv->param->has_bl30_integration) {
 		/* prevent BL30 from using the SAR ADC while we are using it */
@@ -504,7 +507,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev)
 						      !(val & MESON_SAR_ADC_DELAY_BL30_BUSY),
 						      1, 10000);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&priv->lock);
 			return ret;
 		}
 	}
@@ -521,7 +524,7 @@ static void meson_sar_adc_unlock(struct iio_dev *indio_dev)
 		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELAY,
 				   MESON_SAR_ADC_DELAY_KERNEL_BUSY, 0);
 
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&priv->lock);
 }
 
 static void meson_sar_adc_clear_fifo(struct iio_dev *indio_dev)
@@ -1174,6 +1177,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 
 	priv = iio_priv(indio_dev);
 	init_completion(&priv->done);
+	mutex_init(&priv->lock);
 
 	match_data = of_device_get_match_data(dev);
 	if (!match_data)
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/15] iio: adc: meson_saradc: do not use internal iio_dev lock

[Martin Blumenstingl]

On Tue, Sep 20, 2022 at 1:28 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thank you for this patch!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/15] iio: adc: rockchip_saradc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/rockchip_saradc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index b87ea7148b58..0681226536c9 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -49,6 +50,8 @@ struct rockchip_saradc {
 	struct clk		*clk;
 	struct completion	completion;
 	struct regulator	*vref;
+	/* lock to protect against multiple access to the device */
+	struct mutex		lock;
 	int			uv_vref;
 	struct reset_control	*reset;
 	const struct rockchip_saradc_data *data;
@@ -94,17 +97,17 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&info->lock);
 
 		ret = rockchip_saradc_conversion(info, chan);
 		if (ret) {
 			rockchip_saradc_power_down(info);
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
 			return ret;
 		}
 
 		*val = info->last_val;
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&info->lock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = info->uv_vref / 1000;
@@ -270,7 +273,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
 	int ret;
 	int i, j = 0;
 
-	mutex_lock(&i_dev->mlock);
+	mutex_lock(&info->lock);
 
 	for_each_set_bit(i, i_dev->active_scan_mask, i_dev->masklength) {
 		const struct iio_chan_spec *chan = &i_dev->channels[i];
@@ -287,7 +290,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
 
 	iio_push_to_buffers_with_timestamp(i_dev, &data, iio_get_time_ns(i_dev));
 out:
-	mutex_unlock(&i_dev->mlock);
+	mutex_unlock(&info->lock);
 
 	iio_trigger_notify_done(i_dev->trig);
 
@@ -332,6 +335,7 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 	info = iio_priv(indio_dev);
+	mutex_init(&info->lock);
 
 	match = of_match_device(rockchip_saradc_match, &pdev->dev);
 	if (!match) {
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/15] iio: adc: sc27xx_adc: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/sc27xx_adc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index f8421cbba8fa..1cdb4e83aaf3 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -4,6 +4,7 @@
 #include <linux/hwspinlock.h>
 #include <linux/iio/iio.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -83,6 +84,8 @@ struct sc27xx_adc_data {
 	struct device *dev;
 	struct regulator *volref;
 	struct regmap *regmap;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	/*
 	 * One hardware spinlock to synchronize between the multiple
 	 * subsystems which will access the unique ADC controller.
@@ -664,9 +667,9 @@ static int sc27xx_adc_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&data->lock);
 		ret = sc27xx_adc_read(data, chan->channel, scale, &tmp);
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&data->lock);
 
 		if (ret)
 			return ret;
@@ -675,10 +678,10 @@ static int sc27xx_adc_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&data->lock);
 		ret = sc27xx_adc_read_processed(data, chan->channel, scale,
 						&tmp);
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&data->lock);
 
 		if (ret)
 			return ret;
@@ -876,6 +879,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	sc27xx_data = iio_priv(indio_dev);
+	mutex_init(&sc27xx_data->lock);
 
 	sc27xx_data->regmap = dev_get_regmap(dev->parent, NULL);
 	if (!sc27xx_data->regmap) {
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/15] iio: adc: vf610_adc: do not use internal iio_dev lock

[Nuno Sá]

In order to drop the internal lock usage we needed two different things:

1) The first place where 'mlock' was being used was a typical case where
iio_device_claim_direct_mode() fits perfectly.
2) In the second case, it was being used to prevent concurrent accesses
to the device and shared data but nothing was being enforced with
regards to buffering (i.e, there was nothing preventing from changing
the conversion mode while buffering). Hence, in this case, a new lock
was introduced in the state structure.

Note that the goal is not to introduce any functional change and that is
the reason why a new lock was introduced to guarantee 2).

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/vf610_adc.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index c6b16cf6e367..b9addd313ef1 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -7,6 +7,7 @@
 
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
@@ -156,6 +157,9 @@ struct vf610_adc {
 	void __iomem *regs;
 	struct clk *clk;
 
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
+
 	u32 vref_uv;
 	u32 value;
 	struct regulator *vref;
@@ -467,11 +471,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev,
 {
 	struct vf610_adc *info = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&info->lock);
 	info->adc_feature.conv_mode = mode;
 	vf610_adc_calculate_rates(info);
 	vf610_adc_hw_init(info);
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&info->lock);
 
 	return 0;
 }
@@ -635,12 +639,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 	case IIO_CHAN_INFO_PROCESSED:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev)) {
-			mutex_unlock(&indio_dev->mlock);
-			return -EBUSY;
-		}
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
 
+		mutex_lock(&info->lock);
 		reinit_completion(&info->completion);
 		hc_cfg = VF610_ADC_ADCHC(chan->channel);
 		hc_cfg |= VF610_ADC_AIEN;
@@ -648,11 +651,13 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
 		ret = wait_for_completion_interruptible_timeout
 				(&info->completion, VF610_ADC_TIMEOUT);
 		if (ret == 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
+			iio_device_release_direct_mode(indio_dev);
 			return -ETIMEDOUT;
 		}
 		if (ret < 0) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
+			iio_device_release_direct_mode(indio_dev);
 			return ret;
 		}
 
@@ -671,11 +676,13 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
 
 			break;
 		default:
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&info->lock);
+			iio_device_release_direct_mode(indio_dev);
 			return -EINVAL;
 		}
 
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&info->lock);
+		iio_device_release_direct_mode(indio_dev);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -812,6 +819,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
 
 	info = iio_priv(indio_dev);
 	info->dev = &pdev->dev;
+	mutex_init(&info->lock);
 
 	info->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(info->regs))
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/15] iio: adc: vf610_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:16 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> In order to drop the internal lock usage we needed two different things:
> 
> 1) The first place where 'mlock' was being used was a typical case where
> iio_device_claim_direct_mode() fits perfectly.
> 2) In the second case, it was being used to prevent concurrent accesses
> to the device and shared data but nothing was being enforced with
> regards to buffering (i.e, there was nothing preventing from changing
> the conversion mode while buffering). Hence, in this case, a new lock
> was introduced in the state structure.
> 
> Note that the goal is not to introduce any functional change and that is
> the reason why a new lock was introduced to guarantee 2).

Hmm. If we can get enough review on this, I'd rather claim_direct_mode()
for part 2 even though it is a functional change.  It's a bit tricky
as that function has no error paths currently so no real reason the 
caller should be expecting errors (generic code will be fine, but generic
code doesn't know about this ABI element anyway).

A request for a refactor precursor patch in line.  It will make
the end result cleaner I think.


> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/vf610_adc.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index c6b16cf6e367..b9addd313ef1 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> @@ -156,6 +157,9 @@ struct vf610_adc {
>  	void __iomem *regs;
>  	struct clk *clk;
>  
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
> +
>  	u32 vref_uv;
>  	u32 value;
>  	struct regulator *vref;
> @@ -467,11 +471,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev,
>  {
>  	struct vf610_adc *info = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&info->lock);
>  	info->adc_feature.conv_mode = mode;
>  	vf610_adc_calculate_rates(info);
>  	vf610_adc_hw_init(info);
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&info->lock);
>  
>  	return 0;
>  }
> @@ -635,12 +639,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  	case IIO_CHAN_INFO_PROCESSED:
> -		mutex_lock(&indio_dev->mlock);
> -		if (iio_buffer_enabled(indio_dev)) {
> -			mutex_unlock(&indio_dev->mlock);
> -			return -EBUSY;
> -		}
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  
> +		mutex_lock(&info->lock);
>  		reinit_completion(&info->completion);
>  		hc_cfg = VF610_ADC_ADCHC(chan->channel);
>  		hc_cfg |= VF610_ADC_AIEN;
> @@ -648,11 +651,13 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>  		ret = wait_for_completion_interruptible_timeout
>  				(&info->completion, VF610_ADC_TIMEOUT);
>  		if (ret == 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&info->lock);

The error paths in here are complex. I'd pull the whole of the
case block out to a new function in a precursor patch and
then you can add the new locking with a goto unlock; pattern.

> +			iio_device_release_direct_mode(indio_dev);
>  			return -ETIMEDOUT;
>  		}
>  		if (ret < 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&info->lock);
> +			iio_device_release_direct_mode(indio_dev);
>  			return ret;
>  		}
>  
> @@ -671,11 +676,13 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>  
>  			break;
>  		default:
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&info->lock);
> +			iio_device_release_direct_mode(indio_dev);
>  			return -EINVAL;
>  		}
>  
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&info->lock);
> +		iio_device_release_direct_mode(indio_dev);
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> @@ -812,6 +819,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>  
>  	info = iio_priv(indio_dev);
>  	info->dev = &pdev->dev;
> +	mutex_init(&info->lock);
>  
>  	info->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(info->regs))


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/15] iio: common: scmi_iio: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/common/scmi_sensors/scmi_iio.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 54ccf19ab2bb..d92f7f651f7b 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/kthread.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/scmi_protocol.h>
 #include <linux/time.h>
 #include <linux/types.h>
@@ -27,6 +28,8 @@ struct scmi_iio_priv {
 	struct scmi_protocol_handle *ph;
 	const struct scmi_sensor_info *sensor_info;
 	struct iio_dev *indio_dev;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	/* adding one additional channel for timestamp */
 	s64 iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
 	struct notifier_block sensor_update_nb;
@@ -198,13 +201,14 @@ static int scmi_iio_write_raw(struct iio_dev *iio_dev,
 			      struct iio_chan_spec const *chan, int val,
 			      int val2, long mask)
 {
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
 	int err;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		mutex_lock(&iio_dev->mlock);
+		mutex_lock(&sensor->lock);
 		err = scmi_iio_set_odr_val(iio_dev, val, val2);
-		mutex_unlock(&iio_dev->mlock);
+		mutex_unlock(&sensor->lock);
 		return err;
 	default:
 		return -EINVAL;
@@ -586,6 +590,7 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
 	sensor->sensor_info = sensor_info;
 	sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb;
 	sensor->indio_dev = iiodev;
+	mutex_init(&sensor->lock);
 
 	/* adding one additional channel for timestamp */
 	iiodev->num_channels = sensor_info->num_axis + 1;
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 11/15] iio: common: scmi_iio: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:17 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Ouch - this driver is fairly new and I missed that it was using mlock.
I guess this is a good example of why we need to finishing hiding it away!

Patch looks fine to me - I'll pick up in v2.

Jonathan

> ---
>  drivers/iio/common/scmi_sensors/scmi_iio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> index 54ccf19ab2bb..d92f7f651f7b 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/scmi_protocol.h>
>  #include <linux/time.h>
>  #include <linux/types.h>
> @@ -27,6 +28,8 @@ struct scmi_iio_priv {
>  	struct scmi_protocol_handle *ph;
>  	const struct scmi_sensor_info *sensor_info;
>  	struct iio_dev *indio_dev;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
>  	/* adding one additional channel for timestamp */
>  	s64 iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
>  	struct notifier_block sensor_update_nb;
> @@ -198,13 +201,14 @@ static int scmi_iio_write_raw(struct iio_dev *iio_dev,
>  			      struct iio_chan_spec const *chan, int val,
>  			      int val2, long mask)
>  {
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>  	int err;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		mutex_lock(&iio_dev->mlock);
> +		mutex_lock(&sensor->lock);
>  		err = scmi_iio_set_odr_val(iio_dev, val, val2);
> -		mutex_unlock(&iio_dev->mlock);
> +		mutex_unlock(&sensor->lock);
>  		return err;
>  	default:
>  		return -EINVAL;
> @@ -586,6 +590,7 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
>  	sensor->sensor_info = sensor_info;
>  	sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb;
>  	sensor->indio_dev = iiodev;
> +	mutex_init(&sensor->lock);
>  
>  	/* adding one additional channel for timestamp */
>  	iiodev->num_channels = sensor_info->num_axis + 1;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 12/15] iio: fyro: itg3200_core: do not use internal iio_dev lock

[Nuno Sá]

The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/gyro/itg3200_core.c  | 9 ++++++---
 include/linux/iio/gyro/itg3200.h | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
index 0491c64e1b32..358aa8ac0c6b 100644
--- a/drivers/iio/gyro/itg3200_core.c
+++ b/drivers/iio/gyro/itg3200_core.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/stat.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 
 #include <linux/iio/iio.h>
@@ -131,6 +132,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
 			     int val2,
 			     long mask)
 {
+	struct itg3200 *st = iio_priv(indio_dev);
 	int ret;
 	u8 t;
 
@@ -139,11 +141,11 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
 		if (val == 0 || val2 != 0)
 			return -EINVAL;
 
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&st->lock);
 
 		ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_DLPF, &t);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&st->lock);
 			return ret;
 		}
 		t = ((t & ITG3200_DLPF_CFG_MASK) ? 1000u : 8000u) / val - 1;
@@ -152,7 +154,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
 					  ITG3200_REG_SAMPLE_RATE_DIV,
 					  t);
 
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&st->lock);
 		return ret;
 
 	default:
@@ -307,6 +309,7 @@ static int itg3200_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+	mutex_init(&st->lock);
 
 	ret = iio_read_mount_matrix(&client->dev, &st->orientation);
 	if (ret)
diff --git a/include/linux/iio/gyro/itg3200.h b/include/linux/iio/gyro/itg3200.h
index a602fe7b84fa..74b6d1cadc86 100644
--- a/include/linux/iio/gyro/itg3200.h
+++ b/include/linux/iio/gyro/itg3200.h
@@ -102,6 +102,8 @@ struct itg3200 {
 	struct i2c_client	*i2c;
 	struct iio_trigger	*trig;
 	struct iio_mount_matrix orientation;
+	/* lock to protect against multiple access to the device */
+	struct mutex		lock;
 };
 
 enum ITG3200_SCAN_INDEX {
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 12/15] iio: fyro: itg3200_core: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:18 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/gyro/itg3200_core.c  | 9 ++++++---
>  include/linux/iio/gyro/itg3200.h | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> index 0491c64e1b32..358aa8ac0c6b 100644
> --- a/drivers/iio/gyro/itg3200_core.c
> +++ b/drivers/iio/gyro/itg3200_core.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/stat.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/delay.h>
>  
>  #include <linux/iio/iio.h>
> @@ -131,6 +132,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  			     int val2,
>  			     long mask)
>  {
> +	struct itg3200 *st = iio_priv(indio_dev);
>  	int ret;
>  	u8 t;
>  
> @@ -139,11 +141,11 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  		if (val == 0 || val2 != 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&st->lock);
>  
>  		ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_DLPF, &t);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&st->lock);
>  			return ret;
>  		}
>  		t = ((t & ITG3200_DLPF_CFG_MASK) ? 1000u : 8000u) / val - 1;
> @@ -152,7 +154,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  					  ITG3200_REG_SAMPLE_RATE_DIV,
>  					  t);
>  
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&st->lock);
>  		return ret;
>  
>  	default:
> @@ -307,6 +309,7 @@ static int itg3200_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	mutex_init(&st->lock);
>  
>  	ret = iio_read_mount_matrix(&client->dev, &st->orientation);
>  	if (ret)
> diff --git a/include/linux/iio/gyro/itg3200.h b/include/linux/iio/gyro/itg3200.h
> index a602fe7b84fa..74b6d1cadc86 100644
> --- a/include/linux/iio/gyro/itg3200.h
> +++ b/include/linux/iio/gyro/itg3200.h
> @@ -102,6 +102,8 @@ struct itg3200 {
>  	struct i2c_client	*i2c;
>  	struct iio_trigger	*trig;
>  	struct iio_mount_matrix orientation;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex		lock;
>  };
>  
>  enum ITG3200_SCAN_INDEX {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 12/15] iio: fyro: itg3200_core: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:18 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
As with earlier, add a note about mutex.h include.

Otherwise, looks fine.  My gut feeling is that we should
have a claim_direct_mode() in that write_raw() as well as
changing frequency whilst doing buffered capture is rarely a good
idea, but meh, we don't need to change the ABI so let us not do so.

Jonathan

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/gyro/itg3200_core.c  | 9 ++++++---
>  include/linux/iio/gyro/itg3200.h | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> index 0491c64e1b32..358aa8ac0c6b 100644
> --- a/drivers/iio/gyro/itg3200_core.c
> +++ b/drivers/iio/gyro/itg3200_core.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/stat.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/delay.h>
>  
>  #include <linux/iio/iio.h>
> @@ -131,6 +132,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  			     int val2,
>  			     long mask)
>  {
> +	struct itg3200 *st = iio_priv(indio_dev);
>  	int ret;
>  	u8 t;
>  
> @@ -139,11 +141,11 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  		if (val == 0 || val2 != 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&st->lock);
>  
>  		ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_DLPF, &t);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&st->lock);
>  			return ret;
>  		}
>  		t = ((t & ITG3200_DLPF_CFG_MASK) ? 1000u : 8000u) / val - 1;
> @@ -152,7 +154,7 @@ static int itg3200_write_raw(struct iio_dev *indio_dev,
>  					  ITG3200_REG_SAMPLE_RATE_DIV,
>  					  t);
>  
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&st->lock);
>  		return ret;
>  
>  	default:
> @@ -307,6 +309,7 @@ static int itg3200_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	mutex_init(&st->lock);
>  
>  	ret = iio_read_mount_matrix(&client->dev, &st->orientation);
>  	if (ret)
> diff --git a/include/linux/iio/gyro/itg3200.h b/include/linux/iio/gyro/itg3200.h
> index a602fe7b84fa..74b6d1cadc86 100644
> --- a/include/linux/iio/gyro/itg3200.h
> +++ b/include/linux/iio/gyro/itg3200.h
> @@ -102,6 +102,8 @@ struct itg3200 {
>  	struct i2c_client	*i2c;
>  	struct iio_trigger	*trig;
>  	struct iio_mount_matrix orientation;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex		lock;
>  };
>  
>  enum ITG3200_SCAN_INDEX {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

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á <nuno.sa@analog.com>
---
 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)) {
 			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 */
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Miquel Raynal]

Hi Nuno,

nuno.sa@analog.com 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á <nuno.sa@analog.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sa, Nuno]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Tuesday, September 20, 2022 2:23 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> <orsonzhai@gmail.com>
> Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> lock
> 
> [External]
> 
> Hi Nuno,
> 

Hi Miquel,

Thanks for reviewing...

> nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > ---
> >  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á
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Miquel Raynal]

Hi Nuno,

Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:

> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Tuesday, September 20, 2022 2:23 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > Goede <hdegoede@redhat.com>; Jerome Brunet <jbrunet@baylibre.com>;
> > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>; Andy
> > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > <orsonzhai@gmail.com>
> > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> > lock
> > 
> > [External]
> > 
> > Hi Nuno,
> >   
> 
> Hi Miquel,
> 
> Thanks for reviewing...
> 
> > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > ---
> > >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sa, Nuno]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Tuesday, September 20, 2022 2:56 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> <orsonzhai@gmail.com>
> Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> lock
> 
> [External]
> 
> Hi Nuno,
> 
> Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> 
> > > -----Original Message-----
> > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; linux-
> rockchip@lists.infradead.org;
> > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> Hennerich,
> > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> Ardelean
> > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;
> Andriy
> > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > > Goede <hdegoede@redhat.com>; Jerome Brunet
> <jbrunet@baylibre.com>;
> > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Andy
> > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > > <orsonzhai@gmail.com>
> > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal
> iio_dev
> > > lock
> > >
> > > [External]
> > >
> > > Hi Nuno,
> > >
> >
> > Hi Miquel,
> >
> > Thanks for reviewing...
> >
> > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > ---
> > > >  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á
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Miquel Raynal]

Hi Nuno,

Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:

> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Tuesday, September 20, 2022 2:56 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > Goede <hdegoede@redhat.com>; Jerome Brunet <jbrunet@baylibre.com>;
> > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>; Andy
> > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > <orsonzhai@gmail.com>
> > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev
> > lock
> > 
> > [External]
> > 
> > Hi Nuno,
> > 
> > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> >   
> > > > -----Original Message-----
> > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org; linux-  
> > rockchip@lists.infradead.org;  
> > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;  
> > Hennerich,  
> > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru  
> > Ardelean  
> > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;  
> > Andriy  
> > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > > > Goede <hdegoede@redhat.com>; Jerome Brunet  
> > <jbrunet@baylibre.com>;  
> > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;  
> > Andy  
> > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > > > <orsonzhai@gmail.com>
> > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use internal  
> > iio_dev  
> > > > lock
> > > >
> > > > [External]
> > > >
> > > > Hi Nuno,
> > > >  
> > >
> > > Hi Miquel,
> > >
> > > Thanks for reviewing...
> > >  
> > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > ---
> > > > >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:
> Hi Nuno,
> 
> Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> 
> > > -----Original Message-----
> > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-arm-kernel@lists.infradead.org;
> > > linux-rockchip@lists.infradead.org;
> > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> > > Hennerich,
> > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > Kevin
> > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> > > Ardelean
> > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;
> > > Andriy
> > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > <jbrunet@baylibre.com>;
> > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > > Andy
> > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > Baolin
> > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > > <orsonzhai@gmail.com>
> > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > internal iio_dev
> > > lock
> > > 
> > > [External]
> > > 
> > > Hi Nuno,
> > > 
> > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > >   
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-  
> > > rockchip@lists.infradead.org;  
> > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;  
> > > Hennerich,  
> > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > > > Kevin
> > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > <vz@mleia.com>;
> > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru  
> > > Ardelean  
> > > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; 
> > > Andriy  
> > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo
> > > > > Chen
> > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans
> > > > > de
> > > > > Goede <hdegoede@redhat.com>; Jerome Brunet  
> > > <jbrunet@baylibre.com>;  
> > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > <lars@metafoo.de>;  
> > > Andy  
> > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > > > Baolin
> > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson
> > > > > Zhai
> > > > > <orsonzhai@gmail.com>
> > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > internal  
> > > iio_dev  
> > > > > lock
> > > > > 
> > > > > [External]
> > > > > 
> > > > > Hi Nuno,
> > > > >  
> > > > 
> > > > Hi Miquel,
> > > > 
> > > > Thanks for reviewing...
> > > >  
> > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > ---
> > > > > >  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á
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Miquel Raynal]

Hi Nuno,

noname.nuno@gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:

> On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:
> > Hi Nuno,
> > 
> > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> >   
> > > > -----Original Message-----
> > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org;
> > > > linux-rockchip@lists.infradead.org;
> > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> > > > Hennerich,
> > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > > Kevin
> > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> > > > Ardelean
> > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;
> > > > Andriy
> > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > > <jbrunet@baylibre.com>;
> > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > > > Andy
> > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > > Baolin
> > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > > > <orsonzhai@gmail.com>
> > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > internal iio_dev
> > > > lock
> > > > 
> > > > [External]
> > > > 
> > > > Hi Nuno,
> > > > 
> > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-    
> > > > rockchip@lists.infradead.org;    
> > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;    
> > > > Hennerich,    
> > > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > > > > Kevin
> > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > <vz@mleia.com>;
> > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru    
> > > > Ardelean    
> > > > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;   
> > > > Andriy    
> > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo
> > > > > > Chen
> > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans
> > > > > > de
> > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet    
> > > > <jbrunet@baylibre.com>;    
> > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > <lars@metafoo.de>;    
> > > > Andy    
> > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > > > > Baolin
> > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson
> > > > > > Zhai
> > > > > > <orsonzhai@gmail.com>
> > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > > internal    
> > > > iio_dev    
> > > > > > lock
> > > > > > 
> > > > > > [External]
> > > > > > 
> > > > > > Hi Nuno,
> > > > > >    
> > > > > 
> > > > > Hi Miquel,
> > > > > 
> > > > > Thanks for reviewing...
> > > > >    
> > > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > > ---
> > > > > > >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jonathan Cameron]

On Tue, 20 Sep 2022 17:10:33 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Nuno,
> 
> noname.nuno@gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> 
> > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:  
> > > Hi Nuno,
> > > 
> > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > >     
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-arm-kernel@lists.infradead.org;
> > > > > linux-rockchip@lists.infradead.org;
> > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> > > > > Hennerich,
> > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > > > Kevin
> > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> > > > > Ardelean
> > > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;
> > > > > Andriy
> > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> > > > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > > > <jbrunet@baylibre.com>;
> > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > > > > Andy
> > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > > > Baolin
> > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> > > > > <orsonzhai@gmail.com>
> > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > internal iio_dev
> > > > > lock
> > > > > 
> > > > > [External]
> > > > > 
> > > > > Hi Nuno,
> > > > > 
> > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > > > >       
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-      
> > > > > rockchip@lists.infradead.org;      
> > > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> > > > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;      
> > > > > Hennerich,      
> > > > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>;
> > > > > > > Kevin
> > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > <vz@mleia.com>;
> > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru      
> > > > > Ardelean      
> > > > > > > <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>;     
> > > > > Andriy      
> > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo
> > > > > > > Chen
> > > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans
> > > > > > > de
> > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet      
> > > > > <jbrunet@baylibre.com>;      
> > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > <lars@metafoo.de>;      
> > > > > Andy      
> > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > > > <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>;
> > > > > > > Baolin
> > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson
> > > > > > > Zhai
> > > > > > > <orsonzhai@gmail.com>
> > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > > > internal      
> > > > > iio_dev      
> > > > > > > lock
> > > > > > > 
> > > > > > > [External]
> > > > > > > 
> > > > > > > Hi Nuno,
> > > > > > >      
> > > > > > 
> > > > > > Hi Miquel,
> > > > > > 
> > > > > > Thanks for reviewing...
> > > > > >      
> > > > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > > > ---
> > > > > > > >  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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 17:10:33 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Nuno,
> > 
> > noname.nuno@gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > 
> > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:  
> > > > Hi Nuno,
> > > > 
> > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-arm-kernel@lists.infradead.org;
> > > > > > linux-rockchip@lists.infradead.org;
> > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com;
> > > > > > linux-
> > > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> > > > > > Hennerich,
> > > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > <cixi.geng1@unisoc.com>;
> > > > > > Kevin
> > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > <vz@mleia.com>;
> > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> > > > > > Ardelean
> > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > <festevam@gmail.com>;
> > > > > > Andriy
> > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo
> > > > > > Chen
> > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans
> > > > > > de
> > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > > > > <jbrunet@baylibre.com>;
> > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > <lars@metafoo.de>;
> > > > > > Andy
> > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > <narmstrong@baylibre.com>;
> > > > > > Baolin
> > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson
> > > > > > Zhai
> > > > > > <orsonzhai@gmail.com>
> > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not
> > > > > > use
> > > > > > internal iio_dev
> > > > > > lock
> > > > > > 
> > > > > > [External]
> > > > > > 
> > > > > > Hi Nuno,
> > > > > > 
> > > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > +0000:
> > > > > >       
> > > > > > > > -----Original Message-----
> > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-      
> > > > > > rockchip@lists.infradead.org;      
> > > > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com;
> > > > > > > > linux-
> > > > > > > > iio@vger.kernel.org; Chunyan Zhang
> > > > > > > > <zhang.lyra@gmail.com>;      
> > > > > > Hennerich,      
> > > > > > > > Michael <Michael.Hennerich@analog.com>; Martin
> > > > > > > > Blumenstingl
> > > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > > > <cixi.geng1@unisoc.com>;
> > > > > > > > Kevin
> > > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > > <vz@mleia.com>;
> > > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>;
> > > > > > > > Alexandru      
> > > > > > Ardelean      
> > > > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > > > <festevam@gmail.com>;     
> > > > > > Andriy      
> > > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>;
> > > > > > > > Haibo
> > > > > > > > Chen
> > > > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> > > > > > > > Hans
> > > > > > > > de
> > > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet      
> > > > > > <jbrunet@baylibre.com>;      
> > > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > > <lars@metafoo.de>;      
> > > > > > Andy      
> > > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan
> > > > > > > > Cameron
> > > > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > > > <narmstrong@baylibre.com>;
> > > > > > > > Baolin
> > > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>;
> > > > > > > > Orson
> > > > > > > > Zhai
> > > > > > > > <orsonzhai@gmail.com>
> > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > not use
> > > > > > > > internal      
> > > > > > iio_dev      
> > > > > > > > lock
> > > > > > > > 
> > > > > > > > [External]
> > > > > > > > 
> > > > > > > > Hi Nuno,
> > > > > > > >      
> > > > > > > 
> > > > > > > Hi Miquel,
> > > > > > > 
> > > > > > > Thanks for reviewing...
> > > > > > >      
> > > > > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > > > > ---
> > > > > > > > >  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á

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jonathan Cameron]

On Mon, 26 Sep 2022 12:06:13 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> > On Tue, 20 Sep 2022 17:10:33 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Nuno,
> > > 
> > > noname.nuno@gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > >   
> > > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:    
> > > > > Hi Nuno,
> > > > > 
> > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > > > >       
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > Cc: linux-arm-kernel@lists.infradead.org;
> > > > > > > linux-rockchip@lists.infradead.org;
> > > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com;
> > > > > > > linux-
> > > > > > > iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>;
> > > > > > > Hennerich,
> > > > > > > Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > > <cixi.geng1@unisoc.com>;
> > > > > > > Kevin
> > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > <vz@mleia.com>;
> > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru
> > > > > > > Ardelean
> > > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > > <festevam@gmail.com>;
> > > > > > > Andriy
> > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo
> > > > > > > Chen
> > > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans
> > > > > > > de
> > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > > > > > <jbrunet@baylibre.com>;
> > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > <lars@metafoo.de>;
> > > > > > > Andy
> > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> > > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > > <narmstrong@baylibre.com>;
> > > > > > > Baolin
> > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson
> > > > > > > Zhai
> > > > > > > <orsonzhai@gmail.com>
> > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not
> > > > > > > use
> > > > > > > internal iio_dev
> > > > > > > lock
> > > > > > > 
> > > > > > > [External]
> > > > > > > 
> > > > > > > Hi Nuno,
> > > > > > > 
> > > > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > > +0000:
> > > > > > >         
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-        
> > > > > > > rockchip@lists.infradead.org;        
> > > > > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com;
> > > > > > > > > linux-
> > > > > > > > > iio@vger.kernel.org; Chunyan Zhang
> > > > > > > > > <zhang.lyra@gmail.com>;        
> > > > > > > Hennerich,        
> > > > > > > > > Michael <Michael.Hennerich@analog.com>; Martin
> > > > > > > > > Blumenstingl
> > > > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > > > > <cixi.geng1@unisoc.com>;
> > > > > > > > > Kevin
> > > > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > > > <vz@mleia.com>;
> > > > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>;
> > > > > > > > > Alexandru        
> > > > > > > Ardelean        
> > > > > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > > > > <festevam@gmail.com>;       
> > > > > > > Andriy        
> > > > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>;
> > > > > > > > > Haibo
> > > > > > > > > Chen
> > > > > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> > > > > > > > > Hans
> > > > > > > > > de
> > > > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet        
> > > > > > > <jbrunet@baylibre.com>;        
> > > > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > > > <lars@metafoo.de>;        
> > > > > > > Andy        
> > > > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan
> > > > > > > > > Cameron
> > > > > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > > > > <narmstrong@baylibre.com>;
> > > > > > > > > Baolin
> > > > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>;
> > > > > > > > > Orson
> > > > > > > > > Zhai
> > > > > > > > > <orsonzhai@gmail.com>
> > > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > > not use
> > > > > > > > > internal        
> > > > > > > iio_dev        
> > > > > > > > > lock
> > > > > > > > > 
> > > > > > > > > [External]
> > > > > > > > > 
> > > > > > > > > Hi Nuno,
> > > > > > > > >        
> > > > > > > > 
> > > > > > > > Hi Miquel,
> > > > > > > > 
> > > > > > > > Thanks for reviewing...
> > > > > > > >        
> > > > > > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > > > > > ---
> > > > > > > > > >  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á


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

On Sun, 2022-10-02 at 12:03 +0100, Jonathan Cameron wrote:
> On Mon, 26 Sep 2022 12:06:13 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sat, 2022-09-24 at 16:53 +0100, Jonathan Cameron wrote:
> > > On Tue, 20 Sep 2022 17:10:33 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >   
> > > > Hi Nuno,
> > > > 
> > > > noname.nuno@gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:
> > > >   
> > > > > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:    
> > > > > > Hi Nuno,
> > > > > > 
> > > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 13:15:32
> > > > > > +0000:
> > > > > >       
> > > > > > > > -----Original Message-----
> > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > > Cc: linux-arm-kernel@lists.infradead.org;
> > > > > > > > linux-rockchip@lists.infradead.org;
> > > > > > > > linux-amlogic@lists.infradead.org; linux-imx@nxp.com;
> > > > > > > > linux-
> > > > > > > > iio@vger.kernel.org; Chunyan Zhang
> > > > > > > > <zhang.lyra@gmail.com>;
> > > > > > > > Hennerich,
> > > > > > > > Michael <Michael.Hennerich@analog.com>; Martin
> > > > > > > > Blumenstingl
> > > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > > > <cixi.geng1@unisoc.com>;
> > > > > > > > Kevin
> > > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > > <vz@mleia.com>;
> > > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>;
> > > > > > > > Alexandru
> > > > > > > > Ardelean
> > > > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > > > <festevam@gmail.com>;
> > > > > > > > Andriy
> > > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>;
> > > > > > > > Haibo
> > > > > > > > Chen
> > > > > > > > <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> > > > > > > > Hans
> > > > > > > > de
> > > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet
> > > > > > > > <jbrunet@baylibre.com>;
> > > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > > <lars@metafoo.de>;
> > > > > > > > Andy
> > > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan
> > > > > > > > Cameron
> > > > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > > > <narmstrong@baylibre.com>;
> > > > > > > > Baolin
> > > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > > <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>;
> > > > > > > > Orson
> > > > > > > > Zhai
> > > > > > > > <orsonzhai@gmail.com>
> > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do
> > > > > > > > not
> > > > > > > > use
> > > > > > > > internal iio_dev
> > > > > > > > lock
> > > > > > > > 
> > > > > > > > [External]
> > > > > > > > 
> > > > > > > > Hi Nuno,
> > > > > > > > 
> > > > > > > > Nuno.Sa@analog.com wrote on Tue, 20 Sep 2022 12:44:08
> > > > > > > > +0000:
> > > > > > > >         
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > > > > Cc: linux-arm-kernel@lists.infradead.org; linux-
> > > > > > > > > >         
> > > > > > > > rockchip@lists.infradead.org;        
> > > > > > > > > > linux-amlogic@lists.infradead.org;
> > > > > > > > > > linux-imx@nxp.com;
> > > > > > > > > > linux-
> > > > > > > > > > iio@vger.kernel.org; Chunyan Zhang
> > > > > > > > > > <zhang.lyra@gmail.com>;        
> > > > > > > > Hennerich,        
> > > > > > > > > > Michael <Michael.Hennerich@analog.com>; Martin
> > > > > > > > > > Blumenstingl
> > > > > > > > > > <martin.blumenstingl@googlemail.com>; Sascha Hauer
> > > > > > > > > > <s.hauer@pengutronix.de>; Cixi Geng
> > > > > > > > > > <cixi.geng1@unisoc.com>;
> > > > > > > > > > Kevin
> > > > > > > > > > Hilman <khilman@baylibre.com>; Vladimir Zapolskiy
> > > > > > > > > > <vz@mleia.com>;
> > > > > > > > > > Pengutronix Kernel Team <kernel@pengutronix.de>;
> > > > > > > > > > Alexandru        
> > > > > > > > Ardelean        
> > > > > > > > > > <aardelean@deviqon.com>; Fabio Estevam
> > > > > > > > > > <festevam@gmail.com>;       
> > > > > > > > Andriy        
> > > > > > > > > > Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>;
> > > > > > > > > > Haibo
> > > > > > > > > > Chen
> > > > > > > > > > <haibo.chen@nxp.com>; Shawn Guo
> > > > > > > > > > <shawnguo@kernel.org>;
> > > > > > > > > > Hans
> > > > > > > > > > de
> > > > > > > > > > Goede <hdegoede@redhat.com>; Jerome Brunet        
> > > > > > > > <jbrunet@baylibre.com>;        
> > > > > > > > > > Heiko Stuebner <heiko@sntech.de>; Florian Boor
> > > > > > > > > > <florian.boor@kernelconcepts.de>; Regus, Ciprian
> > > > > > > > > > <Ciprian.Regus@analog.com>; Lars-Peter Clausen
> > > > > > > > > > <lars@metafoo.de>;        
> > > > > > > > Andy        
> > > > > > > > > > Shevchenko <andy.shevchenko@gmail.com>; Jonathan
> > > > > > > > > > Cameron
> > > > > > > > > > <jic23@kernel.org>; Neil Armstrong
> > > > > > > > > > <narmstrong@baylibre.com>;
> > > > > > > > > > Baolin
> > > > > > > > > > Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> > > > > > > > > > <jbhayana@google.com>; Chen-Yu Tsai
> > > > > > > > > > <wens@csie.org>;
> > > > > > > > > > Orson
> > > > > > > > > > Zhai
> > > > > > > > > > <orsonzhai@gmail.com>
> > > > > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100:
> > > > > > > > > > do
> > > > > > > > > > not use
> > > > > > > > > > internal        
> > > > > > > > iio_dev        
> > > > > > > > > > lock
> > > > > > > > > > 
> > > > > > > > > > [External]
> > > > > > > > > > 
> > > > > > > > > > Hi Nuno,
> > > > > > > > > >        
> > > > > > > > > 
> > > > > > > > > Hi Miquel,
> > > > > > > > > 
> > > > > > > > > Thanks for reviewing...
> > > > > > > > >        
> > > > > > > > > > nuno.sa@analog.com 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á <nuno.sa@analog.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  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á
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

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

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

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:20 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

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

Jonathan


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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

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

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

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


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

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

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

- Nuno Sá


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jonathan Cameron]

On Fri, 30 Sep 2022 12:04:39 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

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

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

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

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

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nuno Sá]

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

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

- Nuno Sá


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 15/15] iio: core: move 'mlock' to 'struct iio_dev_opaque'

[Nuno Sá]

Now that there are no more users accessing 'mlock' directly, we can move
it to the iio_dev private structure. Hence, it's now explicit that new
driver's should not directly this lock.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/TODO                   |  3 ---
 drivers/iio/industrialio-buffer.c  | 29 +++++++++++++++++------------
 drivers/iio/industrialio-core.c    | 18 ++++++++++--------
 drivers/iio/industrialio-event.c   |  4 ++--
 drivers/iio/industrialio-trigger.c | 12 ++++++------
 include/linux/iio/iio-opaque.h     |  2 ++
 include/linux/iio/iio.h            |  3 ---
 7 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/TODO b/drivers/iio/TODO
index 7d7326b7085a..2ace27d1ac62 100644
--- a/drivers/iio/TODO
+++ b/drivers/iio/TODO
@@ -7,9 +7,6 @@ tree
   - ABI Documentation
   - Audit driviers/iio/staging/Documentation
 
-- Replace iio_dev->mlock by either a local lock or use
-iio_claim_direct.(Requires analysis of the purpose of the lock.)
-
 - Converting drivers from device tree centric to more generic
 property handlers.
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 228598b82a2f..9cd7db549fcb 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -507,13 +507,14 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	int ret;
 	bool state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	struct iio_buffer *buffer = this_attr->buffer;
 
 	ret = kstrtobool(buf, &state);
 	if (ret < 0)
 		return ret;
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
@@ -532,7 +533,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	}
 
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret < 0 ? ret : len;
 
@@ -554,6 +555,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	bool state;
 
@@ -561,14 +563,14 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
 	buffer->scan_timestamp = state;
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
@@ -642,6 +644,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	unsigned int val;
 	int ret;
@@ -653,7 +656,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 	if (val == buffer->length)
 		return len;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 	} else {
@@ -665,7 +668,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 	if (buffer->length && buffer->length < buffer->watermark)
 		buffer->watermark = buffer->length;
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
@@ -1256,7 +1259,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (insert_buffer && iio_buffer_is_active(insert_buffer))
 		insert_buffer = NULL;
@@ -1277,7 +1280,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
 
 out_unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	mutex_unlock(&iio_dev_opaque->info_exist_lock);
 
 	return ret;
@@ -1296,6 +1299,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 	int ret;
 	bool requested_state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	bool inlist;
 
@@ -1303,7 +1307,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	/* Find out if it is in the list */
 	inlist = iio_buffer_is_active(buffer);
@@ -1317,7 +1321,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 		ret = __iio_update_buffers(indio_dev, NULL, buffer);
 
 done:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return (ret < 0) ? ret : len;
 }
 
@@ -1334,6 +1338,7 @@ static ssize_t watermark_store(struct device *dev,
 			       const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	unsigned int val;
 	int ret;
@@ -1344,7 +1349,7 @@ static ssize_t watermark_store(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (val > buffer->length) {
 		ret = -EINVAL;
@@ -1358,7 +1363,7 @@ static ssize_t watermark_store(struct device *dev,
 
 	buffer->watermark = val;
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 151ff3993354..fca7dcc4be29 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -285,16 +285,16 @@ int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	const struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 
-	ret = mutex_lock_interruptible(&indio_dev->mlock);
+	ret = mutex_lock_interruptible(&iio_dev_opaque->mlock);
 	if (ret)
 		return ret;
 	if ((ev_int && iio_event_enabled(ev_int)) ||
 	    iio_buffer_enabled(indio_dev)) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	iio_dev_opaque->clock_id = clock_id;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return 0;
 }
@@ -1674,7 +1674,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	indio_dev->dev.type = &iio_device_type;
 	indio_dev->dev.bus = &iio_bus_type;
 	device_initialize(&indio_dev->dev);
-	mutex_init(&indio_dev->mlock);
+	mutex_init(&iio_dev_opaque->mlock);
 	mutex_init(&iio_dev_opaque->info_exist_lock);
 	INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list);
 
@@ -1696,7 +1696,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);
 
 	lockdep_register_key(&iio_dev_opaque->mlock_key);
-	lockdep_set_class(&indio_dev->mlock, &iio_dev_opaque->mlock_key);
+	lockdep_set_class(&iio_dev_opaque->mlock, &iio_dev_opaque->mlock_key);
 
 	return indio_dev;
 }
@@ -2058,10 +2058,12 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
  */
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
 {
-	mutex_lock(&indio_dev->mlock);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (iio_buffer_enabled(indio_dev)) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	return 0;
@@ -2079,7 +2081,7 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
  */
 void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 {
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 3d78da2531a9..1a26393a7c0c 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -198,7 +198,7 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
 	if (ev_int == NULL)
 		return -ENODEV;
 
-	fd = mutex_lock_interruptible(&indio_dev->mlock);
+	fd = mutex_lock_interruptible(&iio_dev_opaque->mlock);
 	if (fd)
 		return fd;
 
@@ -219,7 +219,7 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
 	}
 
 unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return fd;
 }
 
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 6885a186fe27..a2f3cc2f65ef 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -120,12 +120,12 @@ int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri
 		return -EINVAL;
 
 	iio_dev_opaque = to_iio_dev_opaque(indio_dev);
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	WARN_ON(iio_dev_opaque->trig_readonly);
 
 	indio_dev->trig = iio_trigger_get(trig);
 	iio_dev_opaque->trig_readonly = true;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return 0;
 }
@@ -438,16 +438,16 @@ static ssize_t current_trigger_store(struct device *dev,
 	struct iio_trigger *trig;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	if (iio_dev_opaque->trig_readonly) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EPERM;
 	}
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	trig = iio_trigger_acquire_by_name(buf);
 	if (oldtrig == trig) {
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index d1f8b30a7c8b..5aec3945555b 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -11,6 +11,7 @@
  *				checked by device drivers but should be considered
  *				read-only as this is a core internal bit
  * @driver_module:		used to make it harder to undercut users
+ * @mlock:			lock used to prevent simultaneous device state changes
  * @mlock_key:			lockdep class for iio_dev lock
  * @info_exist_lock:		lock to prevent use during removal
  * @trig_readonly:		mark the current trigger immutable
@@ -43,6 +44,7 @@ struct iio_dev_opaque {
 	int				currentmode;
 	int				id;
 	struct module			*driver_module;
+	struct mutex			mlock;
 	struct lock_class_key		mlock_key;
 	struct mutex			info_exist_lock;
 	bool				trig_readonly;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f0ec8a5e5a7a..b27ae8c7b59e 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -548,8 +548,6 @@ struct iio_buffer_setup_ops {
  *			and owner
  * @buffer:		[DRIVER] any buffer present
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
- * @mlock:		[INTERN] lock used to prevent simultaneous device state
- *			changes
  * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
  * @masklength:		[INTERN] the length of the mask established from
  *			channels
@@ -574,7 +572,6 @@ struct iio_dev {
 
 	struct iio_buffer		*buffer;
 	int				scan_bytes;
-	struct mutex			mlock;
 
 	const unsigned long		*available_scan_masks;
 	unsigned			masklength;
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 15/15] iio: core: move 'mlock' to 'struct iio_dev_opaque'

[Jonathan Cameron]

On Tue, 20 Sep 2022 13:28:21 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> Now that there are no more users accessing 'mlock' directly, we can move
> it to the iio_dev private structure. Hence, it's now explicit that new
> driver's should not directly this lock.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
LGTM  Nice work

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel