[06/12] iio: adc: ad9467: add mutex to struct ad9467_state

Commit Message

Nuno Sa via B4 Relay Nov. 21, 2023, 10:20 a.m. UTC

From: Nuno Sa <[e-mail]>

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 <[e-mail]>
---
 drivers/iio/adc/ad9467.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

David Lechner Nov. 30, 2023, 9:50 p.m. UTC | #1

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[e-mail]> wrote:
>
> From: Nuno Sa <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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.

Nuno Sá Dec. 1, 2023, 8:49 a.m. UTC | #2

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
> <[e-mail]> wrote:
> > 
> > From: Nuno Sa <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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á

Jonathan Cameron Dec. 4, 2023, 3:21 p.m. UTC | #3

On Fri, 01 Dec 2023 09:49:38 +0100
Nuno Sá <[e-mail]> 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
> > <[e-mail]> wrote:  
> > > 
> > > From: Nuno Sa <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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á
>

Jonathan Cameron Dec. 4, 2023, 3:23 p.m. UTC | #4

On Tue, 21 Nov 2023 11:20:19 +0100
Nuno Sa via B4 Relay <[e-mail]> wrote:

> From: Nuno Sa <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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)
>

Nuno Sá Dec. 4, 2023, 4:10 p.m. UTC | #5

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 <[e-mail]> wrote:
> 
> > From: Nuno Sa <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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á
>

Jonathan Cameron Dec. 4, 2023, 4:51 p.m. UTC | #6

On Mon, 04 Dec 2023 17:10:01 +0100
Nuno Sá <[e-mail]> 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 <[e-mail]> wrote:
> >   
> > > From: Nuno Sa <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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á
> >   
>

Patch

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)