* [PATCH 2/5] iio: adc: palmas_gpadc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock Mircea Caprioru
@ 2020-09-28 13:13 ` Mircea Caprioru
2021-02-21 16:38 ` Jonathan Cameron
2020-09-28 13:13 ` [PATCH 3/5] iio: adc: npcm_adc: " Mircea Caprioru
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Mircea Caprioru @ 2020-09-28 13:13 UTC (permalink / raw)
To: jic23
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean, Mircea Caprioru
From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
As part of the general cleanup of indio_dev->mlock, this change replaces
it with a local lock on the device's state structure.
This is part of a bigger cleanup.
Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
---
drivers/iio/adc/palmas_gpadc.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 889b88768b63..14874f11614d 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -90,6 +90,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = {
* 3: 800 uA
* @extended_delay: enable the gpadc extended delay mode
* @auto_conversion_period: define the auto_conversion_period
+ * @lock: Lock to protect the device state during a potential concurrent
+ * read access from userspace. Reading a raw value requires a sequence
+ * of register writes, then a wait for a completion callback,
+ * and finally a register read, during which userspace could issue
+ * another read request. This lock protects a read access from
+ * ocurring before another one has finished.
*
* This is the palmas_gpadc structure to store run-time information
* and pointers for this driver instance.
@@ -110,6 +116,7 @@ struct palmas_gpadc {
bool wakeup1_enable;
bool wakeup2_enable;
int auto_conversion_period;
+ struct mutex lock;
};
/*
@@ -388,7 +395,7 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
if (adc_chan > PALMAS_ADC_CH_MAX)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&adc->lock);
switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -414,12 +421,12 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
goto out;
}
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&adc->lock);
return ret;
out:
palmas_gpadc_read_done(adc, adc_chan);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&adc->lock);
return ret;
}
@@ -516,6 +523,9 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
adc->dev = &pdev->dev;
adc->palmas = dev_get_drvdata(pdev->dev.parent);
adc->adc_info = palmas_gpadc_info;
+
+ mutex_init(&adc->lock);
+
init_completion(&adc->conv_completion);
dev_set_drvdata(&pdev->dev, indio_dev);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/5] iio: adc: palmas_gpadc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 ` [PATCH 2/5] iio: adc: palmas_gpadc: " Mircea Caprioru
@ 2021-02-21 16:38 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-02-21 16:38 UTC (permalink / raw)
To: Mircea Caprioru
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean
On Mon, 28 Sep 2020 16:13:30 +0300
Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
> This is part of a bigger cleanup.
> Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
Applied,
Thanks,
Jonathan
> ---
> drivers/iio/adc/palmas_gpadc.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> index 889b88768b63..14874f11614d 100644
> --- a/drivers/iio/adc/palmas_gpadc.c
> +++ b/drivers/iio/adc/palmas_gpadc.c
> @@ -90,6 +90,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = {
> * 3: 800 uA
> * @extended_delay: enable the gpadc extended delay mode
> * @auto_conversion_period: define the auto_conversion_period
> + * @lock: Lock to protect the device state during a potential concurrent
> + * read access from userspace. Reading a raw value requires a sequence
> + * of register writes, then a wait for a completion callback,
> + * and finally a register read, during which userspace could issue
> + * another read request. This lock protects a read access from
> + * ocurring before another one has finished.
> *
> * This is the palmas_gpadc structure to store run-time information
> * and pointers for this driver instance.
> @@ -110,6 +116,7 @@ struct palmas_gpadc {
> bool wakeup1_enable;
> bool wakeup2_enable;
> int auto_conversion_period;
> + struct mutex lock;
> };
>
> /*
> @@ -388,7 +395,7 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> if (adc_chan > PALMAS_ADC_CH_MAX)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&adc->lock);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> @@ -414,12 +421,12 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> goto out;
> }
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&adc->lock);
> return ret;
>
> out:
> palmas_gpadc_read_done(adc, adc_chan);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&adc->lock);
>
> return ret;
> }
> @@ -516,6 +523,9 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> adc->dev = &pdev->dev;
> adc->palmas = dev_get_drvdata(pdev->dev.parent);
> adc->adc_info = palmas_gpadc_info;
> +
> + mutex_init(&adc->lock);
> +
> init_completion(&adc->conv_completion);
> dev_set_drvdata(&pdev->dev, indio_dev);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] iio: adc: npcm_adc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock Mircea Caprioru
2020-09-28 13:13 ` [PATCH 2/5] iio: adc: palmas_gpadc: " Mircea Caprioru
@ 2020-09-28 13:13 ` Mircea Caprioru
2021-02-21 16:39 ` Jonathan Cameron
2020-09-28 13:13 ` [PATCH 4/5] iio: adc: vf610_adc: " Mircea Caprioru
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Mircea Caprioru @ 2020-09-28 13:13 UTC (permalink / raw)
To: jic23
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean, Mircea Caprioru
From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
As part of the general cleanup of indio_dev->mlock, this change replaces
it with a local lock on the device's state structure.
This is part of a bigger cleanup.
Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
---
drivers/iio/adc/npcm_adc.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/npcm_adc.c b/drivers/iio/adc/npcm_adc.c
index d9d105920001..f7bc0bb7f112 100644
--- a/drivers/iio/adc/npcm_adc.c
+++ b/drivers/iio/adc/npcm_adc.c
@@ -25,6 +25,15 @@ struct npcm_adc {
wait_queue_head_t wq;
struct regulator *vref;
struct reset_control *reset;
+ /*
+ * Lock to protect the device state during a potential concurrent
+ * read access from userspace. Reading a raw value requires a sequence
+ * of register writes, then a wait for a event and finally a register
+ * read, during which userspace could issue another read request.
+ * This lock protects a read access from ocurring before another one
+ * has finished.
+ */
+ struct mutex lock;
};
/* ADC registers */
@@ -135,9 +144,9 @@ static int npcm_adc_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&info->lock);
ret = npcm_adc_read(info, val, chan->channel);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
if (ret) {
dev_err(info->dev, "NPCM ADC read failed\n");
return ret;
@@ -187,6 +196,8 @@ static int npcm_adc_probe(struct platform_device *pdev)
return -ENOMEM;
info = iio_priv(indio_dev);
+ mutex_init(&info->lock);
+
info->dev = &pdev->dev;
info->regs = devm_platform_ioremap_resource(pdev, 0);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/5] iio: adc: npcm_adc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 ` [PATCH 3/5] iio: adc: npcm_adc: " Mircea Caprioru
@ 2021-02-21 16:39 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-02-21 16:39 UTC (permalink / raw)
To: Mircea Caprioru
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean
On Mon, 28 Sep 2020 16:13:31 +0300
Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
> This is part of a bigger cleanup.
> Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
Applied,
thanks,
Jonathan
> ---
> drivers/iio/adc/npcm_adc.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/npcm_adc.c b/drivers/iio/adc/npcm_adc.c
> index d9d105920001..f7bc0bb7f112 100644
> --- a/drivers/iio/adc/npcm_adc.c
> +++ b/drivers/iio/adc/npcm_adc.c
> @@ -25,6 +25,15 @@ struct npcm_adc {
> wait_queue_head_t wq;
> struct regulator *vref;
> struct reset_control *reset;
> + /*
> + * Lock to protect the device state during a potential concurrent
> + * read access from userspace. Reading a raw value requires a sequence
> + * of register writes, then a wait for a event and finally a register
> + * read, during which userspace could issue another read request.
> + * This lock protects a read access from ocurring before another one
> + * has finished.
> + */
> + struct mutex lock;
> };
>
> /* ADC registers */
> @@ -135,9 +144,9 @@ static int npcm_adc_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&info->lock);
> ret = npcm_adc_read(info, val, chan->channel);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> if (ret) {
> dev_err(info->dev, "NPCM ADC read failed\n");
> return ret;
> @@ -187,6 +196,8 @@ static int npcm_adc_probe(struct platform_device *pdev)
> return -ENOMEM;
> info = iio_priv(indio_dev);
>
> + mutex_init(&info->lock);
> +
> info->dev = &pdev->dev;
>
> info->regs = devm_platform_ioremap_resource(pdev, 0);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/5] iio: adc: vf610_adc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock Mircea Caprioru
2020-09-28 13:13 ` [PATCH 2/5] iio: adc: palmas_gpadc: " Mircea Caprioru
2020-09-28 13:13 ` [PATCH 3/5] iio: adc: npcm_adc: " Mircea Caprioru
@ 2020-09-28 13:13 ` Mircea Caprioru
2020-09-29 16:14 ` Jonathan Cameron
2020-09-28 13:13 ` [PATCH 5/5] iio: adc: rockchip_saradc: " Mircea Caprioru
2021-02-21 16:37 ` [PATCH 1/5] iio: adc: spear_adc: " Jonathan Cameron
4 siblings, 1 reply; 12+ messages in thread
From: Mircea Caprioru @ 2020-09-28 13:13 UTC (permalink / raw)
To: jic23
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean, Mircea Caprioru
From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
As part of the general cleanup of indio_dev->mlock, this change replaces
it with a local lock on the device's state structure.
This is part of a bigger cleanup.
Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
---
drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 1d794cf3e3f1..b7d583993f0b 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -168,6 +168,15 @@ struct vf610_adc {
struct completion completion;
u16 buffer[8];
+ /*
+ * Lock to protect the device state during a potential concurrent
+ * read access from userspace. Reading a raw value requires a sequence
+ * of register writes, then a wait for a completion callback,
+ * and finally a register read, during which userspace could issue
+ * another read request. This lock protects a read access from
+ * ocurring before another one has finished.
+ */
+ struct mutex lock;
};
static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
@@ -464,11 +473,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;
}
@@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
case IIO_CHAN_INFO_PROCESSED:
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&info->lock);
if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
return -EBUSY;
}
@@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
ret = wait_for_completion_interruptible_timeout
(&info->completion, VF610_ADC_TIMEOUT);
if (ret == 0) {
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
return -ETIMEDOUT;
}
if (ret < 0) {
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
return ret;
}
@@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
break;
default:
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
return -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&info->lock);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
@@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
}
info = iio_priv(indio_dev);
+
+ mutex_init(&info->lock);
+
info->dev = &pdev->dev;
info->regs = devm_platform_ioremap_resource(pdev, 0);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/5] iio: adc: vf610_adc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 ` [PATCH 4/5] iio: adc: vf610_adc: " Mircea Caprioru
@ 2020-09-29 16:14 ` Jonathan Cameron
2020-09-30 5:57 ` Alexandru Ardelean
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2020-09-29 16:14 UTC (permalink / raw)
To: Mircea Caprioru
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean
On Mon, 28 Sep 2020 16:13:32 +0300
Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
> This is part of a bigger cleanup.
> Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
There are more problems in the locking in here than just this one.
See below. The taking of mlock like this was what originally motivated
the efforts to hide it away from drivers.
In this particular case I don't think a local lock is the correct solution.
Thanks,
Jonathan
> ---
> drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 1d794cf3e3f1..b7d583993f0b 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -168,6 +168,15 @@ struct vf610_adc {
>
> struct completion completion;
> u16 buffer[8];
Side note. That buffer isn't correctly aligned. I'll add this one to
my next series fixing those.
> + /*
> + * Lock to protect the device state during a potential concurrent
> + * read access from userspace. Reading a raw value requires a sequence
> + * of register writes, then a wait for a completion callback,
> + * and finally a register read, during which userspace could issue
> + * another read request. This lock protects a read access from
> + * ocurring before another one has finished.
> + */
> + struct mutex lock;
> };
>
> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> @@ -464,11 +473,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);
Hmm. So there is a bit of a question on what the locking here is doing.
(see below for a different use of mlock).
What it will do currently is to prevent the conversion mode changing whilst
we are in buffered mode. It will also protect against concurrent
calls of this function.
I would replace this with iio_device_claim_direct_mode() rather than a
local 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;
> }
> @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> case IIO_CHAN_INFO_PROCESSED:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&info->lock);
> if (iio_buffer_enabled(indio_dev)) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
Should be use iio_device_claim_direct_mode()
mlock is being taken here to stop us entering buffered mode.
Whilst I'd rather a driver didn't rely on internal details of
IIO, it is rather fiddly to get the locking right when there is a completion
going on, so I think here you are safe to do so.
> return -EBUSY;
> }
>
> @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> ret = wait_for_completion_interruptible_timeout
> (&info->completion, VF610_ADC_TIMEOUT);
> if (ret == 0) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> return -ETIMEDOUT;
> }
> if (ret < 0) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> return ret;
> }
>
> @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>
> break;
> default:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> return -EINVAL;
> }
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
> }
>
> info = iio_priv(indio_dev);
> +
> + mutex_init(&info->lock);
> +
> info->dev = &pdev->dev;
>
> info->regs = devm_platform_ioremap_resource(pdev, 0);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 4/5] iio: adc: vf610_adc: Replace indio_dev->mlock with own device lock
2020-09-29 16:14 ` Jonathan Cameron
@ 2020-09-30 5:57 ` Alexandru Ardelean
2020-09-30 10:49 ` Jonathan Cameron
0 siblings, 1 reply; 12+ messages in thread
From: Alexandru Ardelean @ 2020-09-30 5:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Mircea Caprioru, Hennerich, Michael, Alexandru Ardelean,
Lars-Peter Clausen, Greg Kroah-Hartman, LKML, linux-iio,
Sergiu Cuciurean
On Tue, Sep 29, 2020 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 28 Sep 2020 16:13:32 +0300
> Mircea Caprioru <mircea.caprioru@analog.com> wrote:
>
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> >
> > As part of the general cleanup of indio_dev->mlock, this change replaces
> > it with a local lock on the device's state structure.
> >
> > This is part of a bigger cleanup.
> > Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
> >
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
>
> There are more problems in the locking in here than just this one.
> See below. The taking of mlock like this was what originally motivated
> the efforts to hide it away from drivers.
>
> In this particular case I don't think a local lock is the correct solution.
>
> Thanks,
>
> Jonathan
>
>
> > ---
> > drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> > index 1d794cf3e3f1..b7d583993f0b 100644
> > --- a/drivers/iio/adc/vf610_adc.c
> > +++ b/drivers/iio/adc/vf610_adc.c
> > @@ -168,6 +168,15 @@ struct vf610_adc {
> >
> > struct completion completion;
> > u16 buffer[8];
>
> Side note. That buffer isn't correctly aligned. I'll add this one to
> my next series fixing those.
>
> > + /*
> > + * Lock to protect the device state during a potential concurrent
> > + * read access from userspace. Reading a raw value requires a sequence
> > + * of register writes, then a wait for a completion callback,
> > + * and finally a register read, during which userspace could issue
> > + * another read request. This lock protects a read access from
> > + * ocurring before another one has finished.
> > + */
> > + struct mutex lock;
> > };
> >
> > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> > @@ -464,11 +473,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);
> Hmm. So there is a bit of a question on what the locking here is doing.
> (see below for a different use of mlock).
>
> What it will do currently is to prevent the conversion mode changing whilst
> we are in buffered mode. It will also protect against concurrent
> calls of this function.
>
> I would replace this with iio_device_claim_direct_mode() rather than a
> local lock.
This raises a new question: if there's any drivers that we missed [for
iio_device_claim_direct_mode()].
While I was aware of iio_device_claim_direct_mode(), I missed this
fact when pushing the mlock cleanup.
Oh well, I'll do a quick audit over the current drivers that were converted.
Hopefully I don't find anything :P
>
> > 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;
> > }
> > @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > case IIO_CHAN_INFO_PROCESSED:
> > - mutex_lock(&indio_dev->mlock);
> > + mutex_lock(&info->lock);
> > if (iio_buffer_enabled(indio_dev)) {
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&info->lock);
>
> Should be use iio_device_claim_direct_mode()
>
> mlock is being taken here to stop us entering buffered mode.
>
> Whilst I'd rather a driver didn't rely on internal details of
> IIO, it is rather fiddly to get the locking right when there is a completion
> going on, so I think here you are safe to do so.
>
> > return -EBUSY;
> > }
> >
> > @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > ret = wait_for_completion_interruptible_timeout
> > (&info->completion, VF610_ADC_TIMEOUT);
> > if (ret == 0) {
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&info->lock);
> > return -ETIMEDOUT;
> > }
> > if (ret < 0) {
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&info->lock);
> > return ret;
> > }
> >
> > @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> >
> > break;
> > default:
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&info->lock);
> > return -EINVAL;
> > }
> >
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&info->lock);
> > return IIO_VAL_INT;
> >
> > case IIO_CHAN_INFO_SCALE:
> > @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
> > }
> >
> > info = iio_priv(indio_dev);
> > +
> > + mutex_init(&info->lock);
> > +
> > info->dev = &pdev->dev;
> >
> > info->regs = devm_platform_ioremap_resource(pdev, 0);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 4/5] iio: adc: vf610_adc: Replace indio_dev->mlock with own device lock
2020-09-30 5:57 ` Alexandru Ardelean
@ 2020-09-30 10:49 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2020-09-30 10:49 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: Jonathan Cameron, Mircea Caprioru, Hennerich, Michael,
Alexandru Ardelean, Lars-Peter Clausen, Greg Kroah-Hartman, LKML,
linux-iio, Sergiu Cuciurean
On Wed, 30 Sep 2020 08:57:55 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> On Tue, Sep 29, 2020 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 28 Sep 2020 16:13:32 +0300
> > Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> >
> > > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > >
> > > As part of the general cleanup of indio_dev->mlock, this change replaces
> > > it with a local lock on the device's state structure.
> > >
> > > This is part of a bigger cleanup.
> > > Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
> > >
> > > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
> >
> > There are more problems in the locking in here than just this one.
> > See below. The taking of mlock like this was what originally motivated
> > the efforts to hide it away from drivers.
> >
> > In this particular case I don't think a local lock is the correct solution.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > > ---
> > > drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++--------
> > > 1 file changed, 20 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> > > index 1d794cf3e3f1..b7d583993f0b 100644
> > > --- a/drivers/iio/adc/vf610_adc.c
> > > +++ b/drivers/iio/adc/vf610_adc.c
> > > @@ -168,6 +168,15 @@ struct vf610_adc {
> > >
> > > struct completion completion;
> > > u16 buffer[8];
> >
> > Side note. That buffer isn't correctly aligned. I'll add this one to
> > my next series fixing those.
> >
> > > + /*
> > > + * Lock to protect the device state during a potential concurrent
> > > + * read access from userspace. Reading a raw value requires a sequence
> > > + * of register writes, then a wait for a completion callback,
> > > + * and finally a register read, during which userspace could issue
> > > + * another read request. This lock protects a read access from
> > > + * ocurring before another one has finished.
> > > + */
> > > + struct mutex lock;
> > > };
> > >
> > > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> > > @@ -464,11 +473,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);
> > Hmm. So there is a bit of a question on what the locking here is doing.
> > (see below for a different use of mlock).
> >
> > What it will do currently is to prevent the conversion mode changing whilst
> > we are in buffered mode. It will also protect against concurrent
> > calls of this function.
> >
> > I would replace this with iio_device_claim_direct_mode() rather than a
> > local lock.
>
> This raises a new question: if there's any drivers that we missed [for
> iio_device_claim_direct_mode()].
> While I was aware of iio_device_claim_direct_mode(), I missed this
> fact when pushing the mlock cleanup.
>
> Oh well, I'll do a quick audit over the current drivers that were converted.
> Hopefully I don't find anything :P
I was keeping an eye out for this, so hopefully I didn't miss any!
Good to check though :)
Jonathan
>
> >
> > > 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;
> > > }
> > > @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > > switch (mask) {
> > > case IIO_CHAN_INFO_RAW:
> > > case IIO_CHAN_INFO_PROCESSED:
> > > - mutex_lock(&indio_dev->mlock);
> > > + mutex_lock(&info->lock);
> > > if (iio_buffer_enabled(indio_dev)) {
> > > - mutex_unlock(&indio_dev->mlock);
> > > + mutex_unlock(&info->lock);
> >
> > Should be use iio_device_claim_direct_mode()
> >
> > mlock is being taken here to stop us entering buffered mode.
> >
> > Whilst I'd rather a driver didn't rely on internal details of
> > IIO, it is rather fiddly to get the locking right when there is a completion
> > going on, so I think here you are safe to do so.
> >
> > > return -EBUSY;
> > > }
> > >
> > > @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > > ret = wait_for_completion_interruptible_timeout
> > > (&info->completion, VF610_ADC_TIMEOUT);
> > > if (ret == 0) {
> > > - mutex_unlock(&indio_dev->mlock);
> > > + mutex_unlock(&info->lock);
> > > return -ETIMEDOUT;
> > > }
> > > if (ret < 0) {
> > > - mutex_unlock(&indio_dev->mlock);
> > > + mutex_unlock(&info->lock);
> > > return ret;
> > > }
> > >
> > > @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > >
> > > break;
> > > default:
> > > - mutex_unlock(&indio_dev->mlock);
> > > + mutex_unlock(&info->lock);
> > > return -EINVAL;
> > > }
> > >
> > > - mutex_unlock(&indio_dev->mlock);
> > > + mutex_unlock(&info->lock);
> > > return IIO_VAL_INT;
> > >
> > > case IIO_CHAN_INFO_SCALE:
> > > @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
> > > }
> > >
> > > info = iio_priv(indio_dev);
> > > +
> > > + mutex_init(&info->lock);
> > > +
> > > info->dev = &pdev->dev;
> > >
> > > info->regs = devm_platform_ioremap_resource(pdev, 0);
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] iio: adc: rockchip_saradc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock Mircea Caprioru
` (2 preceding siblings ...)
2020-09-28 13:13 ` [PATCH 4/5] iio: adc: vf610_adc: " Mircea Caprioru
@ 2020-09-28 13:13 ` Mircea Caprioru
2020-09-29 16:23 ` Jonathan Cameron
2021-02-21 16:37 ` [PATCH 1/5] iio: adc: spear_adc: " Jonathan Cameron
4 siblings, 1 reply; 12+ messages in thread
From: Mircea Caprioru @ 2020-09-28 13:13 UTC (permalink / raw)
To: jic23
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean, Mircea Caprioru
From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
As part of the general cleanup of indio_dev->mlock, this change replaces
it with a local lock on the device's state structure.
This is part of a bigger cleanup.
Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
---
drivers/iio/adc/rockchip_saradc.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 1f3d7d639d37..80084c526cc6 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -53,6 +53,15 @@ struct rockchip_saradc {
const struct rockchip_saradc_data *data;
u16 last_val;
const struct iio_chan_spec *last_chan;
+ /*
+ * Lock to protect the device state during a potential concurrent
+ * read access from userspace. Reading a raw value requires a sequence
+ * of register writes, then a wait for a completion callback,
+ * and finally a register read, during which userspace could issue
+ * another read request. This lock protects a read access from
+ * ocurring before another one has finished.
+ */
+ struct mutex lock;
};
static void rockchip_saradc_power_down(struct rockchip_saradc *info)
@@ -92,17 +101,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:
ret = regulator_get_voltage(info->vref);
@@ -254,7 +263,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];
@@ -271,7 +280,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
iio_push_to_buffers_with_timestamp(i_dev, &data, iio_get_time_ns(i_dev));
out:
- mutex_unlock(&i_dev->mlock);
+ mutex_unlock(&info->lock);
iio_trigger_notify_done(i_dev->trig);
@@ -332,6 +341,8 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
info->reset = NULL;
}
+ mutex_init(&info->lock);
+
init_completion(&info->completion);
irq = platform_get_irq(pdev, 0);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 5/5] iio: adc: rockchip_saradc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 ` [PATCH 5/5] iio: adc: rockchip_saradc: " Mircea Caprioru
@ 2020-09-29 16:23 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2020-09-29 16:23 UTC (permalink / raw)
To: Mircea Caprioru
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean
On Mon, 28 Sep 2020 16:13:33 +0300
Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
> This is part of a bigger cleanup.
> Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
Another driver with buffered support which means we need to think
harder about what is going on with these locks.
> ---
> drivers/iio/adc/rockchip_saradc.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 1f3d7d639d37..80084c526cc6 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -53,6 +53,15 @@ struct rockchip_saradc {
> const struct rockchip_saradc_data *data;
> u16 last_val;
> const struct iio_chan_spec *last_chan;
> + /*
> + * Lock to protect the device state during a potential concurrent
> + * read access from userspace. Reading a raw value requires a sequence
> + * of register writes, then a wait for a completion callback,
> + * and finally a register read, during which userspace could issue
> + * another read request. This lock protects a read access from
> + * ocurring before another one has finished.
> + */
> + struct mutex lock;
> };
>
> static void rockchip_saradc_power_down(struct rockchip_saradc *info)
> @@ -92,17 +101,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);
Probably a case for iio_device_claim_direct_mode() as I doubt we want
to enter buffered mode when halfway through this.
>
> 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:
> ret = regulator_get_voltage(info->vref);
> @@ -254,7 +263,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);
Hmm. I wonder what this was meant to protect?
I can't see why we need it if we are claiming direct mode correctly above.
>
> for_each_set_bit(i, i_dev->active_scan_mask, i_dev->masklength) {
> const struct iio_chan_spec *chan = &i_dev->channels[i];
> @@ -271,7 +280,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
>
> iio_push_to_buffers_with_timestamp(i_dev, &data, iio_get_time_ns(i_dev));
> out:
> - mutex_unlock(&i_dev->mlock);
> + mutex_unlock(&info->lock);
>
> iio_trigger_notify_done(i_dev->trig);
>
> @@ -332,6 +341,8 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> info->reset = NULL;
> }
>
> + mutex_init(&info->lock);
> +
> init_completion(&info->completion);
>
> irq = platform_get_irq(pdev, 0);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock
2020-09-28 13:13 [PATCH 1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock Mircea Caprioru
` (3 preceding siblings ...)
2020-09-28 13:13 ` [PATCH 5/5] iio: adc: rockchip_saradc: " Mircea Caprioru
@ 2021-02-21 16:37 ` Jonathan Cameron
4 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2021-02-21 16:37 UTC (permalink / raw)
To: Mircea Caprioru
Cc: Michael.Hennerich, alexandru.ardelean, lars, gregkh,
linux-kernel, linux-iio, Sergiu Cuciurean
On Mon, 28 Sep 2020 16:13:29 +0300
Mircea Caprioru <mircea.caprioru@analog.com> wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
> This is part of a bigger cleanup.
> Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/
>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
I guess I was waiting for a v2 of the series. Seeing as it has been a while
and the first 3 patches are fine on their own, I'll pick them up now.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/iio/adc/spear_adc.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c
> index 1bc986a7009d..d93e580b3dc5 100644
> --- a/drivers/iio/adc/spear_adc.c
> +++ b/drivers/iio/adc/spear_adc.c
> @@ -75,6 +75,15 @@ struct spear_adc_state {
> struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
> struct clk *clk;
> struct completion completion;
> + /*
> + * Lock to protect the device state during a potential concurrent
> + * read access from userspace. Reading a raw value requires a sequence
> + * of register writes, then a wait for a completion callback,
> + * and finally a register read, during which userspace could issue
> + * another read request. This lock protects a read access from
> + * ocurring before another one has finished.
> + */
> + struct mutex lock;
> u32 current_clk;
> u32 sampling_freq;
> u32 avg_samples;
> @@ -146,7 +155,7 @@ static int spear_adc_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
> SPEAR_ADC_STATUS_AVG_SAMPLE(st->avg_samples) |
> @@ -159,7 +168,7 @@ static int spear_adc_read_raw(struct iio_dev *indio_dev,
> wait_for_completion(&st->completion); /* set by ISR */
> *val = st->value;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return IIO_VAL_INT;
>
> @@ -187,7 +196,7 @@ static int spear_adc_write_raw(struct iio_dev *indio_dev,
> if (mask != IIO_CHAN_INFO_SAMP_FREQ)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> if ((val < SPEAR_ADC_CLK_MIN) ||
> (val > SPEAR_ADC_CLK_MAX) ||
> @@ -199,7 +208,7 @@ static int spear_adc_write_raw(struct iio_dev *indio_dev,
> spear_adc_set_clk(st, val);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> @@ -271,6 +280,9 @@ static int spear_adc_probe(struct platform_device *pdev)
> }
>
> st = iio_priv(indio_dev);
> +
> + mutex_init(&st->lock);
> +
> st->np = np;
>
> /*
^ permalink raw reply [flat|nested] 12+ messages in thread