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.
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
