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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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;

> Not sure how to formulate this properly... Perhaps:
> Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for DMA drivers that
> have the DMA_PRIVATE cap set. For DMA drivers which have the DMA_PRIVATE cap
> set, it is advisable to skip the forementioned testcase using below command::
> 
> > +
> > +	# pci_endpoint_test -f pci_ep_basic -v memcpy -T COPY_TEST -v dma
> 
> Is this really correct? I would guess that it should be
> pci_endpoint_test -f pci_ep_data_transfer -v memcpy -T COPY_TEST -v dma
> 

ah, typo. Thanks for catching!

> 
> (...)
> 
> > +TEST_F(pci_ep_basic, BAR_TEST)
> > +{
> > +	int ret, i;
> > +
> > +	for (i = 0; i <= 5; i++) {
> > +		pci_ep_ioctl(PCITEST_BAR, i);
> > +		EXPECT_FALSE(ret) TH_LOG("Test failed for BAR%d", i);
> > +	}
> > +}
> 
> From looking at this function, will we still be able to test a single BAR?
> Previous pcitest.c allowed us to do pcitest -b <barno> to only test a
> specific BAR. I think that is a useful feature that we shouldn't remove.
> 
> It would be nice if we could do something like:
> # pci_endpoint_test -f pci_ep_basic -T BAR_TEST -v <barno>
> 

I'll try to add it.

> 
> (...)
> 
> > +
> > +TEST_F(pci_ep_data_transfer, COPY_TEST)
> > +{
> > +	struct pci_endpoint_test_xfer_param param = {0};
> 
> This (also other places in this file) can be written as:
> struct pci_endpoint_test_xfer_param param = {};
> 

Ack.

- Mani

-- 
மணிவண்ணன் சதாசிவம்