Re: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

[Niklas Cassel <cassel@kernel.org>]

On 31 December 2024 20:18:12 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>On Tue, Dec 31, 2024 at 06:42:42PM +0100, Niklas Cassel wrote:
>> On Tue, Dec 31, 2024 at 06:43:41PM +0530, Manivannan Sadhasivam wrote:
>> 
>> (...)
>> 
>> > +	#  RUN           pci_ep_data_transfer.dma.COPY_TEST ...
>> > +	#            OK  pci_ep_data_transfer.dma.COPY_TEST
>> > +	ok 11 pci_ep_data_transfer.dma.COPY_TEST
>> > +	# PASSED: 11 / 11 tests passed.
>> > +	# Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0
>> > +
>> > +
>> > +Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA
>> > +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such
>> > +controllers, it is advisable to skip the forementioned testcase using below
>> > +command::
>> 
>> Hm.. this is strictly not correct. If will currently fail because pci-epf-test.c
>> does:
>> if ((reg->flags & FLAG_USE_DMA) && epf_test->dma_private)
>> 	return -EINVAL;
>> 
>> So even if a DMA driver has support for the DMA_MEMCPY cap, if the DMA driver
>> also has the DMA_PRIVATE cap, this test will fail because of the code in
>> pci-epf-test.c.
>> 
>
>Right. But I think the condition should be changed to test for the MEMCPY
>capability instead. Like,
>
>diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>index ef6677f34116..0b211d60a85b 100644
>--- a/drivers/pci/endpoint/functions/pci-epf-test.c
>+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>@@ -328,7 +328,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>        void *copy_buf = NULL, *buf;
> 
>        if (reg->flags & FLAG_USE_DMA) {
>-               if (epf_test->dma_private) {
>+               if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {
>                        dev_err(dev, "Cannot transfer data using DMA\n");
>                        ret = -EINVAL;
>                        goto set_status;
>

That check does seem to make more sense than the code that is currently there.
(Perhaps send this as a proper patch?)
Note that I'm not an expert at dmaengine.

I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.


Kind regards,
Niklas