Re: [PATCH 3/7] spi: bcm2835: Fix race on DMA termination

[Lukas Wunner <lukas@xxxxxxxxx>]

On Fri, Nov 09, 2018 at 04:28:48PM +0100, Stefan Wahren wrote:
> Am 08.11.18 um 08:06 schrieb Lukas Wunner:
> > If a DMA transfer finishes orderly right when spi_transfer_one_message()
> > determines that it has timed out, the callbacks bcm2835_spi_dma_done()
> > and bcm2835_spi_handle_err() race to call dmaengine_terminate_all(),
> > potentially leading to double termination.
> >
> > Prevent by atomically changing the dma_pending flag before calling
> > dmaengine_terminate_all().
> >
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > Fixes: 3ecd37edaa2a ("spi: bcm2835: enable dma modes for transfers meeting certain conditions")
> > Cc: stable@xxxxxxxxxxxxxxx # v4.2+
> > ---
> >  drivers/spi/spi-bcm2835.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> > index 594e9712ecbc..774161bbcb2e 100644
> > --- a/drivers/spi/spi-bcm2835.c
> > +++ b/drivers/spi/spi-bcm2835.c
> > @@ -232,10 +232,9 @@ static void bcm2835_spi_dma_done(void *data)
> >  	 * is called the tx-dma must have finished - can't get to this
> >  	 * situation otherwise...
> >  	 */
> > -	dmaengine_terminate_all(master->dma_tx);
> > -
> > -	/* mark as no longer pending */
> > -	bs->dma_pending = 0;
> > +	if (cmpxchg(&bs->dma_pending, true, false)) {
> > +		dmaengine_terminate_all(master->dma_tx);
> > +	}
> >  
> >  	/* and mark as completed */;
> >  	complete(&master->xfer_completion);
> > @@ -617,10 +616,9 @@ static void bcm2835_spi_handle_err(struct spi_master *master,
> >  	struct bcm2835_spi *bs = spi_master_get_devdata(master);
> >  
> >  	/* if an error occurred and we have an active dma, then terminate */
> > -	if (bs->dma_pending) {
> > +	if (cmpxchg(&bs->dma_pending, true, false)) {
> >  		dmaengine_terminate_all(master->dma_tx);
> >  		dmaengine_terminate_all(master->dma_rx);
> > -		bs->dma_pending = 0;
> >  	}
> >  	/* and reset */
> >  	bcm2835_spi_reset_hw(master);
> 
> i'm not sure but this doesn't look like the right way (tm) to me.

Why?


> Btw according to dmaengine.h the usage of function
> dmaengine_terminate_all is deprecated. Would be nice to get this fixed, too.

Yes, migrating to dmaengine_terminate_sync() / _async() will prevent
the race addressed by the present patch in the DMA driver, relieving
DMA clients such as this SPI driver from that responsibility (see
commit message of b36f09c3c441).  It would obviate the need for the
dma_pending flag.

I agree that it is desirable to migrate to the new API but the point here
is to fix the existing code in patches small enough that they can be
backported to stable, not rework the driver to migrate to the new API.
(Which wouldn't be eligible for stable, and also, the new API was
introduced in v4.5 and the race in this SPI driver has existed since
v4.2, i.e. earlier.)

However I will have to respin this patch and change the type of the
dma_pending flag to unsigned int because cmpxchg() requires a 32-bit
argument on arches such as extensa and this patch breaks the build
on them for COMPILE_TEST=y.  This was reported by 0-day.

Thanks,

Lukas