Re: [PATCH 7/8] dmaengine: switchtec-dma: unmap descriptor before completing cookie

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: switchtec-dma: recursive deadlock when invoking callback with complete_lock held
--

commit ffea26942d7e62541800c95b1e3d4d09f66f0c18
Author: Wolfram Sang <wsa+renesas@sang-engineering.com>

dmaengine: switchtec-dma: unmap descriptor before completing cookie

This commit ensures that the descriptor gets unmapped before completing the
cookie to ensure that buffers are flushed.

Link: https://lore.kernel.org/r/20261009093745.539924-1-vkoul@kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index a386c3a284e6e..84a0a2e5e8722 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -486,8 +486,8 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
>  		}
>  
>  		do {
> -			dma_cookie_complete(&desc->txd);
>  			dma_descriptor_unmap(&desc->txd);
> +			dma_cookie_complete(&desc->txd);
>  			dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
Can this lead to a recursive deadlock if the client callback queries the DMA
status or terminates the transfer?

In switchtec_dma_cleanup_completed(), complete_lock is acquired earlier in the
function. If the DMA client's completion callback invokes dmaengine_tx_status()
or dmaengine_terminate_all(), those APIs will call into the driver's
switchtec_dma_tx_status() or switchtec_dma_terminate_all() functions.

switchtec_dma_tx_status() calls switchtec_dma_cleanup_completed(), which
attempts to re-acquire complete_lock.

switchtec_dma_terminate_all() also attempts to acquire complete_lock directly:

switchtec_dma_terminate_all()
    spin_lock_bh(&swdma_chan->complete_lock);

Since spin_lock_bh is not recursive, would this result in a deadlock?

>  			desc->txd.callback = NULL;
>  			desc->txd.callback_result = NULL;
[ ... ]
> @@ -529,8 +529,8 @@ switchtec_dma_abort_desc(struct switchtec_dma_chan *swdma_chan, int force)
>  		res.residue = desc->orig_size;
>  		res.result = DMA_TRANS_ABORTED;
>  
> -		dma_cookie_complete(&desc->txd);
>  		dma_descriptor_unmap(&desc->txd);
> +		dma_cookie_complete(&desc->txd);
>  		if (!force)
>  			dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
Does this code have the same deadlock regression as switchtec_dma_cleanup_completed()
since complete_lock is also held when switchtec_dma_abort_desc() invokes the
callback?

>  		desc->txd.callback = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010124348.19055-1-wsa+renesas@sang-engineering.com?part=7