Re: [PATCH v5 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 2 potential issue(s) to consider:
- [High] Removing the DMA serialization mutex causes severe CPU starvation, spurious timeouts, and cascading DMA transfer aborts.
- [Low] Unused local variable triggers a compiler warning and leaves dead mutexes in the device structure.
--

commit 4b68a703ef44d7ed4b6b20d869ad94bab23d5076
Author: Frank Li <Frank.Li@nxp.com>

nvmet: pci-epf: Use dmaengine_prep_config_single_safe() API

This commit uses the new dmaengine_prep_config_single_safe() API to combine
DMA configuration and descriptor preparation into a single call. It also
removes the mutex that was previously used in the DMA transfer path.

> Since dmaengine_prep_config_single_safe() performs the configuration and
> preparation atomically and the mutex can be removed.

Is this description entirely accurate?

While the configuration and preparation might now be atomic, the mutex also
strictly serialized the DMA transaction pipeline.

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

Does removing this mutex introduce concurrency regressions with the shared DMA
channel during the wait phase?

With the mutex removed, multiple concurrent NVMe worker threads can submit
transfers to the same channel. Since dma_sync_wait() busy-waits using
cpu_relax(), could multiple threads spinning simultaneously lead to severe CPU
starvation?

Additionally, dma_sync_wait() has a hardcoded 5-second timeout computed at
entry. If concurrent transfers queue up, could later transfers spuriously
time out while waiting for earlier ones to finish in hardware?

If a spurious timeout occurs, the error path further down calls
dmaengine_terminate_sync(chan). Does this unconditionally abort all active
and pending transfers on the shared channel, potentially causing cascading
data transfer failures?

Could unbounded concurrent submissions also exhaust the DMA channel's
fixed-size descriptor pool under load?

> -
>  	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;

[ ... ]

> @@ -426,9 +419,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;
>  }

This isn't a bug, but does leaving the local lock variable and its assignments
in the preceding switch statement trigger a -Wunused-but-set-variable compiler
warning?

Also, since dma_rx_lock and dma_tx_lock in struct nvmet_pci_epf are no
longer used anywhere else, should they be removed from the structure to avoid
leaving dead code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-dma_prep_config-v5-0-26865bf7d935@nxp.com?part=7