[PATCH v4 0/2] staging: iio: adc: ad7816: Fix SPI read operations and VMAP_STACK issue

[PATCH v4 0/2] staging: iio: adc: ad7816: Fix SPI read operations and VMAP_STACK issue

[Abdelnasser Hussein]

Changes in v4:
- Separated changes into two logical patches.
- Patch 1: Used guard(mutex) and devm_mutex_init() instead of manual lock/unlock and mutex_init().
- Patch 2: Used __aligned(IIO_DMA_MINALIGN) instead of ____cacheline_aligned.
- Patch 2: Fixed the sizeof() argument in spi_read().

Abdelnasser Hussein (2):
  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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)


base-commit: 3d6d817622b0a9721e3cc404df3469171582be13
-- 
2.54.0

[PATCH v4 1/2] staging: iio: adc: ad7816: Serialize SPI read operations

[Abdelnasser Hussein]

Add a mutex to serialize SPI read operations and prevent data corruption. Used guard(mutex) and devm_mutex_init() for safer and automatic cleanup.
Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 0e32a2295990..ab80b3a889bb 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -14,6 +14,8 @@
 #include <linux/list.h>
 #include <linux/spi/spi.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/cleanup.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -65,7 +67,8 @@ 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);
 
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
@@ -360,6 +363,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

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

[Abdelnasser Hussein]

Stack buffers are not guaranteed to be cache-coherent and must not
be used with SPI reads, which can trigger issues with VMAP_STACK.
Fix this by using a dedicated DMA-safe rx_buf aligned with
IIO_DMA_MINALIGN. Also, correct the sizeof() argument from *data
to chip->rx_buf to match the buffer size.

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, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index ab80b3a889bb..e14ae805f076 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -52,6 +52,8 @@ 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 */
+	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
 };
 
 enum ad7816_type {
@@ -94,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 v4 1/2] staging: iio: adc: ad7816: Serialize SPI read operations

[Joshua Crofts]

On Sun,  6 Sep 2026 14:40:48 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> Add a mutex to serialize SPI read operations and prevent data corruption. Used guard(mutex) and devm_mutex_init() for safer and automatic cleanup.

Please wrap your commit message to ~75 characters per line.

> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..ab80b3a889bb 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -14,6 +14,8 @@
>  #include <linux/list.h>
>  #include <linux/spi/spi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/cleanup.h>

Even if the header list isn't sorted, try to add the headers
in the approximate location if the list were sorted. Better yet, 
you could send a patch that orders and cleans up the include list.

>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -65,7 +67,8 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  {
>  	struct spi_device *spi_dev = chip->spi_dev;
>  	int ret;
> -	__be16 buf;

Stray change? I believe this goes into patch 2.

> +
> +	guard(mutex)(&chip->lock);
>  	gpiod_set_value(chip->rdwr_pin, 1);
>  	gpiod_set_value(chip->rdwr_pin, 0);
> @@ -360,6 +363,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;



-- 
Kind regards,
Joshua Crofts

Re: [PATCH v4 1/2] staging: iio: adc: ad7816: Serialize SPI read operations

[Joshua Crofts]

On Sun,  6 Sep 2026 14:40:48 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> Add a mutex to serialize SPI read operations and prevent data corruption. Used guard(mutex) and devm_mutex_init() for safer and automatic cleanup.
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..ab80b3a889bb 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -14,6 +14,8 @@
>  #include <linux/list.h>
>  #include <linux/spi/spi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/cleanup.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -65,7 +67,8 @@ 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);

Actually, I thought I was going crazy when this compiled, only to realize
you added the mutex in patch 2. Compiling with only this patch applied yields
10+ errors. Each patch must be atomic and cannot break the build, even if the
entire applied series compiles correctly.

Please revisit this.

-- 
Kind regards,
Joshua Crofts

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

[Joshua Crofts]

On Sun,  6 Sep 2026 14:40:49 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index ab80b3a889bb..e14ae805f076 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -52,6 +52,8 @@ 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 */

See my 2 replies to the previous patch. This change shouldn't be here.

> +	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  enum ad7816_type {
> @@ -94,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));

As well as the fact you removed buf in the first patch, causing build
errors.

-- 
Kind regards,
Joshua Crofts

Re: [PATCH v4 0/2] staging: iio: adc: ad7816: Fix SPI read operations and VMAP_STACK issue

[Joshua Crofts]

On Sun,  6 Sep 2026 14:40:47 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

No overall description of what the patch series actually does?

> Changes in v4:
> - Separated changes into two logical patches.
> - Patch 1: Used guard(mutex) and devm_mutex_init() instead of manual lock/unlock and mutex_init().
> - Patch 2: Used __aligned(IIO_DMA_MINALIGN) instead of ____cacheline_aligned.
> - Patch 2: Fixed the sizeof() argument in spi_read().

I assume you forgot to send this patch? It's not listed below...

Either way, NACK to this series until all of the patches do exactly
what their commit messages say. Applying patch 1 causes compiler
errors. Ideally, you should compile each patch separately to prevent this.

> Abdelnasser Hussein (2):
>   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 | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> 
> base-commit: 3d6d817622b0a9721e3cc404df3469171582be13



-- 
Kind regards,
Joshua Crofts

Re: [PATCH v4 0/2] staging: iio: adc: ad7816: Fix SPI read operations and VMAP_STACK issue

[nasser]

hi Joshua,

Thanks a lot for the detailed review and for catching these issues.
i apologize for the bisectability breakage. I now realize that
removing buf and missing the lock declaration in patch 1 caused the
build errors.
I will fix the commit splitting, adjust the commit message wrapping,
sort the includes correctly, and add a proper cover letter
description.
I will test each commit separately and send v5 shortly.

Thanks,
Abdelnasser

Re: [PATCH v4 1/2] staging: iio: adc: ad7816: Serialize SPI read operations

[Jonathan Cameron]

On Sun,  6 Sep 2026 14:40:48 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> Add a mutex to serialize SPI read operations and prevent data corruption. Used guard(mutex) and devm_mutex_init() for safer and automatic cleanup.
Check submitting patches, this is not well formatted.

> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>

Alongside the comments Joshua had I'd expect to see a description of what
the race you are preventing is and if you are going to say things like
data corruption, an illustration of what gets corrupted and how.

Jonathan

> ---
>  drivers/staging/iio/adc/ad7816.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..ab80b3a889bb 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -14,6 +14,8 @@
>  #include <linux/list.h>
>  #include <linux/spi/spi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/cleanup.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -65,7 +67,8 @@ 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);
>  
>  	gpiod_set_value(chip->rdwr_pin, 1);
>  	gpiod_set_value(chip->rdwr_pin, 0);
> @@ -360,6 +363,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;

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

[Jonathan Cameron]

On Sun,  6 Sep 2026 14:40:49 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> Stack buffers are not guaranteed to be cache-coherent and must not
> be used with SPI reads, which can trigger issues with VMAP_STACK.

This needs a rewrite and to show more understanding of what is going
on.  What it says write now implies that some how buffers on the heap
are magically cache coherent and ones on the stack are not

I'd suggest watching the talk Wolfram Sang gave at ELCE a few years
back on this topic as a starting point.

> Fix this by using a dedicated DMA-safe rx_buf aligned with
> IIO_DMA_MINALIGN. Also, correct the sizeof() argument from *data
> to chip->rx_buf to match the buffer size.

They are the same size.  I'm not saying the fix is wrong but calling
it correct is misleading.

> 
> 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, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index ab80b3a889bb..e14ae805f076 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -52,6 +52,8 @@ 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 */
> +	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  enum ad7816_type {
> @@ -94,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 v4 2/2] staging: iio: adc: ad7816: Use DMA-safe buffer for SPI read

[nasser]

Hi Jonathan,

Thanks a lot for the detailed explanation and for taking the time to
explain the underlying architecture. I really appreciate the mentoring
and the pointer to Wolfram Sang's talk; I will definitely watch it.

I will rewrite the commit message to properly explain the cache line
sharing and VMAP_STACK issues, rather than implying stack memory is
magically not cache-coherent. I will also fix the misleading "correct
the sizeof" wording.
I am preparing v5 with these updates and will send it shortly.

Thanks,
Abdelnasser