Re: [PATCH v2] iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Nov 2020 10:30:02 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, Nov 14, 2020 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 13 Nov 2020 11:40:59 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >  
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > >
> > > Use a heap allocated memory for the SPI transfer buffer. Using stack memory
> > > can corrupt stack memory when using DMA on some systems.
> > >
> > > This change moves the buffer from the stack of the trigger handler call to
> > > the heap of the buffer of the state struct. The size increases takes into
> > > account the alignment for the timestamp, which is 8 bytes.
> > > So the buffer is put at an offset of 8 bytes.
> > >
> > > Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> >
> > There are neater options for solving this problem - see inline.
> >
> > In particular I don't think you have a problem with setting the
> > rx and tx buffers to use the same memory.
> >  
> > > ---
> > >
> > > Changelog v1 -> v2:
> > > * bumped the buffer on state struct to 24 bytes
> > > * increased the offset to 8 bytes to account for the timestamp alignment
> > >
> > >  drivers/iio/adc/ad_sigma_delta.c       | 2 +-
> > >  include/linux/iio/adc/ad_sigma_delta.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > > index 86039e9ecaca..9f730c9d6aaa 100644
> > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > @@ -395,9 +395,9 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> > >       struct iio_poll_func *pf = p;
> > >       struct iio_dev *indio_dev = pf->indio_dev;
> > >       struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> > > +     uint8_t *data = &sigma_delta->data[8];
> > >       unsigned int reg_size;
> > >       unsigned int data_reg;
> > > -     uint8_t data[16];
> > >
> > >       memset(data, 0x00, 16);
> > >
> > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> > > index a3a838dcf8e4..8fb74755f873 100644
> > > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > > @@ -80,7 +80,7 @@ struct ad_sigma_delta {
> > >        * DMA (thus cache coherency maintenance) requires the
> > >        * transfer buffers to live in their own cache lines.
> > >        */  
> >
> > If you do end up with something like this, it needs a clear explanation of 'why'
> > the size is 24 bytes.  No good just having it in the patch description.
> >  
> > > -     uint8_t                         data[4] ____cacheline_aligned;
> > > +     uint8_t                         data[24] ____cacheline_aligned;  
> >
> > This is downright confusing.  I'd just split the buffer into tx and rx
> > parts.   The first (doesn't matter which) needs to be marked __cacheline_aligned.
> > If the rx is second mark it __aligned(8) to force that to be appropriate for
> > the timestamp.
> >
> > Or... (I haven't checked thoroughly for this from point of view of how it is used
> > in the drivers) use the same buffer for tx and rx.  That is supposed to be safe for
> > SPI drivers though wonderfully there is a ? after the statement of that in
> > include/linux/spi.h.  I think that is just pointing out that microwire doesn't
> > support duplex rather than saying it's invalid in general...  
> 
> I'm a bit paranoid to use the same buffer for RX & TX [in general].
> It sounds like this could hide some bugs in some weird DMA implementations.
> The DMA implementations could be fine on their own, but they wouldn't
> expect that TX & RX buffers point to the same place.

Whilst in theory it should either be fine, or the hardware should use
a bounce buffer if it's not, I can understand your paranoia so
I'm fine with separate buffers.

Jonathan

> 
> >
> >
> >  
> > >  };
> > >
> > >  static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,  
> >