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

[Vinod Koul <vkoul@xxxxxxxxxx>]

On 10-11-18, 15:29, Lukas Wunner wrote:
> 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.)

I dont think porting the API introduction to stable would be an issue as
we fix things with those APIs

If you dont want that approach then why not submit this as fix to stable
pre 4.5 and for rest use the new APIs.

> 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

-- 
~Vinod