Re: [PATCH v4 17/18] nvmet: New NVMe PCI endpoint target driver

[Niklas Cassel <cassel@kernel.org>]

Hello Vinod,

I am a bit confused about the usage of the dmaengine API, and I hope that you
could help make me slightly less confused :)

If you look at the nvmet_pciep_epf_dma_transfer() function below, it takes a
mutex around the dmaengine_slave_config(), dmaengine_prep_slave_single(),
dmaengine_submit(), dma_sync_wait(), and dmaengine_terminate_sync() calls.

I really wish that we would remove this mutex, to get better performance.


If I look at e.g. the drivers/dma/dw-edma/dw-edma-core.c driver, I can see
that dmaengine_prep_slave_single() (which will call
device_prep_slave_sg(.., .., 1, .., .., ..)) allocates a new
dma_async_tx_descriptor for each function call.

I can see that device_prep_slave_sg() (dw_edma_device_prep_slave_sg()) will
call dw_edma_device_transfer() which will call vchan_tx_prep(), which adds
the descriptor to the tail of a list.

I can also see that dw_edma_done_interrupt() will automatically start the
transfer of the next descriptor (using vchan_next_desc()).

So this looks like it is supposed to be asynchronous... however, if we simply
remove the mutex, we get IOMMU errors, most likely because the DMA writes to
an incorrect address.

It looks like this is because dmaengine_prep_slave_single() really requires
dmaengine_slave_config() for each transfer. (Since we are supplying a src_addr
in the sconf that we are supplying to dmaengine_slave_config().)

(i.e. we can't call dmaengine_slave_config() while a DMA transfer is active.)

So while this API is supposed to be async, to me it looks like it can only
be used in a synchronous manner... But that seems like a really weird design.

Am I missing something obvious here?


If the dmaengine_prep_slave_single() instead took a sconfig as a parameter,
then it seems like it would be possible to use this API asynchronously.

There does seem to be other drivers holding a mutex around
dmaengine_slave_config() + dmaengine_prep_slave_single()... but it feels like
this should be avoidable (at least for dw-edma), if
dmaengine_prep_slave_single() simply took an sconf.

What am I missing? :)


Kind regards,
Niklas


On Thu, Dec 12, 2024 at 08:34:39PM +0900, Damien Le Moal wrote:
> +static int nvmet_pciep_epf_dma_transfer(struct nvmet_pciep_epf *nvme_epf,
> +		struct nvmet_pciep_segment *seg, enum dma_data_direction dir)
> +{
> +	struct pci_epf *epf = nvme_epf->epf;
> +	struct dma_async_tx_descriptor *desc;
> +	struct dma_slave_config sconf = {};
> +	struct device *dev = &epf->dev;
> +	struct device *dma_dev;
> +	struct dma_chan *chan;
> +	dma_cookie_t cookie;
> +	dma_addr_t dma_addr;
> +	struct mutex *lock;
> +	int ret;
> +
> +	switch (dir) {
> +	case DMA_FROM_DEVICE:
> +		lock = &nvme_epf->dma_rx_lock;
> +		chan = nvme_epf->dma_rx_chan;
> +		sconf.direction = DMA_DEV_TO_MEM;
> +		sconf.src_addr = seg->pci_addr;
> +		break;
> +	case DMA_TO_DEVICE:
> +		lock = &nvme_epf->dma_tx_lock;
> +		chan = nvme_epf->dma_tx_chan;
> +		sconf.direction = DMA_MEM_TO_DEV;
> +		sconf.dst_addr = seg->pci_addr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(lock);
> +
> +	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;
> +	}
> +
> +	desc = dmaengine_prep_slave_single(chan, dma_addr, seg->length,
> +					   sconf.direction, DMA_CTRL_ACK);
> +	if (!desc) {
> +		dev_err(dev, "Failed to prepare DMA\n");
> +		ret = -EIO;
> +		goto unmap;
> +	}
> +
> +	cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dev_err(dev, "DMA submit failed %d\n", ret);
> +		goto unmap;
> +	}
> +
> +	if (dma_sync_wait(chan, cookie) != DMA_COMPLETE) {
> +		dev_err(dev, "DMA transfer failed\n");
> +		ret = -EIO;
> +	}
> +
> +	dmaengine_terminate_sync(chan);
> +
> +unmap:
> +	dma_unmap_single(dma_dev, dma_addr, seg->length, dir);
> +
> +unlock:
> +	mutex_unlock(lock);
> +
> +	return ret;
> +}