[PATCH v2 00/16] Make 'mlock' really private

[PATCH v2 00/16] 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).

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

v2:

[PATCH 1-8, 10-12/16]
 * Mention the inclusion of mutex.h in the commit message.

[PATCH 1-8, 10, 12/16]
 * Initialize mutex as late as possible.
Note that [PATCH 11/16] was not included since the code to do so was not
direct enough. Would need to get a pointer to the private struture
outside of scmi_alloc_iiodev() to do it. While not hard, the added changes
in the code is not really worth it (IMO of course).

[PATCH 1/16]
 * Refactored the commit message a bit. I guess this one will still needs
more discussion...

[PATCH 9/16]
 * New patch to add an helper function to read the samples.

[PATCH 13/16]
 * New patch to introduce iio_device_{claim|release}_buffer_mode() APIs.

[PATCH 14/16]
 * Make use of the new iio_device_{claim|release}_buffer_mode() helpers

[PATCH 15/16]
 * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
in combination with claim_direct_mode(). This is needed so that we make sure
we always get one of the modes (and hence the iio_dev lock) to safely call
max30102_get_temp(). Note that I'm not particular "happy" with the code but
OTOH, it does not look as bad as I thought :). Anyways, if there are no
complains with it, I'm ok to leave it as-is. Otherwise, I think we can think
on the flag approach (briefly discussed in the first series).

Nuno Sá (16):
  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: add helper function to read samples
  iio: adc: vf610_adc: vf610_adc: do not use internal iio_dev lock
  iio: common: scmi_iio: do not use internal iio_dev lock
  iio: gyro: itg3200_core: do not use internal iio_dev lock
  iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs
  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                   |  20 ++--
 drivers/iio/adc/axp288_adc.c               |  10 +-
 drivers/iio/adc/imx7d_adc.c                |  14 ++-
 drivers/iio/adc/lpc32xx_adc.c              |  11 ++-
 drivers/iio/adc/ltc2497-core.c             |   7 +-
 drivers/iio/adc/ltc2497.h                  |   2 +
 drivers/iio/adc/meson_saradc.c             |  11 ++-
 drivers/iio/adc/rockchip_saradc.c          |  15 ++-
 drivers/iio/adc/sc27xx_adc.c               |  14 ++-
 drivers/iio/adc/vf610_adc.c                | 104 ++++++++++++---------
 drivers/iio/common/scmi_sensors/scmi_iio.c |   9 +-
 drivers/iio/gyro/itg3200_core.c            |  10 +-
 drivers/iio/health/max30100.c              |   9 +-
 drivers/iio/health/max30102.c              |  19 +++-
 drivers/iio/industrialio-buffer.c          |  29 +++---
 drivers/iio/industrialio-core.c            |  58 ++++++++++--
 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                    |   5 +-
 22 files changed, 249 insertions(+), 121 deletions(-)

-- 
2.37.3


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

[PATCH v2 01/16] 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
since it was also being used to protect st->config against the current
device state.

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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 262bd7665b33..44f7a80a0749 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) &
@@ -863,6 +868,9 @@ static int ad799x_probe(struct i2c_client *client,
 		if (ret)
 			goto error_cleanup_ring;
 	}
+
+	mutex_init(&st->lock);
+
 	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto error_cleanup_ring;
-- 
2.37.3


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

Re: [PATCH v2 01/16] iio: adc: ad799x: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:48:54 +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
> since it was also being used to protect st->config against the current
> device state.
> 
> 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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
In interests of cutting down scope of any future versions
(Should there need to be anyway) I'm going to pick up the non controversial
patches.

Applied to the togreg branch of iio.git though that's only pushed out
as testing for now as we are mid merge window.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad799x.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 262bd7665b33..44f7a80a0749 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) &
> @@ -863,6 +868,9 @@ static int ad799x_probe(struct i2c_client *client,
>  		if (ret)
>  			goto error_cleanup_ring;
>  	}
> +
> +	mutex_init(&st->lock);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
>  		goto error_cleanup_ring;


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

[PATCH v2 02/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
index 580361bd9849..53781010f789 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);
+
 	info->irq = platform_get_irq(pdev, 0);
 	if (info->irq < 0)
 		return info->irq;
@@ -289,6 +293,8 @@ static int axp288_adc_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	mutex_init(&info->lock);
+
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
-- 
2.37.3


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

Re: [PATCH v2 02/16] iio: adc: axp288_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:48:55 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied with tiny tweak to drop the unrelated white space change.

Applied to the togreg branch of iio.git which is only pushed out
as testing until after a rebase on rc1.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/axp288_adc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 580361bd9849..53781010f789 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);
> +
>  	info->irq = platform_get_irq(pdev, 0);
>  	if (info->irq < 0)
>  		return info->irq;
> @@ -289,6 +293,8 @@ static int axp288_adc_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	mutex_init(&info->lock);
> +
>  	return devm_iio_device_register(&pdev->dev, indio_dev);
>  }
>  


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

[PATCH v2 03/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 86caff1d006b..22da81bac97f 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:
@@ -531,6 +533,8 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	mutex_init(&info->lock);
+
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't register the device.\n");
-- 
2.37.3


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

RE: [PATCH v2 03/16] iio: adc: imx7d_adc: do not use internal iio_dev lock

[Bough Chen]

> -----Original Message-----
> From: Nuno Sá <nuno.sa@analog.com>
> Sent: 2022年10月4日 21:49
> To: linux-amlogic@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-rockchip@lists.infradead.org
> Cc: Heiko Stuebner <heiko@sntech.de>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Neil Armstrong
> <narmstrong@baylibre.com>; Shawn Guo <shawnguo@kernel.org>; Lars-Peter
> Clausen <lars@metafoo.de>; Jyoti Bhayana <jbhayana@google.com>; Hans de
> Goede <hdegoede@redhat.com>; Andriy Tryshnivskyy
> <andriy.tryshnivskyy@opensynergy.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Miquel Raynal <miquel.raynal@bootlin.com>; Cixi
> Geng <cixi.geng1@unisoc.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Ciprian Regus <ciprian.regus@analog.com>;
> Fabio Estevam <festevam@gmail.com>; Nuno Sá <nuno.sa@analog.com>;
> Sascha Hauer <s.hauer@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Florian Boor <florian.boor@kernelconcepts.de>;
> Michael Hennerich <Michael.Hennerich@analog.com>; Orson Zhai
> <orsonzhai@gmail.com>; Chen-Yu Tsai <wens@csie.org>; Chunyan Zhang
> <zhang.lyra@gmail.com>; Vladimir Zapolskiy <vz@mleia.com>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Bough Chen <haibo.chen@nxp.com>; Kevin Hilman
> <khilman@baylibre.com>; Jonathan Cameron <jic23@kernel.org>
> Subject: [PATCH v2 03/16] iio: adc: imx7d_adc: do not use internal iio_dev lock
> 
> The iio_device lock is only meant for internal use. Hence define a device local
> lock to protect against concurrent accesses.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

Thanks!
Best Regards
Haibo Chen
> ---
>  drivers/iio/adc/imx7d_adc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c index
> 86caff1d006b..22da81bac97f 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:
> @@ -531,6 +533,8 @@ static int imx7d_adc_probe(struct platform_device
> *pdev)
>  	if (ret)
>  		return ret;
> 
> +	mutex_init(&info->lock);
> +
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
> --
> 2.37.3

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

Re: [PATCH v2 03/16] iio: adc: imx7d_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Sun, 9 Oct 2022 02:00:22 +0000
Bough Chen <haibo.chen@nxp.com> wrote:

> > -----Original Message-----
> > From: Nuno Sá <nuno.sa@analog.com>
> > Sent: 2022年10月4日 21:49
> > To: linux-amlogic@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> > linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-rockchip@lists.infradead.org
> > Cc: Heiko Stuebner <heiko@sntech.de>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Neil Armstrong
> > <narmstrong@baylibre.com>; Shawn Guo <shawnguo@kernel.org>; Lars-Peter
> > Clausen <lars@metafoo.de>; Jyoti Bhayana <jbhayana@google.com>; Hans de
> > Goede <hdegoede@redhat.com>; Andriy Tryshnivskyy
> > <andriy.tryshnivskyy@opensynergy.com>; Pengutronix Kernel Team
> > <kernel@pengutronix.de>; Miquel Raynal <miquel.raynal@bootlin.com>; Cixi
> > Geng <cixi.geng1@unisoc.com>; Baolin Wang
> > <baolin.wang@linux.alibaba.com>; Ciprian Regus <ciprian.regus@analog.com>;
> > Fabio Estevam <festevam@gmail.com>; Nuno Sá <nuno.sa@analog.com>;
> > Sascha Hauer <s.hauer@pengutronix.de>; Alexandru Ardelean
> > <aardelean@deviqon.com>; Florian Boor <florian.boor@kernelconcepts.de>;
> > Michael Hennerich <Michael.Hennerich@analog.com>; Orson Zhai
> > <orsonzhai@gmail.com>; Chen-Yu Tsai <wens@csie.org>; Chunyan Zhang
> > <zhang.lyra@gmail.com>; Vladimir Zapolskiy <vz@mleia.com>; Andy
> > Shevchenko <andy.shevchenko@gmail.com>; Jerome Brunet
> > <jbrunet@baylibre.com>; Bough Chen <haibo.chen@nxp.com>; Kevin Hilman
> > <khilman@baylibre.com>; Jonathan Cameron <jic23@kernel.org>
> > Subject: [PATCH v2 03/16] iio: adc: imx7d_adc: do not use internal iio_dev lock
> > 
> > The iio_device lock is only meant for internal use. Hence define a device local
> > lock to protect against concurrent accesses.
> > 
> > While at it, properly include "mutex.h" for mutex related APIs.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> 
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to take a look at it.

Note I'll be rebasing on rc1 once available.

Thanks,

Jonathan

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

[PATCH v2 04/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
index b56ce15255cf..732c924a976d 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;
 
@@ -201,6 +204,8 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
 	iodev->modes = INDIO_DIRECT_MODE;
 	iodev->num_channels = ARRAY_SIZE(lpc32xx_adc_iio_channels);
 
+	mutex_init(&st->lock);
+
 	retval = devm_iio_device_register(&pdev->dev, iodev);
 	if (retval)
 		return retval;
-- 
2.37.3


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

Re: [PATCH v2 04/16] iio: adc: lpc32xx_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:48:57 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied.

Thanks,

J

> ---
>  drivers/iio/adc/lpc32xx_adc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index b56ce15255cf..732c924a976d 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;
>  
> @@ -201,6 +204,8 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>  	iodev->modes = INDIO_DIRECT_MODE;
>  	iodev->num_channels = ARRAY_SIZE(lpc32xx_adc_iio_channels);
>  
> +	mutex_init(&st->lock);
> +
>  	retval = devm_iio_device_register(&pdev->dev, iodev);
>  	if (retval)
>  		return retval;


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

[PATCH v2 05/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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..996f6cbbed3c 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;
 
@@ -214,6 +215,8 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
 	ddata->addr_prev = LTC2497_CONFIG_DEFAULT;
 	ddata->time_prev = ktime_get();
 
+	mutex_init(&ddata->lock);
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto err_array_unregister;
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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 05/16] iio: adc: ltc2947-core: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:48:58 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied

> ---
>  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..996f6cbbed3c 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;
>  
> @@ -214,6 +215,8 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
>  	ddata->addr_prev = LTC2497_CONFIG_DEFAULT;
>  	ddata->time_prev = ktime_get();
>  
> +	mutex_init(&ddata->lock);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto err_array_unregister;
> 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,


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

[PATCH v2 06/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 1a68b099d323..85b6826cc10c 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)
@@ -1250,6 +1253,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err;
 
+	mutex_init(&priv->lock);
+
 	ret = meson_sar_adc_hw_enable(indio_dev);
 	if (ret)
 		goto err;
-- 
2.37.3


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

Re: [PATCH v2 06/16] iio: adc: meson_saradc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:48:59 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Applied to the togreg branch of iio.git and pushed out (shortly) as testing
for 0-day etc to take a look.

Note I'll be rebasing on rc1 once it's out.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/meson_saradc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 1a68b099d323..85b6826cc10c 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)
> @@ -1250,6 +1253,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err;
>  
> +	mutex_init(&priv->lock);
> +
>  	ret = meson_sar_adc_hw_enable(indio_dev);
>  	if (ret)
>  		goto err;


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

[PATCH v2 07/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index b87ea7148b58..79448c5ffc2a 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);
 
@@ -478,6 +481,8 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	mutex_init(&info->lock);
+
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
-- 
2.37.3


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

Re: [PATCH v2 07/16] iio: adc: rockchip_saradc: do not use internal iio_dev lock

[Heiko Stübner]

Am Dienstag, 4. Oktober 2022, 15:49:00 CEST schrieb Nuno Sá:
> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Acked-by: Heiko Stuebner <heiko@sntech.de>



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

Re: [PATCH v2 07/16] iio: adc: rockchip_saradc: do not use internal iio_dev lock

[Jonathan Cameron]

On Wed, 05 Oct 2022 09:44:37 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Am Dienstag, 4. Oktober 2022, 15:49:00 CEST schrieb Nuno Sá:
> > The iio_device lock is only meant for internal use. Hence define a
> > device local lock to protect against concurrent accesses.
> > 
> > While at it, properly include "mutex.h" for mutex related APIs.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> 
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> 
> 
Applied and pushed out as testing. Note I'll be rebasing on rc1 once available.

Thanks,

Jonathan


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

[PATCH v2 08/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index f8421cbba8fa..ff1fc329bb9b 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;
@@ -934,6 +937,9 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 	indio_dev->info = &sc27xx_info;
 	indio_dev->channels = sc27xx_channels;
 	indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
+
+	mutex_init(&sc27xx_data->lock);
+
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		dev_err(dev, "could not register iio (ADC)");
-- 
2.37.3


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

Re: [PATCH v2 08/16] iio: adc: sc27xx_adc: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:49:01 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied. Pushed out as testing for now, will rebase after rc1.

Thanks,

J
> ---
>  drivers/iio/adc/sc27xx_adc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index f8421cbba8fa..ff1fc329bb9b 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;
> @@ -934,6 +937,9 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>  	indio_dev->info = &sc27xx_info;
>  	indio_dev->channels = sc27xx_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
> +
> +	mutex_init(&sc27xx_data->lock);
> +
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret)
>  		dev_err(dev, "could not register iio (ADC)");


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

[PATCH v2 09/16] iio: adc: vf610_adc: add helper function to read samples

[Nuno Sá]

This is a precursor change to make it simpler to remove the 'mlock'
usage. Having the code in it's own helper function, also makes it easier
to read the error paths.

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

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index c6b16cf6e367..a6f9182d7766 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -622,6 +622,58 @@ static const struct attribute_group vf610_attribute_group = {
 	.attrs = vf610_attributes,
 };
 
+static int vf610_read_sample(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val)
+{
+	struct vf610_adc *info = iio_priv(indio_dev);
+	unsigned int hc_cfg;
+	int ret;
+
+	mutex_lock(&indio_dev->mlock);
+	if (iio_buffer_enabled(indio_dev)) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	reinit_completion(&info->completion);
+	hc_cfg = VF610_ADC_ADCHC(chan->channel);
+	hc_cfg |= VF610_ADC_AIEN;
+	writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
+	ret = wait_for_completion_interruptible_timeout(&info->completion,
+							VF610_ADC_TIMEOUT);
+	if (ret == 0) {
+		ret = -ETIMEDOUT;
+		goto out_unlock;
+	}
+
+	if (ret < 0)
+		goto out_unlock;
+
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		*val = info->value;
+		break;
+	case IIO_TEMP:
+		/*
+		 * Calculate in degree Celsius times 1000
+		 * Using the typical sensor slope of 1.84 mV/°C
+		 * and VREFH_ADC at 3.3V, V at 25°C of 699 mV
+		 */
+		*val = 25000 - ((int)info->value - VF610_VTEMP25_3V3) *
+				1000000 / VF610_TEMP_SLOPE_COEFF;
+
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_unlock:
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret;
+}
+
 static int vf610_read_raw(struct iio_dev *indio_dev,
 			struct iio_chan_spec const *chan,
 			int *val,
@@ -629,53 +681,15 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
 			long mask)
 {
 	struct vf610_adc *info = iio_priv(indio_dev);
-	unsigned int hc_cfg;
 	long ret;
 
 	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;
-		}
-
-		reinit_completion(&info->completion);
-		hc_cfg = VF610_ADC_ADCHC(chan->channel);
-		hc_cfg |= VF610_ADC_AIEN;
-		writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
-		ret = wait_for_completion_interruptible_timeout
-				(&info->completion, VF610_ADC_TIMEOUT);
-		if (ret == 0) {
-			mutex_unlock(&indio_dev->mlock);
-			return -ETIMEDOUT;
-		}
-		if (ret < 0) {
-			mutex_unlock(&indio_dev->mlock);
+		ret = vf610_read_sample(indio_dev, chan, val);
+		if (ret < 0)
 			return ret;
-		}
-
-		switch (chan->type) {
-		case IIO_VOLTAGE:
-			*val = info->value;
-			break;
-		case IIO_TEMP:
-			/*
-			 * Calculate in degree Celsius times 1000
-			 * Using the typical sensor slope of 1.84 mV/°C
-			 * and VREFH_ADC at 3.3V, V at 25°C of 699 mV
-			 */
-			*val = 25000 - ((int)info->value - VF610_VTEMP25_3V3) *
-					1000000 / VF610_TEMP_SLOPE_COEFF;
-
-			break;
-		default:
-			mutex_unlock(&indio_dev->mlock);
-			return -EINVAL;
-		}
 
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-- 
2.37.3


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

RE: [PATCH v2 09/16] iio: adc: vf610_adc: add helper function to read samples

[Bough Chen]

> -----Original Message-----
> From: Nuno Sá <nuno.sa@analog.com>
> Sent: 2022年10月4日 21:49
> To: linux-amlogic@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-rockchip@lists.infradead.org
> Cc: Heiko Stuebner <heiko@sntech.de>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Neil Armstrong
> <narmstrong@baylibre.com>; Shawn Guo <shawnguo@kernel.org>; Lars-Peter
> Clausen <lars@metafoo.de>; Jyoti Bhayana <jbhayana@google.com>; Hans de
> Goede <hdegoede@redhat.com>; Andriy Tryshnivskyy
> <andriy.tryshnivskyy@opensynergy.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Miquel Raynal <miquel.raynal@bootlin.com>; Cixi
> Geng <cixi.geng1@unisoc.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Ciprian Regus <ciprian.regus@analog.com>;
> Fabio Estevam <festevam@gmail.com>; Nuno Sá <nuno.sa@analog.com>;
> Sascha Hauer <s.hauer@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Florian Boor <florian.boor@kernelconcepts.de>;
> Michael Hennerich <Michael.Hennerich@analog.com>; Orson Zhai
> <orsonzhai@gmail.com>; Chen-Yu Tsai <wens@csie.org>; Chunyan Zhang
> <zhang.lyra@gmail.com>; Vladimir Zapolskiy <vz@mleia.com>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Bough Chen <haibo.chen@nxp.com>; Kevin Hilman
> <khilman@baylibre.com>; Jonathan Cameron <jic23@kernel.org>
> Subject: [PATCH v2 09/16] iio: adc: vf610_adc: add helper function to read
> samples
> 
> This is a precursor change to make it simpler to remove the 'mlock'
> usage. Having the code in it's own helper function, also makes it easier to read
> the error paths.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

> ---
>  drivers/iio/adc/vf610_adc.c | 94 +++++++++++++++++++++----------------
>  1 file changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c index
> c6b16cf6e367..a6f9182d7766 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -622,6 +622,58 @@ static const struct attribute_group
> vf610_attribute_group = {
>  	.attrs = vf610_attributes,
>  };
> 
> +static int vf610_read_sample(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val) {
> +	struct vf610_adc *info = iio_priv(indio_dev);
> +	unsigned int hc_cfg;
> +	int ret;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_buffer_enabled(indio_dev)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	reinit_completion(&info->completion);
> +	hc_cfg = VF610_ADC_ADCHC(chan->channel);
> +	hc_cfg |= VF610_ADC_AIEN;
> +	writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> +	ret = wait_for_completion_interruptible_timeout(&info->completion,
> +							VF610_ADC_TIMEOUT);
> +	if (ret == 0) {
> +		ret = -ETIMEDOUT;
> +		goto out_unlock;
> +	}
> +
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		*val = info->value;
> +		break;
> +	case IIO_TEMP:
> +		/*
> +		 * Calculate in degree Celsius times 1000
> +		 * Using the typical sensor slope of 1.84 mV/°C
> +		 * and VREFH_ADC at 3.3V, V at 25°C of 699 mV
> +		 */
> +		*val = 25000 - ((int)info->value - VF610_VTEMP25_3V3) *
> +				1000000 / VF610_TEMP_SLOPE_COEFF;
> +
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret;
> +}
> +
>  static int vf610_read_raw(struct iio_dev *indio_dev,
>  			struct iio_chan_spec const *chan,
>  			int *val,
> @@ -629,53 +681,15 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>  			long mask)
>  {
>  	struct vf610_adc *info = iio_priv(indio_dev);
> -	unsigned int hc_cfg;
>  	long ret;
> 
>  	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;
> -		}
> -
> -		reinit_completion(&info->completion);
> -		hc_cfg = VF610_ADC_ADCHC(chan->channel);
> -		hc_cfg |= VF610_ADC_AIEN;
> -		writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> -		ret = wait_for_completion_interruptible_timeout
> -				(&info->completion, VF610_ADC_TIMEOUT);
> -		if (ret == 0) {
> -			mutex_unlock(&indio_dev->mlock);
> -			return -ETIMEDOUT;
> -		}
> -		if (ret < 0) {
> -			mutex_unlock(&indio_dev->mlock);
> +		ret = vf610_read_sample(indio_dev, chan, val);
> +		if (ret < 0)
>  			return ret;
> -		}
> -
> -		switch (chan->type) {
> -		case IIO_VOLTAGE:
> -			*val = info->value;
> -			break;
> -		case IIO_TEMP:
> -			/*
> -			 * Calculate in degree Celsius times 1000
> -			 * Using the typical sensor slope of 1.84 mV/°C
> -			 * and VREFH_ADC at 3.3V, V at 25°C of 699 mV
> -			 */
> -			*val = 25000 - ((int)info->value - VF610_VTEMP25_3V3) *
> -					1000000 / VF610_TEMP_SLOPE_COEFF;
> -
> -			break;
> -		default:
> -			mutex_unlock(&indio_dev->mlock);
> -			return -EINVAL;
> -		}
> 
> -		mutex_unlock(&indio_dev->mlock);
>  		return IIO_VAL_INT;
> 
>  	case IIO_CHAN_INFO_SCALE:
> --
> 2.37.3

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

[PATCH v2 10/16] iio: adc: vf610_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).

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index a6f9182d7766..ae31aafd2653 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;
 }
@@ -629,12 +633,11 @@ static int vf610_read_sample(struct iio_dev *indio_dev,
 	unsigned int hc_cfg;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_enabled(indio_dev)) {
-		ret = -EBUSY;
-		goto out_unlock;
-	}
+	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;
@@ -669,7 +672,8 @@ static int vf610_read_sample(struct iio_dev *indio_dev,
 	}
 
 out_unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&info->lock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return ret;
 }
@@ -892,6 +896,8 @@ static int vf610_adc_probe(struct platform_device *pdev)
 		goto error_iio_device_register;
 	}
 
+	mutex_init(&info->lock);
+
 	ret = iio_device_register(indio_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't register the device.\n");
-- 
2.37.3


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

RE: [PATCH v2 10/16] iio: adc: vf610_adc: vf610_adc: do not use internal iio_dev lock

[Bough Chen]

> -----Original Message-----
> From: Nuno Sá <nuno.sa@analog.com>
> Sent: 2022年10月4日 21:49
> To: linux-amlogic@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-rockchip@lists.infradead.org
> Cc: Heiko Stuebner <heiko@sntech.de>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Neil Armstrong
> <narmstrong@baylibre.com>; Shawn Guo <shawnguo@kernel.org>; Lars-Peter
> Clausen <lars@metafoo.de>; Jyoti Bhayana <jbhayana@google.com>; Hans de
> Goede <hdegoede@redhat.com>; Andriy Tryshnivskyy
> <andriy.tryshnivskyy@opensynergy.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Miquel Raynal <miquel.raynal@bootlin.com>; Cixi
> Geng <cixi.geng1@unisoc.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Ciprian Regus <ciprian.regus@analog.com>;
> Fabio Estevam <festevam@gmail.com>; Nuno Sá <nuno.sa@analog.com>;
> Sascha Hauer <s.hauer@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Florian Boor <florian.boor@kernelconcepts.de>;
> Michael Hennerich <Michael.Hennerich@analog.com>; Orson Zhai
> <orsonzhai@gmail.com>; Chen-Yu Tsai <wens@csie.org>; Chunyan Zhang
> <zhang.lyra@gmail.com>; Vladimir Zapolskiy <vz@mleia.com>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Bough Chen <haibo.chen@nxp.com>; Kevin Hilman
> <khilman@baylibre.com>; Jonathan Cameron <jic23@kernel.org>
> Subject: [PATCH v2 10/16] iio: adc: vf610_adc: vf610_adc: do not use internal
> iio_dev lock
> 
> 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).
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

Thanks!

> ---
>  drivers/iio/adc/vf610_adc.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c index
> a6f9182d7766..ae31aafd2653 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;
>  }
> @@ -629,12 +633,11 @@ static int vf610_read_sample(struct iio_dev
> *indio_dev,
>  	unsigned int hc_cfg;
>  	int ret;
> 
> -	mutex_lock(&indio_dev->mlock);
> -	if (iio_buffer_enabled(indio_dev)) {
> -		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> +	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;
> @@ -669,7 +672,8 @@ static int vf610_read_sample(struct iio_dev *indio_dev,
>  	}
> 
>  out_unlock:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&info->lock);
> +	iio_device_release_direct_mode(indio_dev);
> 
>  	return ret;
>  }
> @@ -892,6 +896,8 @@ static int vf610_adc_probe(struct platform_device
> *pdev)
>  		goto error_iio_device_register;
>  	}
> 
> +	mutex_init(&info->lock);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
> --
> 2.37.3

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

[PATCH v2 11/16] 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.

While at it, properly include "mutex.h" for mutex related APIs.

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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:49:04 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied to the togreg branch of iio.git, but for now only pushed out as testing
as I'll be rebasing on rc1 once available.

Thanks,

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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2 12/16] iio: gyro: 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.

While at it, properly include "mutex.h" for mutex related APIs.

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

diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
index 0491c64e1b32..915985fbaed0 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:
@@ -336,6 +338,8 @@ static int itg3200_probe(struct i2c_client *client,
 	if (ret)
 		goto error_remove_trigger;
 
+	mutex_init(&st->lock);
+
 	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto error_remove_trigger;
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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 12/16] iio: gyro: itg3200_core: do not use internal iio_dev lock

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:49:05 +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.
> 
> While at it, properly include "mutex.h" for mutex related APIs.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Applied, but only pushed out as testing (which will be rebased)
until after rc1.

Jonathan

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

[PATCH v2 13/16] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Nuno Sá]

These APIs are analogous to iio_device_claim_direct_mode() and
iio_device_release_direct_mode() but, as the name suggests, with the
logic flipped. While this looks odd enough, it will have at least two
users (in following changes) and it will be important to move the iio
mlock to the private struct.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 38 +++++++++++++++++++++++++++++++++
 include/linux/iio/iio.h         |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 151ff3993354..c23d3abb33c5 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2083,6 +2083,44 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
+/**
+ * iio_device_claim_buffered_mode - Keep device in buffer mode
+ * @indio_dev:	the iio_dev associated with the device
+ *
+ * If the device is in buffer mode it is guaranteed to stay
+ * that way until iio_device_release_buffer_mode() is called.
+ *
+ * Use with iio_device_release_buffer_mode()
+ *
+ * Returns: 0 on success, -EBUSY on failure
+ */
+int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
+{
+	mutex_lock(&indio_dev->mlock);
+
+	if (iio_buffer_enabled(indio_dev))
+		return 0;
+
+	mutex_unlock(&indio_dev->mlock);
+	return -EBUSY;
+}
+EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
+
+/**
+ * iio_device_release_buffer_mode - releases claim on buffer mode
+ * @indio_dev:	the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in buffer mode.
+ *
+ * Use with iio_device_claim_buffer_mode()
+ */
+void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
+{
+	mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
+
 /**
  * iio_device_get_current_mode() - helper function providing read-only access to
  *				   the opaque @currentmode variable
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f0ec8a5e5a7a..9d3bd6379eb8 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -629,6 +629,8 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
 void iio_device_release_direct_mode(struct iio_dev *indio_dev);
+int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
+void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
 
 extern struct bus_type iio_bus_type;
 
-- 
2.37.3


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

Re: [PATCH v2 13/16] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Andy Shevchenko]

On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> These APIs are analogous to iio_device_claim_direct_mode() and
> iio_device_release_direct_mode() but, as the name suggests, with the
> logic flipped. While this looks odd enough, it will have at least two
> users (in following changes) and it will be important to move the iio
> mlock to the private struct.

...

> +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> +{
> +       mutex_lock(&indio_dev->mlock);
> +
> +       if (iio_buffer_enabled(indio_dev))

Do you need to annotate these two APIs to make sparse happy about
locking balance?

(Try to run `make W=1 C=1 ...` with your patches and look if any new
warnings appear.)

> +               return 0;
> +
> +       mutex_unlock(&indio_dev->mlock);
> +       return -EBUSY;
> +}

...

> +void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
> +{
> +       mutex_unlock(&indio_dev->mlock);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

Re: [PATCH v2 13/16] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Nuno Sá]

On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > These APIs are analogous to iio_device_claim_direct_mode() and
> > iio_device_release_direct_mode() but, as the name suggests, with
> > the
> > logic flipped. While this looks odd enough, it will have at least
> > two
> > users (in following changes) and it will be important to move the
> > iio
> > mlock to the private struct.
> 
> ...
> 
> > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> > +{
> > +       mutex_lock(&indio_dev->mlock);
> > +
> > +       if (iio_buffer_enabled(indio_dev))
> 
> Do you need to annotate these two APIs to make sparse happy about
> locking balance?
> 
> (Try to run `make W=1 C=1 ...` with your patches and look if any new
> warnings appear.)

make W=1 C=1 drivers/iio/industrialio-core.o
#  UPD     include/config/kernel.release
  UPD     include/generated/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CC      drivers/iio/industrialio-core.o
  CHECK   drivers/iio/industrialio-core.c
drivers/iio/industrialio-core.c: note: in included file (through
include/linux/bitops.h, include/linux/kernel.h):
./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning:
unreplaced symbol 'old'
./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning:
unreplaced symbol 'p'
./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning:
unreplaced symbol 'val'
./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning:
unreplaced symbol 'val'
./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning:
unreplaced symbol 'mask'
./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning:
unreplaced symbol 'return'
./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning:
unreplaced symbol 'return'
drivers/iio/industrialio-core.c:2100: warning: expecting prototype for
iio_device_claim_buffered_mode(). Prototype was for
iio_device_claim_buffer_mode() instead

Don't really see anything odd in here... Am I missing something? 

Anyways, I guess you mean annotations as __acquires() and
__releases()... Well, this API is pretty much analogous to
iio_device_claim_direct_mode() which also don't have such annotations.
Thus, I would say to add them (if we are going too) in a future patch
to both APIs...

Also fine with adding them now if Jonathan feels it's necessary.

- Nuno Sá 


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

Re: [PATCH v2 13/16] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Jonathan Cameron]

On Wed, 05 Oct 2022 10:37:39 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > These APIs are analogous to iio_device_claim_direct_mode() and
> > > iio_device_release_direct_mode() but, as the name suggests, with
> > > the
> > > logic flipped. While this looks odd enough, it will have at least
> > > two
> > > users (in following changes) and it will be important to move the
> > > iio
> > > mlock to the private struct.  
> > 
> > ...
> >   
> > > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> > > +{
> > > +       mutex_lock(&indio_dev->mlock);
> > > +
> > > +       if (iio_buffer_enabled(indio_dev))  
> > 
> > Do you need to annotate these two APIs to make sparse happy about
> > locking balance?
> > 
> > (Try to run `make W=1 C=1 ...` with your patches and look if any new
> > warnings appear.)  
> 
> make W=1 C=1 drivers/iio/industrialio-core.o
> #  UPD     include/config/kernel.release

...

> drivers/iio/industrialio-core.c:2100: warning: expecting prototype for
> iio_device_claim_buffered_mode(). Prototype was for
> iio_device_claim_buffer_mode() instead

That one wants fixing as this patch introduces it.

> 
> Don't really see anything odd in here... Am I missing something? 
> 
> Anyways, I guess you mean annotations as __acquires() and
> __releases()... Well, this API is pretty much analogous to
> iio_device_claim_direct_mode() which also don't have such annotations.
> Thus, I would say to add them (if we are going too) in a future patch
> to both APIs...
> 
> Also fine with adding them now if Jonathan feels it's necessary.

I've wondered for a while why we don't get reports as
a result of those not being annotated.  However, follow up patch
probably makes sense rather than rolling it into this series.

Jonathan
> 


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

Re: [PATCH v2 13/16] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Nuno Sá]

On Sun, 2022-10-09 at 12:41 +0100, Jonathan Cameron wrote:
> On Wed, 05 Oct 2022 10:37:39 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com>
> > > wrote:  
> > > > 
> > > > These APIs are analogous to iio_device_claim_direct_mode() and
> > > > iio_device_release_direct_mode() but, as the name suggests,
> > > > with
> > > > the
> > > > logic flipped. While this looks odd enough, it will have at
> > > > least
> > > > two
> > > > users (in following changes) and it will be important to move
> > > > the
> > > > iio
> > > > mlock to the private struct.  
> > > 
> > > ...
> > >   
> > > > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> > > > +{
> > > > +       mutex_lock(&indio_dev->mlock);
> > > > +
> > > > +       if (iio_buffer_enabled(indio_dev))  
> > > 
> > > Do you need to annotate these two APIs to make sparse happy about
> > > locking balance?
> > > 
> > > (Try to run `make W=1 C=1 ...` with your patches and look if any
> > > new
> > > warnings appear.)  
> > 
> > make W=1 C=1 drivers/iio/industrialio-core.o
> > #  UPD     include/config/kernel.release
> 
> ...
> 
> > drivers/iio/industrialio-core.c:2100: warning: expecting prototype
> > for
> > iio_device_claim_buffered_mode(). Prototype was for
> > iio_device_claim_buffer_mode() instead
> 
> That one wants fixing as this patch introduces it.
> 

Bah, That's why another pair of eyes is useful... I looked for that
warning without seeing what it was complaining about. Now, I could
finally see it :)

- Nuno Sá
> 


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

[PATCH v2 14/16] 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, let's make use of the new
iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
during the complete read.

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

diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index ad5717965223..465ce8996d35 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -387,18 +387,15 @@ 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_buffer_mode(indio_dev)) {
 			ret = -EAGAIN;
-		else {
+		} else {
 			ret = max30100_get_temp(data, val);
 			if (!ret)
 				ret = IIO_VAL_INT;
 
+			iio_device_release_buffer_mode(indio_dev);
 		}
-
-		mutex_unlock(&indio_dev->mlock);
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1;  /* 0.0625 */
-- 
2.37.3


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

Re: [PATCH v2 14/16] iio: health: max30100: do not use internal iio_dev lock

[Andy Shevchenko]

On Tue, Oct 4, 2022 at 4:49 PM 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
> allow it. Hence, let's make use of the new
> iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
> during the complete read.

...

> +               if (iio_device_claim_buffer_mode(indio_dev)) {
>                         ret = -EAGAIN;

Why is the error code shadowed? Isn't it better to return exactly the
one you resend to the upper caller(s)? Each unclear error code
shadowing should be at least explained.

> +               } else {

>                 }

-- 
With Best Regards,
Andy Shevchenko

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

Re: [PATCH v2 14/16] iio: health: max30100: do not use internal iio_dev lock

[Nuno Sá]

On Tue, 2022-10-04 at 17:13 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM 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
> > allow it. Hence, let's make use of the new
> > iio_device_claim_buffer_mode() API to make sure we stay in buffered
> > mode
> > during the complete read.
> 
> ...
> 
> > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> >                         ret = -EAGAIN;
> 
> Why is the error code shadowed? Isn't it better to return exactly the
> one you resend to the upper caller(s)? Each unclear error code
> shadowing should be at least explained.
> >           }

I'm keeping the same error that was returned before. Changing the error
code returned to userspace can break some apps relying on it. But if
everyone is ok with assuming the risk and changing it, fine by me.


- Nuno Sá

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

Re: [PATCH v2 14/16] iio: health: max30100: do not use internal iio_dev lock

[Jonathan Cameron]

On Wed, 05 Oct 2022 10:09:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:13 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM 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
> > > allow it. Hence, let's make use of the new
> > > iio_device_claim_buffer_mode() API to make sure we stay in buffered
> > > mode
> > > during the complete read.  
> > 
> > ...
> >   
> > > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> > >                         ret = -EAGAIN;  
> > 
> > Why is the error code shadowed? Isn't it better to return exactly the
> > one you resend to the upper caller(s)? Each unclear error code
> > shadowing should be at least explained.  
> > >           }  
> 
> I'm keeping the same error that was returned before. Changing the error
> code returned to userspace can break some apps relying on it. But if
> everyone is ok with assuming the risk and changing it, fine by me.

Hmm. For most drivers I'd say change it, but these weird health parts
use a highly custom userspace so it's just possible we'd break it
by changing the return code.  Unfortunately I don't know of anyone with current
access to the code of that software stack to check this for us as
there have been a lot of changes at TI in recent years.

So probably best to leave it alone, but add a comment to the patch description
to give reasoning.

Thanks,

Jonathan

> 
> 
> - Nuno Sá


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

[PATCH v2 15/16] 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, we want to
know if we are in buffered mode or not to know if the device is powered
(buffer mode) or not. And depending on that max30102_get_temp() will
power on the device if needed. Hence, in order to keep the same
functionality, we try to:

1. Claim Buffered mode;
2: If 1) succeeds call max30102_get_temp() without powering on the
   device;
3: Release Buffered mode;
4: If 1) fails, Claim Direct mode;
5: If 4) succeeds call max30102_get_temp() with powering on the device;
6: Release Direct mode;
7: If 4) fails, goto to 1) and try again.

This dance between buffered and direct mode is not particularly pretty
(as well as the loop introduced by the goto statement) but it does allow
us to get rid of the mlock usage while keeping the same behavior.

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

diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index abbcef563807..b8b5ccf582fb 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -477,12 +477,23 @@ 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))
+any_mode_retry:
+		if (iio_device_claim_buffer_mode(indio_dev)) {
+			/*
+			 * This one is a *bit* hacky. If we cannot claim buffer
+			 * mode, then try direct mode so that we make sure
+			 * things cannot concurrently change. And we just keep
+			 * trying until we get one of the modes...
+			 */
+			if (iio_device_claim_direct_mode(indio_dev))
+				goto any_mode_retry;
+
 			ret = max30102_get_temp(data, val, true);
-		else
+			iio_device_release_direct_mode(indio_dev);
+		} else {
 			ret = max30102_get_temp(data, val, false);
-		mutex_unlock(&indio_dev->mlock);
+			iio_device_release_buffer_mode(indio_dev);
+		}
 		if (ret)
 			return ret;
 
-- 
2.37.3


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

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

[Andy Shevchenko]

On Tue, Oct 4, 2022 at 4:49 PM 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, we want to
> know if we are in buffered mode or not to know if the device is powered
> (buffer mode) or not. And depending on that max30102_get_temp() will
> power on the device if needed. Hence, in order to keep the same
> functionality, we try to:
>
> 1. Claim Buffered mode;
> 2: If 1) succeeds call max30102_get_temp() without powering on the
>    device;
> 3: Release Buffered mode;
> 4: If 1) fails, Claim Direct mode;
> 5: If 4) succeeds call max30102_get_temp() with powering on the device;
> 6: Release Direct mode;
> 7: If 4) fails, goto to 1) and try again.
>
> This dance between buffered and direct mode is not particularly pretty
> (as well as the loop introduced by the goto statement) but it does allow
> us to get rid of the mlock usage while keeping the same behavior.

...

> +               if (iio_device_claim_buffer_mode(indio_dev)) {

Why is ret not used here?

> +                       /*
> +                        * This one is a *bit* hacky. If we cannot claim buffer
> +                        * mode, then try direct mode so that we make sure
> +                        * things cannot concurrently change. And we just keep
> +                        * trying until we get one of the modes...
> +                        */
> +                       if (iio_device_claim_direct_mode(indio_dev))

...and here?

> +                               goto any_mode_retry;

> +               } else {

> +               }

I.o.w. what error code will be visible to the caller and why.

-- 
With Best Regards,
Andy Shevchenko

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

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

[Nuno Sá]

On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM 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, we want
> > to
> > know if we are in buffered mode or not to know if the device is
> > powered
> > (buffer mode) or not. And depending on that max30102_get_temp()
> > will
> > power on the device if needed. Hence, in order to keep the same
> > functionality, we try to:
> > 
> > 1. Claim Buffered mode;
> > 2: If 1) succeeds call max30102_get_temp() without powering on the
> >    device;
> > 3: Release Buffered mode;
> > 4: If 1) fails, Claim Direct mode;
> > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > device;
> > 6: Release Direct mode;
> > 7: If 4) fails, goto to 1) and try again.
> > 
> > This dance between buffered and direct mode is not particularly
> > pretty
> > (as well as the loop introduced by the goto statement) but it does
> > allow
> > us to get rid of the mlock usage while keeping the same behavior.
> 
> ...
> 
> > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> 
> Why is ret not used here?
> 
> > +                       /*
> > +                        * This one is a *bit* hacky. If we cannot
> > claim buffer
> > +                        * mode, then try direct mode so that we
> > make sure
> > +                        * things cannot concurrently change. And
> > we just keep
> > +                        * trying until we get one of the modes...
> > +                        */
> > +                       if
> > (iio_device_claim_direct_mode(indio_dev))
> 
> ...and here?
> 
> > +                               goto any_mode_retry;
> 
> > +               } else {
> 
> > +               }
> 
> I.o.w. what error code will be visible to the caller and why.
> 

Note that we do not really care about the error code returned by both
iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We
just loop until we get one of the modes (thus ret = 0) so that we can
safely call one of the max30102_get_temp() variants. And that will be
the visible error code (if any). That said, you can look at the first
version of the series about this (and the previous patch) and why this
is being done like this (note that I'm also not 100% convinced about
this ping pong between getting one of the IIO modes but it's also not
that bad and if there's no major complains, I'm fine with it).

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

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

[Jonathan Cameron]

On Wed, 05 Oct 2022 10:17:00 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM 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, we want
> > > to
> > > know if we are in buffered mode or not to know if the device is
> > > powered
> > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > will
> > > power on the device if needed. Hence, in order to keep the same
> > > functionality, we try to:
> > > 
> > > 1. Claim Buffered mode;
> > > 2: If 1) succeeds call max30102_get_temp() without powering on the
> > >    device;
> > > 3: Release Buffered mode;
> > > 4: If 1) fails, Claim Direct mode;
> > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > device;
> > > 6: Release Direct mode;
> > > 7: If 4) fails, goto to 1) and try again.
> > > 
> > > This dance between buffered and direct mode is not particularly
> > > pretty
> > > (as well as the loop introduced by the goto statement) but it does
> > > allow
> > > us to get rid of the mlock usage while keeping the same behavior.  
> > 
> > ...
> >   
> > > +               if (iio_device_claim_buffer_mode(indio_dev)) {  
> > 
> > Why is ret not used here?
> >   
> > > +                       /*
> > > +                        * This one is a *bit* hacky. If we cannot
> > > claim buffer
> > > +                        * mode, then try direct mode so that we
> > > make sure
> > > +                        * things cannot concurrently change. And
> > > we just keep
> > > +                        * trying until we get one of the modes...
> > > +                        */
> > > +                       if
> > > (iio_device_claim_direct_mode(indio_dev))  
> > 
> > ...and here?
> >   
> > > +                               goto any_mode_retry;  
> >   
> > > +               } else {  
> >   
> > > +               }  
> > 
> > I.o.w. what error code will be visible to the caller and why.
> >   
> 
> Note that we do not really care about the error code returned by both
> iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We
> just loop until we get one of the modes (thus ret = 0) so that we can
> safely call one of the max30102_get_temp() variants. And that will be
> the visible error code (if any). That said, you can look at the first
> version of the series about this (and the previous patch) and why this
> is being done like this (note that I'm also not 100% convinced about
> this ping pong between getting one of the IIO modes but it's also not
> that bad and if there's no major complains, I'm fine with it).

This is a vanishingly rare corner case and not in a remotely high performance
path so I'm not keen on introducing a more complex API just to handle
this corner. If we added a means to get the lock independent of mode
we'd have an interface that is far to likely to get missused.
What you have here does the job and looks nasty enough to put people off
copying it unless they really need it!

Jonathan

> 
> - Nuno Sá


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

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

[Jonathan Cameron]

On Sun, 9 Oct 2022 12:44:40 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 05 Oct 2022 10:17:00 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote:  
> > > On Tue, Oct 4, 2022 at 4:49 PM 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, we want
> > > > to
> > > > know if we are in buffered mode or not to know if the device is
> > > > powered
> > > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > > will
> > > > power on the device if needed. Hence, in order to keep the same
> > > > functionality, we try to:
> > > > 
> > > > 1. Claim Buffered mode;
> > > > 2: If 1) succeeds call max30102_get_temp() without powering on the
> > > >    device;
> > > > 3: Release Buffered mode;
> > > > 4: If 1) fails, Claim Direct mode;
> > > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > > device;
> > > > 6: Release Direct mode;
> > > > 7: If 4) fails, goto to 1) and try again.
> > > > 
> > > > This dance between buffered and direct mode is not particularly
> > > > pretty
> > > > (as well as the loop introduced by the goto statement) but it does
> > > > allow
> > > > us to get rid of the mlock usage while keeping the same behavior.    
> > > 
> > > ...
> > >     
> > > > +               if (iio_device_claim_buffer_mode(indio_dev)) {    
> > > 
> > > Why is ret not used here?
> > >     
> > > > +                       /*
> > > > +                        * This one is a *bit* hacky. If we cannot
> > > > claim buffer
> > > > +                        * mode, then try direct mode so that we
> > > > make sure
> > > > +                        * things cannot concurrently change. And
> > > > we just keep
> > > > +                        * trying until we get one of the modes...
> > > > +                        */
> > > > +                       if
> > > > (iio_device_claim_direct_mode(indio_dev))    
> > > 
> > > ...and here?
> > >     
> > > > +                               goto any_mode_retry;    
> > >     
> > > > +               } else {    
> > >     
> > > > +               }    
> > > 
> > > I.o.w. what error code will be visible to the caller and why.
> > >     
> > 
> > Note that we do not really care about the error code returned by both
> > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We
> > just loop until we get one of the modes (thus ret = 0) so that we can
> > safely call one of the max30102_get_temp() variants. And that will be
> > the visible error code (if any). That said, you can look at the first
> > version of the series about this (and the previous patch) and why this
> > is being done like this (note that I'm also not 100% convinced about
> > this ping pong between getting one of the IIO modes but it's also not
> > that bad and if there's no major complains, I'm fine with it).  
> 
> This is a vanishingly rare corner case and not in a remotely high performance
> path so I'm not keen on introducing a more complex API just to handle
> this corner. If we added a means to get the lock independent of mode
> we'd have an interface that is far to likely to get missused.
> What you have here does the job and looks nasty enough to put people off
> copying it unless they really need it!
> 
> Jonathan
> 
I should probably have said lgtm for how you have it here.

Jonathan



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

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

[Nuno Sá]

On Sun, 2022-10-09 at 12:44 +0100, Jonathan Cameron wrote:
> On Wed, 05 Oct 2022 10:17:00 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 4, 2022 at 4:49 PM 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, we
> > > > want
> > > > to
> > > > know if we are in buffered mode or not to know if the device is
> > > > powered
> > > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > > will
> > > > power on the device if needed. Hence, in order to keep the same
> > > > functionality, we try to:
> > > > 
> > > > 1. Claim Buffered mode;
> > > > 2: If 1) succeeds call max30102_get_temp() without powering on
> > > > the
> > > >    device;
> > > > 3: Release Buffered mode;
> > > > 4: If 1) fails, Claim Direct mode;
> > > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > > device;
> > > > 6: Release Direct mode;
> > > > 7: If 4) fails, goto to 1) and try again.
> > > > 
> > > > This dance between buffered and direct mode is not particularly
> > > > pretty
> > > > (as well as the loop introduced by the goto statement) but it
> > > > does
> > > > allow
> > > > us to get rid of the mlock usage while keeping the same
> > > > behavior.  
> > > 
> > > ...
> > >   
> > > > +               if (iio_device_claim_buffer_mode(indio_dev)) { 
> > > 
> > > Why is ret not used here?
> > >   
> > > > +                       /*
> > > > +                        * This one is a *bit* hacky. If we
> > > > cannot
> > > > claim buffer
> > > > +                        * mode, then try direct mode so that
> > > > we
> > > > make sure
> > > > +                        * things cannot concurrently change.
> > > > And
> > > > we just keep
> > > > +                        * trying until we get one of the
> > > > modes...
> > > > +                        */
> > > > +                       if
> > > > (iio_device_claim_direct_mode(indio_dev))  
> > > 
> > > ...and here?
> > >   
> > > > +                               goto any_mode_retry;  
> > >   
> > > > +               } else {  
> > >   
> > > > +               }  
> > > 
> > > I.o.w. what error code will be visible to the caller and why.
> > >   
> > 
> > Note that we do not really care about the error code returned by
> > both
> > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode().
> > We
> > just loop until we get one of the modes (thus ret = 0) so that we
> > can
> > safely call one of the max30102_get_temp() variants. And that will
> > be
> > the visible error code (if any). That said, you can look at the
> > first
> > version of the series about this (and the previous patch) and why
> > this
> > is being done like this (note that I'm also not 100% convinced
> > about
> > this ping pong between getting one of the IIO modes but it's also
> > not
> > that bad and if there's no major complains, I'm fine with it).
> 
> This is a vanishingly rare corner case and not in a remotely high
> performance
> path so I'm not keen on introducing a more complex API just to handle
> this corner. If we added a means to get the lock independent of mode
> we'd have an interface that is far to likely to get missused.

Totally agree. It crossed my mind to have an helper for getting the
lock but that would defeat the purpose of this patchset! Well, I'm also
fine with the code as it stands so I'll leave it for v3 and if no one
complains I guess we're good to go... 


- Nuno Sá

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

[PATCH v2 16/16] 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    | 26 +++++++++++++++-----------
 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, 42 insertions(+), 37 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 c23d3abb33c5..ebbc64e4f673 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);
 
@@ -2096,12 +2098,14 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
  */
 int iio_device_claim_buffer_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))
 		return 0;
 
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return -EBUSY;
 }
 EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
@@ -2117,7 +2121,7 @@ EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
  */
 void iio_device_release_buffer_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_buffer_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 9d3bd6379eb8..8e0afaaa3f75 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Andy Shevchenko]

On Tue, Oct 4, 2022 at 4:49 PM 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.

use this


I like the end result!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

P.S. Shouldn't we annotate the respective APIs with might_sleep() and
Co (if it's not done yet)?

PPS Reading to the topic:
https://blog.ffwll.ch/2022/07/locking-engineering.html
https://blog.ffwll.ch/2022/08/locking-hierarchy.html
https://blog.ffwll.ch/2020/08/lockdep-false-positives.html


> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/TODO                   |  3 ---
>  drivers/iio/industrialio-buffer.c  | 29 +++++++++++++++++------------
>  drivers/iio/industrialio-core.c    | 26 +++++++++++++++-----------
>  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, 42 insertions(+), 37 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 c23d3abb33c5..ebbc64e4f673 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);
>
> @@ -2096,12 +2098,14 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
>   */
>  int iio_device_claim_buffer_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))
>                 return 0;
>
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>         return -EBUSY;
>  }
>  EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
> @@ -2117,7 +2121,7 @@ EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
>   */
>  void iio_device_release_buffer_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_buffer_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 9d3bd6379eb8..8e0afaaa3f75 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
>


-- 
With Best Regards,
Andy Shevchenko

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

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

[Nuno Sá]

On Tue, 2022-10-04 at 17:21 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM 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.
> 
> use this
> 
> 
> I like the end result!
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> P.S. Shouldn't we annotate the respective APIs with might_sleep() and
> Co (if it's not done yet)?
> 
> 

Hmm, I would say this is the same story as with sparse annotations... I
guess, at least, might_sleep() would make sense but I think we should
probably do it for the complete IIO subsystem where it makes sense
instead of having it in just this new API.

- Nuno Sá 


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

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

[Jonathan Cameron]

On Wed, 05 Oct 2022 10:40:03 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:21 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM 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.  
> > 
> > use this
> > 
> > 
> > I like the end result!
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 
> > P.S. Shouldn't we annotate the respective APIs with might_sleep() and
> > Co (if it's not done yet)?
> > 
> >   
> 
> Hmm, I would say this is the same story as with sparse annotations... I
> guess, at least, might_sleep() would make sense but I think we should
> probably do it for the complete IIO subsystem where it makes sense
> instead of having it in just this new API.

We definitely could add such annotations.

From a documentation point of view that might be useful. 
From a protection / bug catching point of view the calls to mutex_lock()
should fire off much the same error reports, just one level down.

Jonathan

> 
> - Nuno Sá 
> 


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

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

[Jonathan Cameron]

On Tue, 4 Oct 2022 17:21:20 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Oct 4, 2022 at 4:49 PM 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.  
> 
> use this
> 
> 
> I like the end result!
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Hi Andy,

Just to check, just this patch, or series (where you haven't commented?)

I'm going to queue this series up piecewise just because it's fairly
big and most of it is completely non controversial!  So for now I've
not applied your RB, but can do so before I push out as anything non
rebasing (which I won't do until after the merge window).

Thanks,

Jonathan

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

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

[Andy Shevchenko]

On Sun, Oct 9, 2022 at 2:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 4 Oct 2022 17:21:20 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > I like the end result!
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Just to check, just this patch, or series (where you haven't commented?)

Just for this patch.

> I'm going to queue this series up piecewise just because it's fairly
> big and most of it is completely non controversial!  So for now I've
> not applied your RB, but can do so before I push out as anything non
> rebasing (which I won't do until after the merge window).

-- 
With Best Regards,
Andy Shevchenko

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

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

[Jonathan Cameron]

On Tue, 4 Oct 2022 15:49:09 +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>
This looks good to me. 
So series wise, I think I've picked up 1-12. 
To resolve is remaining discussion plus the missmatch in function name vs
comment in 13.

Thanks for getting this tidied up!  It's the end of a very long process.
I almost feel like we need a party - there would be a quite a crowd given
how many people have been involved in this effort over the years. :)

Jonathan

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

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

[Nuno Sá]

On Sun, 2022-10-09 at 13:19 +0100, Jonathan Cameron wrote:
> On Tue, 4 Oct 2022 15:49:09 +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>
> This looks good to me. 
> So series wise, I think I've picked up 1-12. 
> To resolve is remaining discussion plus the missmatch in function
> name vs
> comment in 13.
> 

So, I think we are missing an improvement on the commit message for
patch 14 (about shadowing the error code) and fixing the function
name..
> Thanks for getting this tidied up!  It's the end of a very long
> process.
> I almost feel like we need a party - there would be a quite a crowd
> given
> how many people have been involved in this effort over the years. :)
> 

Sure, happy to help! This one felt like something that we really need
to take care :)

- Nuno Sá


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