[PATCH] staging: iio: ad7816: avoid DMA from stack in spi_read

[PATCH] staging: iio: ad7816: avoid DMA from stack in spi_read

[Abdelnasser Hussein]

The SPI core may use DMA for transfers. Using a stack-allocated
buffer for DMA is unsafe and can trigger faults when VMAP_STACK is
enabled, since the stack is not guaranteed to be DMA-accessible.

Move the transfer buffer from the stack into the ad7816_chip_info
structure so it has a stable lifetime suitable for DMA transfers.
Mark the buffer with ____cacheline_aligned to ensure proper alignment
for DMA operations.

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

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 0e32a2295990..fcaadebff0f4 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -50,6 +50,7 @@ struct ad7816_chip_info {
 	u8  oti_data[AD7816_CS_MAX + 1];
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
+	__be16 rx_buf ____cacheline_aligned;
 };
 
 enum ad7816_type {
@@ -65,7 +66,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;
 
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
@@ -91,14 +91,12 @@ 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] staging: iio: ad7816: avoid DMA from stack in spi_read

[David Lechner]

On 8/2/26 6:38 AM, Abdelnasser Hussein wrote:
> The SPI core may use DMA for transfers. Using a stack-allocated
> buffer for DMA is unsafe and can trigger faults when VMAP_STACK is
> enabled, since the stack is not guaranteed to be DMA-accessible.
> 
> Move the transfer buffer from the stack into the ad7816_chip_info
> structure so it has a stable lifetime suitable for DMA transfers.
> Mark the buffer with ____cacheline_aligned to ensure proper alignment
> for DMA operations.

Should also mention fixing the "wrong" sizeof() use in the spi_read()
call. It was the correct size, but the wrong variable was referenced.

> 

Probably deserves a Fixes: tag.

> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..fcaadebff0f4 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -50,6 +50,7 @@ struct ad7816_chip_info {
>  	u8  oti_data[AD7816_CS_MAX + 1];
>  	u8  channel_id;	/* 0 always be temperature */
>  	u8  mode;
> +	__be16 rx_buf ____cacheline_aligned;

In IIO, we have a special macro for this instead of `____cacheline_aligned`. 

__aligned(IIO_DMA_MINALIGN);

>  };
>  
>  enum ad7816_type {
> @@ -65,7 +66,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;
>  
>  	gpiod_set_value(chip->rdwr_pin, 1);
>  	gpiod_set_value(chip->rdwr_pin, 0);
> @@ -91,14 +91,12 @@ 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] staging: iio: ad7816: avoid DMA from stack in spi_read

[Jonathan Cameron]

On Sun, 2 Aug 2026 10:31:29 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/2/26 6:38 AM, Abdelnasser Hussein wrote:
> > The SPI core may use DMA for transfers. Using a stack-allocated
> > buffer for DMA is unsafe and can trigger faults when VMAP_STACK is
> > enabled, since the stack is not guaranteed to be DMA-accessible.
> > 
> > Move the transfer buffer from the stack into the ad7816_chip_info
> > structure so it has a stable lifetime suitable for DMA transfers.
> > Mark the buffer with ____cacheline_aligned to ensure proper alignment
> > for DMA operations.  
> 
> Should also mention fixing the "wrong" sizeof() use in the spi_read()
> call. It was the correct size, but the wrong variable was referenced.
> 
> >   
> 
> Probably deserves a Fixes: tag.
> 
> > Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7816.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> > index 0e32a2295990..fcaadebff0f4 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -50,6 +50,7 @@ struct ad7816_chip_info {
> >  	u8  oti_data[AD7816_CS_MAX + 1];
> >  	u8  channel_id;	/* 0 always be temperature */
> >  	u8  mode;
> > +	__be16 rx_buf ____cacheline_aligned;  
> 
> In IIO, we have a special macro for this instead of `____cacheline_aligned`. 

We've had a couple of these recently.  ____cacheline_aligned is simply
wrong and I'm curious where that is coming from?  That's the performance
hint cache line size, typically that of l1 and l2. In some systems other
caches before the incoherent SPI controllers have larger cacheline sizes
and we have to align to those.

The correct option if not using the IIO one is __aligned(ARCH_DMA_MINALIGN)

For historical reasons IIO has it's own version of that which predates
all architectures providing ARCH_DMA_MINALIGN.

> 
> __aligned(IIO_DMA_MINALIGN);
> 
> >  };
> >  
> >  enum ad7816_type {
> > @@ -65,7 +66,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;
> >  
> >  	gpiod_set_value(chip->rdwr_pin, 1);
> >  	gpiod_set_value(chip->rdwr_pin, 0);
> > @@ -91,14 +91,12 @@ 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] staging: iio: ad7816: avoid DMA from stack in spi_read

[nasser]

Hi David,

Thanks for the review and suggestions.

> Should also mention fixing the "wrong" sizeof() use in the spi_read() call.
> Probably deserves a Fixes: tag.
> In IIO, we have a special macro for this instead of `____cacheline_aligned`.
> __aligned(IIO_DMA_MINALIGN);

I have applied all these changes. Following Jonathan's advice, I also
added a mutex to protect the SPI read sequence to prevent race
conditions.

I have split these changes into a two-patch v2 series and will send it
to the list shortly.

Best regards,
Abdelnasser

On Sun, Aug 2, 2026 at 9:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 2 Aug 2026 10:31:29 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On 8/2/26 6:38 AM, Abdelnasser Hussein wrote:
> > > The SPI core may use DMA for transfers. Using a stack-allocated
> > > buffer for DMA is unsafe and can trigger faults when VMAP_STACK is
> > > enabled, since the stack is not guaranteed to be DMA-accessible.
> > >
> > > Move the transfer buffer from the stack into the ad7816_chip_info
> > > structure so it has a stable lifetime suitable for DMA transfers.
> > > Mark the buffer with ____cacheline_aligned to ensure proper alignment
> > > for DMA operations.
> >
> > Should also mention fixing the "wrong" sizeof() use in the spi_read()
> > call. It was the correct size, but the wrong variable was referenced.
> >
> > >
> >
> > Probably deserves a Fixes: tag.
> >
> > > Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> > > ---
> > >  drivers/staging/iio/adc/ad7816.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> > > index 0e32a2295990..fcaadebff0f4 100644
> > > --- a/drivers/staging/iio/adc/ad7816.c
> > > +++ b/drivers/staging/iio/adc/ad7816.c
> > > @@ -50,6 +50,7 @@ struct ad7816_chip_info {
> > >     u8  oti_data[AD7816_CS_MAX + 1];
> > >     u8  channel_id; /* 0 always be temperature */
> > >     u8  mode;
> > > +   __be16 rx_buf ____cacheline_aligned;
> >
> > In IIO, we have a special macro for this instead of `____cacheline_aligned`.
>
> We've had a couple of these recently.  ____cacheline_aligned is simply
> wrong and I'm curious where that is coming from?  That's the performance
> hint cache line size, typically that of l1 and l2. In some systems other
> caches before the incoherent SPI controllers have larger cacheline sizes
> and we have to align to those.
>
> The correct option if not using the IIO one is __aligned(ARCH_DMA_MINALIGN)
>
> For historical reasons IIO has it's own version of that which predates
> all architectures providing ARCH_DMA_MINALIGN.
>
> >
> > __aligned(IIO_DMA_MINALIGN);
> >
> > >  };
> > >
> > >  enum ad7816_type {
> > > @@ -65,7 +66,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;
> > >
> > >     gpiod_set_value(chip->rdwr_pin, 1);
> > >     gpiod_set_value(chip->rdwr_pin, 0);
> > > @@ -91,14 +91,12 @@ 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;
> > >  }
> > >
> >
> >
>

[PATCH v2 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[Abdelnasser Hussein]

 drivers/staging/iio/adc/ad7816.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-- 
2.54.0

[PATCH v2 1/2] staging: iio: ad7816: serialize ad7816_spi_read() with a mutex

[Abdelnasser Hussein]

The ad7816_spi_read() path performs a sequence of SPI transfers and GPIO
state changes that must not be interleaved with another read operation.

Without serialization, concurrent callers can interfere with each other,
leading to inconsistent device state and incorrect data being returned.

Add a mutex to struct ad7816_chip_info and hold it across the entire read
sequence to ensure exclusive access to the device.

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

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 0e32a2295990..b5a0c2871e00 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -50,6 +50,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,11 +68,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	int ret;
 	__be16 buf;
 
+	mutex_lock(&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));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI channel setting error\n");
+		mutex_unlock(&chip->lock);
 		return ret;
 	}
 	gpiod_set_value(chip->rdwr_pin, 1);
@@ -94,11 +98,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	ret = spi_read(spi_dev, &buf, sizeof(*data));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
+		mutex_unlock(&chip->lock);
+
 		return ret;
 	}
 
 	*data = be16_to_cpu(buf);
-
+	mutex_unlock(&chip->lock);
 	return ret;
 }
 
@@ -359,7 +365,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
 	if (!indio_dev)
 		return -ENOMEM;
 	chip = iio_priv(indio_dev);
-
+	mutex_init(&chip->lock);
 	chip->spi_dev = spi_dev;
 	for (i = 0; i <= AD7816_CS_MAX; i++)
 		chip->oti_data[i] = 203;
-- 
2.54.0

[PATCH v2 2/2] staging: iio: ad7816: avoid DMA from stack in spi_read

[Abdelnasser Hussein]

The SPI transfer buffer is allocated on the stack, which is unsafe when
the SPI core performs DMA transfers. With VMAP_STACK enabled, this can
lead to DMA mapping failures because the stack is not guaranteed to be
DMA-accessible.

Move the buffer into struct ad7816_chip_info to provide storage with an
appropriate lifetime for DMA, align it with
__aligned(IIO_DMA_MINALIGN), and update the spi_read() sizeof() argument
to reference the relocated buffer.

Fixes: 7924425db04a ("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 b5a0c2871e00..c58a6bf77020 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -51,6 +51,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 {
@@ -66,7 +67,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;
 
 	mutex_lock(&chip->lock);
 
@@ -95,7 +95,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_read(spi_dev, &chip->rx_buf, sizeof(chip->rx_buf));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
 		mutex_unlock(&chip->lock);
@@ -103,7 +103,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 		return ret;
 	}
 
-	*data = be16_to_cpu(buf);
+	*data = be16_to_cpu(chip->rx_buf);
 	mutex_unlock(&chip->lock);
 	return ret;
 }
-- 
2.54.0

Re: [PATCH v2 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[Greg KH]

On Mon, Aug 03, 2026 at 03:15:18PM +0300, Abdelnasser Hussein wrote:
>  drivers/staging/iio/adc/ad7816.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> -- 
> 2.54.0
> 

Something went really wrong here :(

Re: [PATCH v2 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[Joshua Crofts]

On Mon,  3 Aug 2026 15:15:18 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

>  drivers/staging/iio/adc/ad7816.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Please don't send new versions as a reply to the previous version,
it breaks workflow tools like b4 and there's a higher chance it
won't get reviewed by others.

-- 
Kind regards,
Joshua Crofts

Re: [PATCH v2 1/2] staging: iio: ad7816: serialize ad7816_spi_read() with a mutex

[Joshua Crofts]

On Mon,  3 Aug 2026 15:15:19 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> The ad7816_spi_read() path performs a sequence of SPI transfers and GPIO
> state changes that must not be interleaved with another read operation.
> 
> Without serialization, concurrent callers can interfere with each other,
> leading to inconsistent device state and incorrect data being returned.
> 
> Add a mutex to struct ad7816_chip_info and hold it across the entire read
> sequence to ensure exclusive access to the device.
> 
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..b5a0c2871e00 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -50,6 +50,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,11 +68,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  	int ret;
>  	__be16 buf;
>  
> +	mutex_lock(&chip->lock);

This patch would benefit from using guard(mutex) from the <linux/cleanup.h>
header. 
> +
>  	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));
>  	if (ret < 0) {
>  		dev_err(&spi_dev->dev, "SPI channel setting error\n");
> +		mutex_unlock(&chip->lock);
>  		return ret;
>  	}
>  	gpiod_set_value(chip->rdwr_pin, 1);
> @@ -94,11 +98,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  	ret = spi_read(spi_dev, &buf, sizeof(*data));
>  	if (ret < 0) {
>  		dev_err(&spi_dev->dev, "SPI data read error\n");
> +		mutex_unlock(&chip->lock);
> +
>  		return ret;
>  	}
>  
>  	*data = be16_to_cpu(buf);
> -
> +	mutex_unlock(&chip->lock);
>  	return ret;
>  }
>  
> @@ -359,7 +365,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	chip = iio_priv(indio_dev);
> -
> +	mutex_init(&chip->lock);

devm_mutex_init() since the driver uses managed resources. Additionally,
please check the return value of the function and just return on failure.

>  	chip->spi_dev = spi_dev;
>  	for (i = 0; i <= AD7816_CS_MAX; i++)
>  		chip->oti_data[i] = 203;



-- 
Kind regards,
Joshua Crofts

Re: [PATCH v2 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[nasser]

Hi all,

Please ignore this v3 series.

I accidentally sent a few unrelated patches after forgetting to clean
my working directory before running git send-email.

Sorry for the confusion and the extra noise. I'll send a clean v4
with only the correct ad7816 patches.

Best regards,
Abdelnasser