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

[Stefan Wahren <stefan.wahren@xxxxxxxx>]

Hi Lukas,

[add Vinod]

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+
> Cc: Mathias Duckeck <m.duckeck@xxxxxxxxx>
> Cc: Frank Pavlic <f.pavlic@xxxxxxxxx>
> Cc: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
>  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.

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

Sorry, for not providing a more helpful advice.

Regards
Stefan