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

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

[Abdelnasser Hussein]

This series addresses separate issues in the ad7816 driver regarding
race conditions and DMA safety across all SPI operations.

Changes in v7:

    Patch 2: Moved the mutex to the top of the ad7816_chip_info struct
    (right after the pointers) to prevent struct padding/holes, as
    pointed out by Andy Shevchenko. Also expanded the mutex to serialize
    ad7816_spi_write() to prevent race conditions.

    Patch 3: Dropped the dedicated rx_buf. Switched all spi_write() and
    spi_read() calls to use spi_write_then_read(..., NULL, 0) to safely
    handle DMA using an internal bounce buffer, avoiding cacheline sharing,
    as guided by Jonathan Cameron.

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 operations
  staging: iio: adc: ad7816: Fix DMA safety issues in SPI transfers

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

-- 
2.54.0

[PATCH v7 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 v7 2/3] staging: iio: adc: ad7816: Serialize SPI operations

[Abdelnasser Hussein]

The ad7816_spi_read() and ad7816_spi_write() functions perform a
sequence of GPIO state changes followed by an SPI transfer. If multiple
operations occur simultaneously, the GPIO state could be changed by one
thread while another is in the middle of a transfer, leading to a race
condition.

Introduce a mutex to serialize the operations, ensuring that the GPIO
toggling and the SPI transfer are treated as a single atomic operation.
The mutex is placed right after the pointers in the device state
structure to avoid padding holes.

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

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 9e43ce83e071..c18093ca8a82 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>
@@ -47,6 +49,7 @@ struct ad7816_chip_info {
 	struct gpio_desc *rdwr_pin;
 	struct gpio_desc *convert_pin;
 	struct gpio_desc *busy_pin;
+	struct mutex lock; /* protect device state during SPI transfers */
 	u8  oti_data[AD7816_CS_MAX + 1];
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
@@ -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));
@@ -107,6 +112,8 @@ static int ad7816_spi_write(struct ad7816_chip_info *chip, u8 data)
 	struct spi_device *spi_dev = chip->spi_dev;
 	int ret;
 
+	guard(mutex)(&chip->lock);
+
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
 	ret = spi_write(spi_dev, &data, sizeof(data));
@@ -360,6 +367,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 v7 2/3] staging: iio: adc: ad7816: Serialize SPI operations

[Maxwell Doose]

On Tue Sep 15, 2026 at 2:59 AM CDT
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> The ad7816_spi_read() and ad7816_spi_write() functions perform a
> sequence of GPIO state changes followed by an SPI transfer. If multiple
> operations occur simultaneously, the GPIO state could be changed by one
> thread while another is in the middle of a transfer, leading to a race
> condition.
>
> Introduce a mutex to serialize the operations, ensuring that the GPIO
> toggling and the SPI transfer are treated as a single atomic operation.
> The mutex is placed right after the pointers in the device state
> structure to avoid padding holes.
>
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
...
> @@ -47,6 +49,7 @@ struct ad7816_chip_info {
>  	struct gpio_desc *rdwr_pin;
>  	struct gpio_desc *convert_pin;
>  	struct gpio_desc *busy_pin;
> +	struct mutex lock; /* protect device state during SPI transfers */

I'm a little on the fence about the comment but if it satisfies
checkpatch then thats one less patch later.

>  	u8  oti_data[AD7816_CS_MAX + 1];
>  	u8  channel_id;	/* 0 always be temperature */
>  	u8  mode;

Good find!

Reviewed-by: Maxwell Doose <maxwell@maxwelld.cc>

Thanks,
Max

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

[Joshua Crofts]

On Tue, 15 Sep 2026 06:39:34 -0500
"Maxwell Doose" <maxwell@maxwelld.cc> wrote:

> On Tue Sep 15, 2026 at 2:59 AM CDT
> Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:
> 
> > The ad7816_spi_read() and ad7816_spi_write() functions perform a
> > sequence of GPIO state changes followed by an SPI transfer. If multiple
> > operations occur simultaneously, the GPIO state could be changed by one
> > thread while another is in the middle of a transfer, leading to a race
> > condition.
> >
> > Introduce a mutex to serialize the operations, ensuring that the GPIO
> > toggling and the SPI transfer are treated as a single atomic operation.
> > The mutex is placed right after the pointers in the device state
> > structure to avoid padding holes.
> >
> > Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7816.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)  
> ...
> > @@ -47,6 +49,7 @@ struct ad7816_chip_info {
> >  	struct gpio_desc *rdwr_pin;
> >  	struct gpio_desc *convert_pin;
> >  	struct gpio_desc *busy_pin;
> > +	struct mutex lock; /* protect device state during SPI transfers */  
> 
> I'm a little on the fence about the comment but if it satisfies
> checkpatch then thats one less patch later.

It's perfectly fine to comment like this, especially since this seems to
be the style the driver uses.

-- 
Kind regards,
Joshua Crofts

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

[Joshua Crofts]

On Tue, 15 Sep 2026 10:59:38 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

...

> +	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;

The patch in itself is fine, but Sashiko points out that the mutex
could be added in the ad7816_store_mode/channel() functions. Nevertheless,
this patch only focuses on SPI transfers so you could add guards to the
GPIO functions in another patch (I don't think you need to send a v8 though,
just another patch after this series gets merged).

Does this newly added lock also need to be acquired in the sysfs store
functions?
If a userspace process concurrently writes to the mode or channel sysfs
attributes, the GPIO pin and device state can be modified without acquiring
chip->lock:
drivers/staging/iio/adc/ad7816.c:ad7816_store_mode() {
    ...
	if (strcmp(buf, "full") == 0) {
		gpiod_set_value(chip->rdwr_pin, 1);
		chip->mode = AD7816_FULL;
	} else {
    ...
}
drivers/staging/iio/adc/ad7816.c:ad7816_store_channel() {
    ...
	chip->channel_id = data;
    ...
}
Could this concurrent access corrupt the rdwr_pin state, chip->mode, or
chip->channel_id variables during an ongoing SPI transfer, leading to
malformed transactions or corrupted ADC readings?

-- 
Kind regards,
Joshua Crofts

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

[nasser]

hi Joshua,

Thanks for the review and the great catch!
I completely agree that securing the sysfs store functions against
concurrent access is a logical next step.
 I will prepare a separate follow-up patch

Best regards,
Abdelnasser

[PATCH v7 3/3] staging: iio: adc: ad7816: Fix DMA safety issues in SPI transfers

[Abdelnasser Hussein]

The SPI operations in this driver are not DMA safe:
1. spi_read() uses a stack-allocated buffer.
2. spi_write() in ad7816_spi_read() uses a struct member that shares a
   cacheline with other variables.
3. spi_write() in ad7816_spi_write() passes a stack parameter by
   reference.

Fix these violations by replacing all spi_read() and spi_write() calls
with spi_write_then_read(). This safely handles DMA by internally
allocating a bounce buffer for the transfers, avoiding cacheline
sharing issues without needing dedicated aligned buffers.
This implicitly corrects the size argument in read to sizeof(buf).

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 c18093ca8a82..f76f0215119a 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -74,7 +74,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 
 	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));
+	ret = spi_write_then_read(spi_dev, &chip->channel_id, sizeof(chip->channel_id), NULL, 0);
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI channel setting error\n");
 		return ret;
@@ -96,7 +96,7 @@ 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_write_then_read(spi_dev, NULL, 0, &buf, sizeof(buf));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
 		return ret;
@@ -116,7 +116,7 @@ static int ad7816_spi_write(struct ad7816_chip_info *chip, u8 data)
 
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
-	ret = spi_write(spi_dev, &data, sizeof(data));
+	ret = spi_write_then_read(spi_dev, &data, sizeof(data), NULL, 0);
 	if (ret < 0)
 		dev_err(&spi_dev->dev, "SPI oti data write error\n");
 
-- 
2.54.0

Re: [PATCH v7 0/3] staging: iio: adc: ad7816: Fix SPI operations race condition and DMA safety

[Joshua Crofts]

On Tue, 15 Sep 2026 10:59:36 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> This series addresses separate issues in the ad7816 driver regarding
> race conditions and DMA safety across all SPI operations.
> 
> Changes in v7:
> 
>     Patch 2: Moved the mutex to the top of the ad7816_chip_info struct
>     (right after the pointers) to prevent struct padding/holes, as
>     pointed out by Andy Shevchenko. Also expanded the mutex to serialize
>     ad7816_spi_write() to prevent race conditions.
> 
>     Patch 3: Dropped the dedicated rx_buf. Switched all spi_write() and
>     spi_read() calls to use spi_write_then_read(..., NULL, 0) to safely
>     handle DMA using an internal bounce buffer, avoiding cacheline sharing,
>     as guided by Jonathan Cameron.
> 
> 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 operations
>   staging: iio: adc: ad7816: Fix DMA safety issues in SPI transfers
> 
>  drivers/staging/iio/adc/ad7816.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 

For the whole series:

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards,
Joshua Crofts