On Fri, Mar 14, 2025 at 06:25:54PM +0100, Niklas Cassel wrote:
> Hello Mani,
>
> On Fri, Mar 14, 2025 at 06:15:48PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 10, 2025 at 12:10:24PM +0100, Niklas Cassel wrote:
> > > For PCITEST_MSI we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_MSI, since we want to test if MSI works.
> > >
> > > For PCITEST_MSIX we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_MSIX, since we want to test if MSI works.
> > >
> > > For PCITEST_LEGACY_IRQ we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_INTX, since we want to test if INTx works.
> > >
> > > However, for PCITEST_WRITE, PCITEST_READ, PCITEST_COPY, we really don't
> > > care which IRQ type that is used, we just want to use a IRQ type that is
> > > supported by the EPC.
> > >
> > > The old behavior was to always use MSI for PCITEST_WRITE, PCITEST_READ,
> > > PCITEST_COPY, was to always set IRQ type to MSI before doing the actual
> > > test, however, there are EPC drivers that do not support MSI.
> > >
> > > Add a new PCITEST_IRQ_TYPE_AUTO, that will use the CAPS register to see
> > > which IRQ types the endpoint supports, and use one of the supported IRQ
> > > types.
> > >
> >
> > If the intention is to let the test figure out the supported IRQ type, why can't
> > you move the logic to set the supported IRQ to
> > pci_endpoint_test_{copy/read/write} functions itself?
>
> If you look at how things were before your commit 392188bb0f6e ("selftests:
> pci_endpoint: Migrate to Kselftest framework"):
>
> echo "Read Tests"
> echo
>
> pcitest -i 1
>
> pcitest -r -s 1
> pcitest -r -s 1024
> pcitest -r -s 1025
> pcitest -r -s 1024000
> pcitest -r -s 1024001
> echo
>
> echo "Write Tests"
> echo
>
> pcitest -w -s 1
> pcitest -w -s 1024
> pcitest -w -s 1025
> pcitest -w -s 1024000
> pcitest -w -s 1024001
> echo
>
> echo "Copy Tests"
> echo
>
> pcitest -c -s 1
> pcitest -c -s 1024
> pcitest -c -s 1025
> pcitest -c -s 1024000
> pcitest -c -s 1024001
>
>
> All three test cases were using MSI.
>
>
> However, there was no error handling, so for a platform only supporting
> MSI-X, the call to "pcitest -i 1" could fail, and the tests were still
> going to use MSI-X (or whatever supported by the platform), and pass.
>
>
>
> After your commit 392188bb0f6e ("selftests: pci_endpoint: Migrate to
> Kselftest framework"):
>
>
> TEST_F(pci_ep_data_transfer, READ_TEST)
> {
> struct pci_endpoint_test_xfer_param param = {};
> int ret, i;
>
> if (variant->use_dma)
> param.flags = PCITEST_FLAGS_USE_DMA;
>
> pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
>
> for (i = 0; i < ARRAY_SIZE(test_size); i++) {
> param.size = test_size[i];
> pci_ep_ioctl(PCITEST_READ, ¶m);
> EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> test_size[i]);
> }
> }
>
> TEST_F(pci_ep_data_transfer, WRITE_TEST)
> {
> struct pci_endpoint_test_xfer_param param = {};
> int ret, i;
>
> if (variant->use_dma)
> param.flags = PCITEST_FLAGS_USE_DMA;
>
> pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
>
> for (i = 0; i < ARRAY_SIZE(test_size); i++) {
> param.size = test_size[i];
> pci_ep_ioctl(PCITEST_WRITE, ¶m);
> EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> test_size[i]);
> }
> }
>
> TEST_F(pci_ep_data_transfer, COPY_TEST)
> {
> struct pci_endpoint_test_xfer_param param = {};
> int ret, i;
>
> if (variant->use_dma)
> param.flags = PCITEST_FLAGS_USE_DMA;
>
> pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
>
> for (i = 0; i < ARRAY_SIZE(test_size); i++) {
> param.size = test_size[i];
> pci_ep_ioctl(PCITEST_COPY, ¶m);
> EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> test_size[i]);
> }
> }
>
>
> Each test case explicitly calls ioctl(PCITEST_SET_IRQTYPE, 1); to use MSI,
> and unlike before, will fail the test case if PCITEST_SET_IRQTYPE fails.
>
Thanks for spotting the breakage!
>
> Then take Kunihiko commit 9240c27c3fdd ("misc: pci_endpoint_test: Remove
> global 'irq_type' and 'no_msi'")
>
> Before your and Kunihiko commits, a platform that did set the kernel module
> parameter 'irq_type' would, if 'pcitest -i 1' failed, use the value set by
> that kernel module parameter for the read/write/copy test cases.
>
> There is no guarantee that an EPC supports MSI, it might support only legacy
> and INTx, so I was trying to restore use cases that were previously working.
>
>
>
> I guess one option would be to remove the
> "pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);" calls from the test cases that you
> added, and then let the test cases themselves set the proper irq_type in
> the BAR register. But, wouldn't that be an API change? READ/WRITE/COPY
> test ioctls have always respected the (a successful) PCITEST_SET_IRQTYPE,
> now all of a sudden, they shouldn't?
>
This makes no difference IMO. The previous behavior which you explained above,
ignored the result of 'pcitest -i 1'. And it was not user configurable. I think
the original intention was to use MSI for tests if available, else use whatever
the platform supports.
If you want to restore the original behavior, you should remove the ASSERT_EQ()
from READ/WRITE/COPY tests first. Then to ensure that the tests make use of the
supported IRQ type, you can have the logic in the READ/WRITE/COPY tests itself.
If test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED, then just use whatever the
test->irq_type is. Otherwise, use whatever the platform supports.
I don't see a necessity to add a new IRQ_TYPE and then getting it passed to the
host, if the host can figure out the IRQ_TYPE on its own.
- Mani
--
மணிவண்ணன் சதாசிவம்
