[PATCH v6 0/3] staging: iio: adc: ad7816: Fix SPI read race condition and DMA safety

[PATCH v6 0/3] staging: iio: adc: ad7816: Fix SPI read race condition and DMA safety

[Abdelnasser Hussein]

This series addresses two separate issues in the ad7816 driver:
1. A race condition during SPI reads where multiple threads could
   disrupt the GPIO state.
2. A DMA safety issue caused by using a stack-allocated buffer for
   SPI transfers.

Changes in v6:
- Separated the alphabetical sorting of include headers into a dedicated
  precursor patch (Patch 1).
- Kept the mutex addition strictly focused on serializing the read
  operations without modifying the sizeof() argument (Patch 2).
- Moved the sizeof() argument fix to the DMA-safe buffer patch where
  it logically belongs (Patch 3).

Changes in v5:
- Split the changes into separate patches for bisectability.
- Replaced stack buffer with a dedicated rx_buf aligned with
  IIO_DMA_MINALIGN.
Abdelnasser Hussein (3):
  staging: iio: adc: ad7816: Sort headers alphabetically
  staging: iio: adc: ad7816: Serialize SPI read operations
  staging: iio: adc: ad7816: Use DMA-safe buffer for SPI read

 drivers/staging/iio/adc/ad7816.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

-- 
2.54.0

[PATCH v6 1/3] staging: iio: adc: ad7816: Sort headers alphabetically

[Abdelnasser Hussein]

Sort the include directives alphabetically to improve readability and
maintainability. This is a precursor cleanup patch to prepare for the
addition of new headers in subsequent changes.

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 30644d2d7c54..9e43ce83e071 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -5,19 +5,19 @@
  * Copyright 2010 Analog Devices Inc.
  */
 
-#include <linux/interrupt.h>
-#include <linux/gpio/consumer.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
 #include <linux/list.h>
-#include <linux/spi/spi.h>
 #include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include <linux/iio/events.h>
 
 /*
  * AD7816 config masks
-- 
2.54.0

[PATCH v6 2/3] staging: iio: adc: ad7816: Serialize SPI read operations

[Abdelnasser Hussein]

The ad7816_spi_read() function performs a sequence of GPIO state
changes followed by an SPI transfer. If multiple read operations
occur simultaneously, the GPIO state could be changed by one thread
while another is in the middle of a read. This leads to a race
condition where the sensor state is disrupted.

Introduce a mutex to serialize the read sequence, ensuring that the
GPIO toggling and the SPI transfer are treated as a single atomic
operation.

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 9e43ce83e071..acf26c5a267a 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -5,12 +5,14 @@
  * Copyright 2010 Analog Devices Inc.
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
@@ -50,6 +52,7 @@ struct ad7816_chip_info {
 	u8  oti_data[AD7816_CS_MAX + 1];
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
+	struct mutex lock; /* protect device state during SPI transfers */
 };
 
 enum ad7816_type {
@@ -67,6 +70,8 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	int ret;
 	__be16 buf;
 
+	guard(mutex)(&chip->lock);
+
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
 	ret = spi_write(spi_dev, &chip->channel_id, sizeof(chip->channel_id));
@@ -360,6 +365,10 @@ static int ad7816_probe(struct spi_device *spi_dev)
 		return -ENOMEM;
 	chip = iio_priv(indio_dev);
 
+	ret = devm_mutex_init(&spi_dev->dev, &chip->lock);
+	if (ret)
+		return ret;
+
 	chip->spi_dev = spi_dev;
 	for (i = 0; i <= AD7816_CS_MAX; i++)
 		chip->oti_data[i] = 203;
-- 
2.54.0

Re: [PATCH v6 2/3] staging: iio: adc: ad7816: Serialize SPI read operations

[Andy Shevchenko]

On Sat, Sep 12, 2026 at 04:25:16PM +0300, Abdelnasser Hussein wrote:
> The ad7816_spi_read() function performs a sequence of GPIO state
> changes followed by an SPI transfer. If multiple read operations
> occur simultaneously, the GPIO state could be changed by one thread
> while another is in the middle of a read. This leads to a race
> condition where the sensor state is disrupted.
> 
> Introduce a mutex to serialize the read sequence, ensuring that the
> GPIO toggling and the SPI transfer are treated as a single atomic
> operation.

...

> struct ad7816_chip_info {

>  	u8  oti_data[AD7816_CS_MAX + 1];
>  	u8  channel_id;	/* 0 always be temperature */
>  	u8  mode;
> +	struct mutex lock; /* protect device state during SPI transfers */
>  };

Is `pahole` happy with the chosen layout?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/3] staging: iio: adc: ad7816: Serialize SPI read operations

[nasser]

Hi Andy,

Thanks for pointing this out

Placing the mutex after the `u8` variables introduces unnecessary
padding/holes. I will move it up in the struct (right after the
pointers) in v7 to ensure a proper memory layout without wasted space.

Thanks for the review

Best regards,
Abdelnasser

[PATCH v6 3/3] staging: iio: adc: ad7816: Use DMA-safe buffer for SPI read

[Abdelnasser Hussein]

Stack buffers are unsafe for DMA with VMAP_STACK enabled because
they are not physically contiguous and can share cache lines. This
causes data corruption.

Fix this by using a dedicated rx_buf aligned with IIO_DMA_MINALIGN
in the device state structure to ensure cache coherency. This also
corrects the size argument in spi_read() to match the new buffer.

Fixes: 7024425db64a ("staging: iio: adc: new driver for AD7816 devices")
Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index acf26c5a267a..2fd2d6796603 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -53,6 +53,7 @@ struct ad7816_chip_info {
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
 	struct mutex lock; /* protect device state during SPI transfers */
+	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
 };
 
 enum ad7816_type {
@@ -68,7 +69,6 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 {
 	struct spi_device *spi_dev = chip->spi_dev;
 	int ret;
-	__be16 buf;
 
 	guard(mutex)(&chip->lock);
 
@@ -96,13 +96,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 
 	gpiod_set_value(chip->rdwr_pin, 0);
 	gpiod_set_value(chip->rdwr_pin, 1);
-	ret = spi_read(spi_dev, &buf, sizeof(*data));
+	ret = spi_read(spi_dev, &chip->rx_buf, sizeof(chip->rx_buf));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
 		return ret;
 	}
 
-	*data = be16_to_cpu(buf);
+	*data = be16_to_cpu(chip->rx_buf);
 
 	return ret;
 }
-- 
2.54.0

Re: [PATCH v6 3/3] staging: iio: adc: ad7816: Use DMA-safe buffer for SPI read

[Jonathan Cameron]

On Sat, 12 Sep 2026 16:25:17 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> Stack buffers are unsafe for DMA with VMAP_STACK enabled because
> they are not physically contiguous and can share cache lines. This
> causes data corruption.

This one has come up a few times as a false or at least misleading
explanation.  It really doesn't have anything to do with VMAP_STACK
as that only a most deals with pages and cachelines which matter here
are much smaller.

Any stack variable is a problem simply because we don't control the
data layout and pretty much anything else can end up in the same
cache line. 

> 
> Fix this by using a dedicated rx_buf aligned with IIO_DMA_MINALIGN
> in the device state structure to ensure cache coherency. This also
> corrects the size argument in spi_read() to match the new buffer.

Make sure you understand why this alignment marking works to ensure
the buffer is in a cache line on it's own.

However, don't do this for a single use small buffer.
Use spi_write_then_read() (with 0 sized write) but also make
sure you understand why that works.

Jonathan


> 
> Fixes: 7024425db64a ("staging: iio: adc: new driver for AD7816 devices")
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index acf26c5a267a..2fd2d6796603 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -53,6 +53,7 @@ struct ad7816_chip_info {
>  	u8  channel_id;	/* 0 always be temperature */
>  	u8  mode;
>  	struct mutex lock; /* protect device state during SPI transfers */
> +	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  enum ad7816_type {
> @@ -68,7 +69,6 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  {
>  	struct spi_device *spi_dev = chip->spi_dev;
>  	int ret;
> -	__be16 buf;
>  
>  	guard(mutex)(&chip->lock);
>  
> @@ -96,13 +96,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  
>  	gpiod_set_value(chip->rdwr_pin, 0);
>  	gpiod_set_value(chip->rdwr_pin, 1);
> -	ret = spi_read(spi_dev, &buf, sizeof(*data));
> +	ret = spi_read(spi_dev, &chip->rx_buf, sizeof(chip->rx_buf));
>  	if (ret < 0) {
>  		dev_err(&spi_dev->dev, "SPI data read error\n");
>  		return ret;
>  	}
>  
> -	*data = be16_to_cpu(buf);
> +	*data = be16_to_cpu(chip->rx_buf);
>  
>  	return ret;
>  }

Re: [PATCH v6 3/3] staging: iio: adc: ad7816: Use DMA-safe buffer for SPI read

[nasser]

Hi Jonathan,

Thank you so much for taking the time to review my patch and for your
detailed explanation. I truly appreciate it.

Your explanation makes perfect sense. I now understand that the real
issue is cacheline sharing, and why adding a dedicated buffer to the
struct is an overkill for a simple 2-byte read. I also looked into
spi_write_then_read() and saw how it safely handles the transfer by
allocating a bounce buffer under the hood.

I will drop the custom buffer and use spi_write_then_read() as you
guided me in the upcoming v7.

Thank you again for your patience and invaluable mentorship. It really
means a lot to me.

Best regards,
Abdelnasser