Re: [PATCH v6 8/9] PCI: epf-mhi: Use dmaengine_prep_config_single() to simplify code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The DMA workqueue is destroyed before pending DMA transfers are explicitly terminated, which can lead to a Use-After-Free if a DMA callback executes late.
--

commit 01fd36b3138418658a89651874d85c30619199e8
Author: Frank Li <Frank.Li@nxp.com>

PCI: epf-mhi: Use dmaengine_prep_config_single() to simplify code

This commit replaces dmaengine_slave_config() and dmaengine_prep_slave_single()
with a single call to dmaengine_prep_config_single() in the EDMA read and write
paths. The change aims to simplify the code without introducing any functional
differences.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index 7f5326925ed54..c3e3b58fb86cd 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
[ ... ]
> @@ -506,12 +496,6 @@ static int pci_epf_mhi_edma_read_async(struct mhi_ep_cntrl *mhi_cntrl,
>  	config.direction = DMA_DEV_TO_MEM;
>  	config.src_addr = buf_info->host_addr;
>  
> -	ret = dmaengine_slave_config(chan, &config);
> -	if (ret) {
> -		dev_err(dev, "Failed to configure DMA channel\n");
> -		goto err_unlock;
> -	}
> -
>  	dst_addr = dma_map_single(dma_dev, buf_info->dev_addr, buf_info->size,
>  				  DMA_FROM_DEVICE);
>  	ret = dma_mapping_error(dma_dev, dst_addr);
> @@ -520,9 +504,10 @@ static int pci_epf_mhi_edma_read_async(struct mhi_ep_cntrl *mhi_cntrl,
>  		goto err_unlock;
>  	}
>  
> -	desc = dmaengine_prep_slave_single(chan, dst_addr, buf_info->size,
> -					   DMA_DEV_TO_MEM,
> -					   DMA_CTRL_ACK | DMA_PREP_INTERRUPT);
> +	desc = dmaengine_prep_config_single(chan, dst_addr, buf_info->size,
> +					    DMA_DEV_TO_MEM,
> +					    DMA_CTRL_ACK | DMA_PREP_INTERRUPT,
> +					    &config);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with how these
asynchronous DMA transfers are cleaned up.

In pci_epf_mhi_dma_deinit(), the DMA workqueue is destroyed before pending DMA
transfers are explicitly terminated:

static void pci_epf_mhi_dma_deinit(struct pci_epf_mhi *epf_mhi)
{
	destroy_workqueue(epf_mhi->dma_wq);
	dma_release_channel(epf_mhi->dma_chan_tx);
	dma_release_channel(epf_mhi->dma_chan_rx);
	...
}

Does this sequence lead to a use-after-free if a DMA callback executes late?

If a transfer is physically still running in the DMA engine hardware when the
deinitialization begins, destroy_workqueue() will flush existing queued work
but cannot stop the hardware itself.

If the hardware completes the transfer a moment later, the DMA engine
interrupt will fire and invoke pci_epf_mhi_dma_async_callback(), which
blindly calls queue_work(epf_mhi->dma_wq, ...).

Should dmaengine_terminate_sync() be called to abort pending asynchronous
DMA transfers on all active channels before destroying the workqueue?

>  	if (!desc) {
>  		dev_err(dev, "Failed to prepare DMA\n");
>  		ret = -EIO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-dma_prep_config-v6-0-06e49b7acb38@nxp.com?part=8