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

[Vinod Koul <vkoul@kernel.org>]

Hi Niklas,

On 13-12-24, 17:59, Niklas Cassel wrote:
> 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 :)

Sure thing!

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

Yes, I feel nvme being treated as slave transfer, which it might not be.
This API was designed for peripherals like i2c/spi etc where we have a
hardware address to read/write to. So the dma_slave_config would pass on
the transfer details for the peripheral like address, width of fifo,
depth etc and these are setup config, so call once for a channel and then
prepare the descriptor, submit... and repeat of prepare and submit ...

I suspect since you are passing an address which keep changing in the
dma_slave_config, you need to guard that and prep_slave_single() call,
as while preparing the descriptor driver would lookup what was setup for
the configuration.

I suggest then use the prep_memcpy() API instead and pass on source and
destination, no need to lock the calls...

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

-- 
~Vinod