Re: [PATCH v7 7/9] nvmet: pci-epf: Use dmaengine_prep_config_single_safe() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the DMA channel mutexes allows concurrent synchronous transfers on a shared DMA channel, leading to destructive error handling (cascading aborts), severe CPU starvation, spurious timeouts, and descriptor pool exhaustion.
--

commit 57a71fe2904773875ab4a20b8b7b4de2296d0683
Author: Frank Li <Frank.Li@nxp.com>

nvmet: pci-epf: Use dmaengine_prep_config_single_safe() API

This commit replaces the separate DMA channel configuration and descriptor
preparation steps with the new dmaengine_prep_config_single_safe() API.
It also removes the dma_tx_lock and dma_rx_lock mutexes, assuming they are
no longer needed because the new API call is atomic.

> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 2afe8f4d0e461..b1ba2d0bea6d9 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
[ ... ]
> @@ -388,22 +376,15 @@ static int nvmet_pci_epf_dma_transfer(struct nvmet_pci_epf *nvme_epf,
>  		return -EINVAL;
>  	}
>  
> -	mutex_lock(lock);
> -

[Severity: High]
Does removing this mutex expose the entire synchronous transfer lifecycle to
concurrent access?

While dmaengine_prep_config_single_safe() is atomic, the removed mutex
also protected the subsequent dmaengine_submit() and dma_sync_wait() calls
in this function. Since NVMe commands are processed concurrently by unbound
workqueues, multiple worker threads might now share the same global DMA
channel without synchronization.

If multiple threads concurrently spin in dma_sync_wait() on the same channel,
could this cause severe CPU starvation due to its tight busy-wait loop?

Additionally, dma_sync_wait() has a fixed 5-second timeout from the moment
it is called. With concurrent queuing, a newly submitted transfer's wait time
would include the execution of all preceding queued transfers on the channel.
Under high load, might this exceed 5 seconds and cause spurious timeouts?

If a timeout does occur, the error path calls dmaengine_terminate_sync(chan).
Will this forcefully abort all pending and active transfers on the channel,
destroying the in-flight DMA operations of all other concurrent NVMe commands?

Finally, unbounded concurrent submissions could potentially exhaust the DMA
engine's hardware descriptor pool under high I/O load, causing
dmaengine_prep_config_single_safe() to return NULL and fail valid commands.

>  	dma_dev = dmaengine_get_dma_device(chan);
>  	dma_addr = dma_map_single(dma_dev, seg->buf, seg->length, dir);
>  	ret = dma_mapping_error(dma_dev, dma_addr);
>  	if (ret)
> -		goto unlock;
> -
> -	ret = dmaengine_slave_config(chan, &sconf);
> -	if (ret) {
> -		dev_err(dev, "Failed to configure DMA channel\n");
> -		goto unmap;
> -	}
> +		return ret;
>  
> -	desc = dmaengine_prep_slave_single(chan, dma_addr, seg->length,
> -					   sconf.direction, DMA_CTRL_ACK);
> +	desc = dmaengine_prep_config_single_safe(chan, dma_addr, seg->length,
> +						 sconf.direction,
> +						 DMA_CTRL_ACK, &sconf);
[ ... ]
> @@ -426,9 +407,6 @@ static int nvmet_pci_epf_dma_transfer(struct nvmet_pci_epf *nvme_epf,
>  unmap:
>  	dma_unmap_single(dma_dev, dma_addr, seg->length, dir);
>  
> -unlock:
> -	mutex_unlock(lock);
> -
>  	return ret;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-dma_prep_config-v7-0-1f73f4899883@nxp.com?part=7