Re: [PATCH] PCI: endpoint: Add DMA to Linux PCI EP Framework

[Kishon Vijay Abraham I]

Hi Alan,

On 31/05/19 11:46 PM, Alan Mikhak wrote:
> On Thu, May 30, 2019 at 10:08 PM Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>> Hi Alan,
>>>
>>> Hi Kishon,
>>>
>>> I have some improvements in mind for a v2 patch in response to
>>> feedback from Gustavo Pimentel that the current implementation is HW
>>> specific. I hesitate from submitting a v2 patch because it seems best
>>> to seek comment on possible directions this may be taking.
>>>
>>> One alternative is to wait for or modify test functions in
>>> pci-epf-test.c to call DMAengine client APIs, if possible. I imagine
>>> pci-epf-test.c test functions would still allocate the necessary local
>>> buffer on the endpoint side for the same canned tests for everyone to
>>> use. They would prepare the buffer in the existing manner by filling
>>> it with random bytes and calculate CRC in the case of a write test.
>>> However, they would then initiate DMA operations by using DMAengine
>>> client APIs in a generic way instead of calling memcpy_toio() and
>>> memcpy_fromio(). They would post-process the buffer in the existing
>>
>> No, you can't remove memcpy_toio/memcpy_fromio APIs. There could be platforms
>> without system DMA or they could have system DMA but without MEMCOPY channels
>> or without DMA in their PCI controller.
> 
> I agree. I wouldn't remove memcpy_toio/fromio. That is the reason this
> patch introduces the '-d' flag for pcitest to communicate that user
> intent across the PCIe bus to pci-epf-test so the endpoint can
> initiate the transfer using either memcpy_toio/fromio or DMA.
> 
>>> manner such as the checking for CRC in the case of a read test.
>>> Finally, they would release the resources and report results back to
>>> the user of pcitest across the PCIe bus through the existing methods.
>>>
>>> Another alternative I have in mind for v2 is to change the struct
>>> pci_epc_dma that this patch added to pci-epc.h from the following:
>>>
>>> struct pci_epc_dma {
>>>         u32     control;
>>>         u32     size;
>>>         u64     sar;
>>>         u64     dar;
>>> };
>>>
>>> to something similar to the following:
>>>
>>> struct pci_epc_dma {
>>>         size_t  size;
>>>         void *buffer;
>>>         int flags;
>>> };
>>>
>>> The 'flags' field can be a bit field or separate boolean values to
>>> specify such things as linked-list mode vs single-block, etc.
>>> Associated #defines would be removed from pci-epc.h to be replaced if
>>> needed with something generic. The 'size' field specifies the size of
>>> DMA transfer that can fit in the buffer.
>>
>> I still have to look closer into your DMA patch but linked-list mode or single
>> block mode shouldn't be an user select-able option but should be determined by
>> the size of transfer.
> 
> Please consider the following when taking a closer look at this patch.

After seeing comments from Vinod and Arnd, it looks like the better way of
adding DMA support would be to register DMA within PCI endpoint controller to
DMA subsystem (as dmaengine) and use only dmaengine APIs in pci_epf_test.
> 
> In my specific use case, I need to verify that any valid block size,
> including a one byte transfer, can be transferred across the PCIe bus
> by memcpy_toio/fromio() or by DMA either as a single block or as
> linked-list. That is why, instead of deciding based on transfer size,
> this patch introduces the '-L' flag for pcitest to communicate the
> user intent across the PCIe bus to pci-epf-test so the endpoint can
> initiate the DMA transfer using a single block or in linked-list mode.
The -L option seems to select an internal DMA configuration which might be
specific to one implementation. As Gustavo already pointed, we should have only
generic options in pcitest. This would no longer be applicable when we move to
dmaengine.

Thanks
Kishon