From: Nuno Sa <[email protected]>
When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
are done which means we need to properly protect the whole operation so
we are sure we will be in a sane state if two concurrent calls occur.
Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <[email protected]>
---
drivers/iio/adc/ad9467.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 04474dbfa631..91821dee03b7 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -4,7 +4,7 @@
*
* Copyright 2012-2020 Analog Devices Inc.
*/
-
+#include <linux/cleanup.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/device.h>
@@ -122,6 +122,8 @@ struct ad9467_state {
unsigned int output_mode;
struct gpio_desc *pwrdown_gpio;
+ /* protect against concurrent accesses to the device */
+ struct mutex lock;
};
static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
int ret;
if (!readval) {
+ guard(mutex)(&st->lock);
ret = ad9467_spi_write(spi, reg, writeval);
if (ret)
return ret;
@@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
if (scale_val[0] != val || scale_val[1] != val2)
continue;
+ guard(mutex)(&st->lock);
ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
info->scale_table[i][1]);
if (ret < 0)
--
2.42.1
On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[email protected]> wrote:
>
> From: Nuno Sa <[email protected]>
>
> When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> are done which means we need to properly protect the whole operation so
> we are sure we will be in a sane state if two concurrent calls occur.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> drivers/iio/adc/ad9467.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 04474dbfa631..91821dee03b7 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -4,7 +4,7 @@
> *
> * Copyright 2012-2020 Analog Devices Inc.
> */
> -
> +#include <linux/cleanup.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
Ah, the case of the misplaced header from the previous patch is solved. :-)
> #include <linux/device.h>
> @@ -122,6 +122,8 @@ struct ad9467_state {
> unsigned int output_mode;
>
> struct gpio_desc *pwrdown_gpio;
> + /* protect against concurrent accesses to the device */
> + struct mutex lock;
> };
>
> static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
> int ret;
>
> if (!readval) {
> + guard(mutex)(&st->lock);
> ret = ad9467_spi_write(spi, reg, writeval);
> if (ret)
> return ret;
> @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> if (scale_val[0] != val || scale_val[1] != val2)
> continue;
>
> + guard(mutex)(&st->lock);
> ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> info->scale_table[i][1]);
> if (ret < 0)
>
> --
> 2.42.1
>
>
Alternately, this could probably be solved with spi_bus_lock/unlock
and spi_sync_locked rather than introducing a new mutex.
On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <[email protected]> wrote:
> >
> > From: Nuno Sa <[email protected]>
> >
> > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > are done which means we need to properly protect the whole operation so
> > we are sure we will be in a sane state if two concurrent calls occur.
> >
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <[email protected]>
> > ---
> > drivers/iio/adc/ad9467.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 04474dbfa631..91821dee03b7 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,7 +4,7 @@
> > *
> > * Copyright 2012-2020 Analog Devices Inc.
> > */
> > -
> > +#include <linux/cleanup.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
>
> Ah, the case of the misplaced header from the previous patch is solved. :-)
>
Yeps, it needs to be in this patch :)
> > #include <linux/device.h>
> > @@ -122,6 +122,8 @@ struct ad9467_state {
> > unsigned int output_mode;
> >
> > struct gpio_desc *pwrdown_gpio;
> > + /* protect against concurrent accesses to the device */
> > + struct mutex lock;
> > };
> >
> > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > unsigned int reg,
> > int ret;
> >
> > if (!readval) {
> > + guard(mutex)(&st->lock);
> > ret = ad9467_spi_write(spi, reg, writeval);
> > if (ret)
> > return ret;
> > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > int val, int val2)
> > if (scale_val[0] != val || scale_val[1] != val2)
> > continue;
> >
> > + guard(mutex)(&st->lock);
> > ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> > info->scale_table[i][1]);
> > if (ret < 0)
> >
> > --
> > 2.42.1
> >
> >
>
> Alternately, this could probably be solved with spi_bus_lock/unlock
> and spi_sync_locked rather than introducing a new mutex.
Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
have some plans to eventually change this to regmap :)
- Nuno Sá
On Fri, 01 Dec 2023 09:49:38 +0100
Nuno Sá <[email protected]> wrote:
> On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <[email protected]> wrote:
> > >
> > > From: Nuno Sa <[email protected]>
> > >
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > >
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <[email protected]>
> > > ---
> > > drivers/iio/adc/ad9467.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > > *
> > > * Copyright 2012-2020 Analog Devices Inc.
> > > */
> > > -
> > > +#include <linux/cleanup.h>
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> >
> > Ah, the case of the misplaced header from the previous patch is solved. :-)
> >
>
> Yeps, it needs to be in this patch :)
>
> > > #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > > unsigned int output_mode;
> > >
> > > struct gpio_desc *pwrdown_gpio;
> > > + /* protect against concurrent accesses to the device */
> > > + struct mutex lock;
> > > };
> > >
> > > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > > unsigned int reg,
> > > int ret;
> > >
> > > if (!readval) {
> > > + guard(mutex)(&st->lock);
> > > ret = ad9467_spi_write(spi, reg, writeval);
> > > if (ret)
> > > return ret;
> > > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > > int val, int val2)
> > > if (scale_val[0] != val || scale_val[1] != val2)
> > > continue;
> > >
> > > + guard(mutex)(&st->lock);
> > > ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> > > info->scale_table[i][1]);
> > > if (ret < 0)
> > >
> > > --
> > > 2.42.1
> > >
> > >
> >
> > Alternately, this could probably be solved with spi_bus_lock/unlock
> > and spi_sync_locked rather than introducing a new mutex.
>
> Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
> have some plans to eventually change this to regmap :)
Bus lock typically implies that we can't let other users grab the bus in between
for reasons like the chip select needing to be held. I'm not keen on it being
used if the locking is just needed for a specific driver to deal with its
associated device and driver state.
Jonathan
>
> - Nuno Sá
>
On Tue, 21 Nov 2023 11:20:19 +0100
Nuno Sa via B4 Relay <[email protected]> wrote:
> From: Nuno Sa <[email protected]>
>
> When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> are done which means we need to properly protect the whole operation so
> we are sure we will be in a sane state if two concurrent calls occur.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> drivers/iio/adc/ad9467.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 04474dbfa631..91821dee03b7 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -4,7 +4,7 @@
> *
> * Copyright 2012-2020 Analog Devices Inc.
> */
> -
> +#include <linux/cleanup.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/device.h>
> @@ -122,6 +122,8 @@ struct ad9467_state {
> unsigned int output_mode;
>
> struct gpio_desc *pwrdown_gpio;
> + /* protect against concurrent accesses to the device */
Not very specific. Concurrent access usually fine at granularity of
individual read/write as the bus locks protect it. What state
is actually being protected? A shared buffer or some state that we
need to ensure remains consistent between driver and device?
> + struct mutex lock;
> };
>
> static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
> int ret;
>
> if (!readval) {
> + guard(mutex)(&st->lock);
> ret = ad9467_spi_write(spi, reg, writeval);
> if (ret)
> return ret;
> @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> if (scale_val[0] != val || scale_val[1] != val2)
> continue;
>
> + guard(mutex)(&st->lock);
> ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> info->scale_table[i][1]);
> if (ret < 0)
>
On Mon, 2023-12-04 at 15:23 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2023 11:20:19 +0100
> Nuno Sa via B4 Relay <[email protected]> wrote:
>
> > From: Nuno Sa <[email protected]>
> >
> > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > are done which means we need to properly protect the whole operation so
> > we are sure we will be in a sane state if two concurrent calls occur.
> >
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <[email protected]>
> > ---
> > drivers/iio/adc/ad9467.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 04474dbfa631..91821dee03b7 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,7 +4,7 @@
> > *
> > * Copyright 2012-2020 Analog Devices Inc.
> > */
> > -
> > +#include <linux/cleanup.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/device.h>
> > @@ -122,6 +122,8 @@ struct ad9467_state {
> > unsigned int output_mode;
> >
> > struct gpio_desc *pwrdown_gpio;
> > + /* protect against concurrent accesses to the device */
> Not very specific. Concurrent access usually fine at granularity of
> individual read/write as the bus locks protect it. What state
> is actually being protected? A shared buffer or some state that we
> need to ensure remains consistent between driver and device?
At this point not any buffer/data... Just making sure things remain consistent
(typical case when you have multiple reads/writes to the device). That's why a tried
to emphasize "accesses to the device". Maybe I should make it explicit I'm speaking
about multiple reads/writes.
- Nuno Sá
>
On Mon, 04 Dec 2023 17:10:01 +0100
Nuno Sá <[email protected]> wrote:
> On Mon, 2023-12-04 at 15:23 +0000, Jonathan Cameron wrote:
> > On Tue, 21 Nov 2023 11:20:19 +0100
> > Nuno Sa via B4 Relay <[email protected]> wrote:
> >
> > > From: Nuno Sa <[email protected]>
> > >
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > >
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <[email protected]>
> > > ---
> > > drivers/iio/adc/ad9467.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > > *
> > > * Copyright 2012-2020 Analog Devices Inc.
> > > */
> > > -
> > > +#include <linux/cleanup.h>
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> > > #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > > unsigned int output_mode;
> > >
> > > struct gpio_desc *pwrdown_gpio;
> > > + /* protect against concurrent accesses to the device */
> > Not very specific. Concurrent access usually fine at granularity of
> > individual read/write as the bus locks protect it. What state
> > is actually being protected? A shared buffer or some state that we
> > need to ensure remains consistent between driver and device?
>
> At this point not any buffer/data... Just making sure things remain consistent
> (typical case when you have multiple reads/writes to the device). That's why a tried
> to emphasize "accesses to the device". Maybe I should make it explicit I'm speaking
> about multiple reads/writes.
Talk about the data or state rather than the access to it.
Something like
'ensure consistent state obtained on multiple related accesses.'
Or if it's RMW then say that.
>
> - Nuno Sá
> >
>