| Message ID | 20241231131341.39292-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Migrate PCI Endpoint Subsystem tests to Kselftest | expand |
On Tue, Dec 31, 2024 at 06:43:39PM +0530, Manivannan Sadhasivam wrote: > IOCTLs are supposed to return 0 for success and negative error codes for > failure. Currently, this driver is returning 0 for failure and 1 for > success, that's not correct. Hence, fix it! > > Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com > Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++---------------- > tools/pci/pcitest.c | 51 +++---- > 2 files changed, 148 insertions(+), 153 deletions(-) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 5c99da952b7a..7d3f78b6f854 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) > test->irq_type = IRQ_TYPE_UNDEFINED; > } > > -static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > int type) > { > - int irq = -1; > + int irq; > struct pci_dev *pdev = test->pdev; > struct device *dev = &pdev->dev; > - bool res = true; > > switch (type) { > case IRQ_TYPE_INTX: > irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX); > - if (irq < 0) > + if (irq < 0) { > dev_err(dev, "Failed to get Legacy interrupt\n"); > + return -ENOSPC; > + } > + > break; > case IRQ_TYPE_MSI: > irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI); > - if (irq < 0) > + if (irq < 0) { > dev_err(dev, "Failed to get MSI interrupts\n"); > + return -ENOSPC; > + } > + > break; > case IRQ_TYPE_MSIX: > irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX); > - if (irq < 0) > + if (irq < 0) { > dev_err(dev, "Failed to get MSI-X interrupts\n"); > + return -ENOSPC;
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. 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 (...) > +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); > + } > +}
On Tue, Dec 31, 2024 at 05:57:47PM +0100, Niklas Cassel wrote: > On Tue, Dec 31, 2024 at 06:43:39PM +0530, Manivannan Sadhasivam wrote: > > IOCTLs are supposed to return 0 for success and negative error codes for > > failure. Currently, this driver is returning 0 for failure and 1 for > > success, that's not correct. Hence, fix it! > > > > Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com > > Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++---------------- > > tools/pci/pcitest.c | 51 +++---- > > 2 files changed, 148 insertions(+), 153 deletions(-) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index 5c99da952b7a..7d3f78b6f854 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) > > test->irq_type = IRQ_TYPE_UNDEFINED; > > } > > > > -static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > > +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > > int type) > > { > > - int irq = -1; > > + int irq; > > struct pci_dev *pdev = test->pdev; > > struct device *dev = &pdev->dev; > > - bool res = true; > > > > switch (type) { > > case IRQ_TYPE_INTX: > > irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX); > > - if (irq < 0) > > + if (irq < 0) { > > dev_err(dev, "Failed to get Legacy interrupt\n"); > > + return -ENOSPC; > > + } > > + > > break; > > case IRQ_TYPE_MSI: > > irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI); > > - if (irq < 0) > > + if (irq < 0) { > > dev_err(dev, "Failed to get MSI interrupts\n"); > > + return -ENOSPC; > > + } > > + > > break; > > case IRQ_TYPE_MSIX: > > irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX); > > - if (irq < 0) > > + if (irq < 0) { > > dev_err(dev, "Failed to get MSI-X interrupts\n"); > > + return -ENOSPC; > > From the pci_alloc_irq_vectors() kdoc: > * Return: number of allocated vectors (which might be smaller than > * @max_vecs), -ENOSPC if less than @min_vecs interrupt vectors are > * available, other errnos otherwise. > > So pci_alloc_irq_vectors() can return errors different than ENOSPC, > and in that case, you will overwrite that error. > Ack. > > > @@ -442,9 +444,12 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, > > val = wait_for_completion_timeout(&test->irq_raised, > > msecs_to_jiffies(1000)); > > if (!val) > > - return false; > > + return -ETIMEDOUT; > > > > - return pci_irq_vector(pdev, msi_num - 1) == test->last_irq; > > + if (!(pci_irq_vector(pdev, msi_num - 1) == test->last_irq)) > > if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) ? > > Or perhaps even: > > ret = pci_irq_vector(); > if (ret < 0) > return ret; > > if (ret != test->last_irq) > return -EIO; > Ack. > > Otherwise, this looks good to me: > Reviewed-by: Niklas Cassel <cassel@kernel.org> Thanks! - Mani
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
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
On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote: > > > 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?) Will do. > 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. > I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the test currently errors out, which is wrong. - Mani
Hello Mani, Vinod, On Thu, Jan 02, 2025 at 12:34:04PM +0530, Manivannan Sadhasivam wrote: > On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote: > > > > 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. > > > > I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local > addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should > be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the > test currently errors out, which is wrong. While I am okay with your suggested change to pci-epf-test.c: > >- if (epf_test->dma_private) { > >+ if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) { Since this will ensure that a DMA driver implementing DMA_MEMCPY, which cannot be shared (has DMA_PRIVATE set), will not error out. What I'm trying to explain is that in: https://lore.kernel.org/linux-pci/Z2BW4CjdE1p50AhC@vaman/ https://lore.kernel.org/linux-pci/20241217090129.6dodrgi4tn7l3cod@thinkpad/ Vinod (any you) suggested that we should add support for prep_memcpy() (which implies also setting cap DMA_MEMCPY) in the dw-edma DMA driver. However, from section "6.3 Using the DMA" in the DWC databook, the DWC eDMA hardware only supports: - Transfer (copy) of a block of data from local memory to remote memory. - Transfer (copy) of a block of data from remote memory to local memory. Currently, we have: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L843-L844 https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/dma/dw-edma/dw-edma-core.c#L215-L231 Where we can expose per-channel capabilities, so we set MEM_TO_DEV/DEV_TO_MEM per channel, however, these are returned in a struct dma_slave_caps *caps, so this is AFAICT only for DMA_SLAVE, not for DMA_MEMCPY. Looking at: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L975-L979 it seems that DMA_MEMCPY is always assumed to be MEM_TO_MEM. To me, it seems that we would either need a new dma_transaction_type (e.g. DMA_COPY) where we can set dir: MEM_TO_DEV, DEV_TO_MEM, or DEV_TO_DEV. (dw-edma would not support DEV_TO_DEV.) Or, if we should stick with DMA_MEMCPY, we would need another way of telling client drivers that only src or dst can be a remote address. Until this is solved, I think I will stop my work on adding DMA_MEMCPY to the dw-edma driver. Kind regards, Niklas