Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel <cassel@kernel.org>]

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, &param);
		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, &param);
		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, &param);
		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.


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?


Since your commit 392188bb0f6e: each test case calls PCITEST_SET_IRQTYPE,
and gives an error if the PCITEST_SET_IRQTYPE ioctl() fails.

See Kunihiko commit log:
"... all tests that use interrupts first call ioctl(SET_IRQTYPE)
to set "test->irq_type", then write the value of test->irq_type into
the register pointed by test_reg_bar, and request the interrupt to the
endpoint. The endpoint function driver, pci-epf-test, refers to the
register, and determine which type of interrupt to raise."

READ/WRITE/COPY test cases/ioctls use interrupts.


I guess we could modify the read/write/copy test cases to not call
ioctl(SET_IRQTYPE), and remove the verification that ioctl(SET_IRQTYPE)
succeded, and change the behavior from older kernel releases, and make
READ/WRITE/COPY ioctls from now on ignore the configured irq_type using
ioctl(SET_IRQTYPE).

(If the user is using a selftest binary that has the ioctl(SET_IRQTYPE)
and ioctl(SET_IRQTYPE) verification in these test cases, the IRQ_TYPE
will get changed, so the verification will pass, but the succeeding
ioctl will not use that irq_type.)

But...

Is that really simpler / less confusing that just adding IRQ_TYPE_AUTO,
to maintain the uniformity that all test cases that will trigger IRQs
call ioctl(SET_IRQTYPE) before the actual ioctl that will trigger IRQs?


Kind regards,
Niklas