Re: [PATCH v3 3/3] iio: adc: new driver to support Linear technology's ltc2496

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Sat, Nov 23, 2019 at 05:12:04PM +0000, Jonathan Cameron wrote:
> On Thu, 21 Nov 2019 22:00:07 +0100
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
> > and so has a slightly different protocol. Only the actual hardware
> > access is different. The spi protocol is different enough to not be able
> > to map the differences via a regmap.
> > 
> > Also generalize the entry in MAINTAINER to cover the newly introduced
> > file.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> looks good with the exception of the now overly protected DMA buffers.
> 
> See inline.  As that's all I'm seeing that needs fixing I'll just
> fix this up whilst applying.
> 
> I'd like the series to sit a little longer on the list though to give
> devicetree maintainers time to look at the bindings.

ok.

> > +struct ltc2496_driverdata {
> > +	/* this must be the first member */
> > +	struct ltc2497core_driverdata common_ddata;
> > +	struct spi_device *spi;
> > +
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the
> > +	 * transfer buffers to live in their own cache lines.
> > +	 */
> > +	unsigned char rxbuf[3] ____cacheline_aligned;
> > +	unsigned char txbuf[3] ____cacheline_aligned;
> Ah.  I've not explained this clearly enough.  Upshot is you only need
> to ensure that the buffers used for dma are not shared with any other
> usage.  the __cacheline_aligned marker pads the structure to ensure
> the element so marked is at the start of a new cacheline.  This means
> there is no sharing with non DMA related elements which may be accidentally
> reset when the DMA transfer ends.
> 
> Imagine we had:
> struct bob {
> 	int a; //used for all sorts of fun things not related to dma and not
>        	       //protected from happening concurrently with dma.
> 	unsigned char rx_buf[3];
> 	unsigned char tx_buf[3]
> };
> 
> The buffers are used for DMA.  The DMA engine takes a copy of the cacheline
> to start doing it's magic.
> 
> Along comes other activity and writes to 'a'.
> 
> DMA completes, then engine pushes the cacheline back to the memory including
> writing back what it had as a copy of a.  Thus the update to 'a' is lost.
> 
> Now the guarantee we make use of is that DMA engines are not allowed to
> copy cachelines that do not contain the buffers they are using (all sorts
> of things would break if they were).
> 
> However, there is no need to separate rx_buf and tx_buf as they are being
> used by the same DMA engine and nothing else is going to update them whilst
> they are in use.

Yeah, I thought about that when adding the second annotation but then
forgot to mention that in my cover letter.

So I assume you will just drop the 2nd ____cacheline_aligned? That's
fine for me; thanks.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |