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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 24 Nov 2019 20:11:01 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> 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.
Yup.

Jonathan

> 
> Best regards
> Uwe
>