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

[Niklas Cassel <cassel@kernel.org>]

On Tue, Dec 17, 2024 at 02:31:29PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 17, 2024 at 11:51:44AM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Dec 17, 2024 at 10:57:02AM +0530, Vinod Koul wrote:
> > > On 16-12-24, 11:12, Damien Le Moal wrote:
> > > > On 2024/12/16 8:35, Vinod Koul wrote:
> > > > > 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...
> > > > 
> > > > Vinod,
> > > > 
> > > > Thank you for the information. However, I think we can use this only if the DMA
> > > > controller driver implements the device_prep_dma_memcpy operation, no ?
> > > > In our case, the DWC EDMA driver does not seem to implement this.
> > > 
> > > It should be added in that case.
> > > 
> > > Before that, the bigger question is, should nvme be slave transfer or
> > > memcpy.. Was driver support the reason why the slave transfer was used here...?
> > > 
> > > As i said, slave is for peripherals which have a static fifo to
> > > send/receive data from, nvme sounds like a memory transfer to me, is
> > > that a right assumption?
> > > 
> > 
> > My understanding is that DMA_MEMCPY is for local DDR transfer i.e., src and dst
> > are local addresses. And DMA_SLAVE is for transfer between remote and local
> > addresses. I haven't looked into the NVMe EPF driver yet, but it should do
> > the transfer between remote and local addresses. This is similar to MHI EPF
> > driver as well.
> > 
> 
> I had an offline discussion with Vinod and he clarified that the DMA_SLAVE
> implementation is supposed to be used by clients with fixed FIFO. That's why
> 'struct dma_slave_config' has options to configure maxburst, addr_width,
> port_window_size etc... So these are not applicable for our usecase where we
> would be just carrying out transfer between remote and local DDR.
> 
> So we should be implementing prep_memcpy() in dw-edma driver and use DMA_MEMCPY
> in client drivers.
> 
> @Niklas: Since you asked this question, are you volunteering to implement
> prep_memcpy() in dw-edma driver? ;)

I did implement something that seems to be working.

It would be nice if you could help testing it using your MHI EPF driver.
(You need to convert it to use _prep_memcpy() in order to be able to test.)


Kind regards,
Niklas