diff mbox series

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

Message ID	20231121-dev-iio-backend-v1-6-6a3d542eba35@analog.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; From: Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> Date: Tue, 21 Nov 2023 11:20:19 +0100 Subject: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231121-dev-iio-backend-v1-6-6a3d542eba35@analog.com> References: <20231121-dev-iio-backend-v1-0-6a3d542eba35@analog.com> In-Reply-To: <20231121-dev-iio-backend-v1-0-6a3d542eba35@analog.com> To: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-iio@vger.kernel.org Cc: Olivier MOYSAN <olivier.moysan@foss.st.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich <Michael.Hennerich@analog.com>, Nuno Sa <nuno.sa@analog.com> Reply-To: <nuno.sa@analog.com> Precedence: bulk
Series	iio: add new backend framework \| [00/12] iio: add new backend framework [01/12] driver: core: allow modifying device_links flags [02/12] of: property: add device link support for io-backends [03/12] iio: add the IIO backend framework [04/12] iio: adc: ad9467: fix reset gpio handling [05/12] iio: adc: ad9467: don't ignore error codes [06/12] iio: adc: ad9467: add mutex to struct ad9467_state [07/12] iio: adc: ad9467: fix scale setting [08/12] iio: adc: ad9467: use spi_get_device_match_data() [09/12] iio: adc: ad9467: use chip_info variables instead of array [10/12] iio: adc: ad9467: convert to backend framework [11/12] iio: adc: adi-axi-adc: convert to regmap [12/12] iio: adc: adi-axi-adc: move to backend framework

Commit Message

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

  From: Nuno Sa <nuno.sa@analog.com>

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 <nuno.sa@analog.com>
---
 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
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> 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 <nuno.sa@analog.com>
> ---
>  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
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > 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 <nuno.sa@analog.com>
> > ---
> >  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á <noname.nuno@gmail.com> 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
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > 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 <nuno.sa@analog.com>
> > > ---
> > >  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 <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> 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 <nuno.sa@analog.com>
> ---
>  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 <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > 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 <nuno.sa@analog.com>
> > ---
> >  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á <noname.nuno@gmail.com> 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 <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > 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 <nuno.sa@analog.com>
> > > ---
> > >  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á
> >   
>

diff mbox series

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)