[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;
> > >  }
> > >
> >
> >
>

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