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@xxxxxxxxxx> wrote:
>
> On Sun, 2 Aug 2026 10:31:29 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >  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;
> > >  }
> > >
> >
> >
>