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

From: Alexandru Ardelean <ardeleanalex@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	alexandru.tachici@analog.com,
	Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v2] iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack
Date: Thu, 19 Nov 2020 10:30:02 +0200	[thread overview]
Message-ID: <CA+U=DsqYqBGf3VsUqxxuc3M+=DZgU-tt0vhGqXR_AUS5TmJ3mg@mail.gmail.com> (raw)
In-Reply-To: <20201114162021.3caebdab@archlinux>

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.

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