On Fri, Jan 31, 2025 at 07:16:54PM +0900, Kunihiko Hayashi wrote:
Hi Niklas,
On 2025/01/29 20:58, Niklas Cassel wrote:
On Tue, Jan 28, 2025 at 08:02:31PM +0530, Manivannan Sadhasivam wrote:
On Wed, Jan 22, 2025 at 11:24:46AM +0900, Kunihiko Hayashi wrote:
There are two variables that indicate the interrupt type to be used in the next test execution, "irq_type" as global and test->irq_type.
The global is referenced from pci_endpoint_test_get_irq() to preserve the current type for ioctl(PCITEST_GET_IRQTYPE).
The type set in this function isn't reflected in the global "irq_type", so ioctl(PCITEST_GET_IRQTYPE) returns the previous type. As a result, the wrong type will be displayed in "pcitest" as follows:
# pcitest -i 0 SET IRQ TYPE TO LEGACY: OKAY # pcitest -I GET IRQ TYPE: MSIFix this issue by propagating the current type to the global "irq_type".
This is becoming a nuisance. I think we should get rid of the global 'irq_type' and just stick to the one that is configurable using IOCTL command. Even if the user has configured the global 'irq_type' it is bound to change with IOCTL command.
+1
After fixing the issue described in this patch, we can replace with a new member of 'struct pci_endpoint_test' instead.
Sorry, but I don't understand what you mean here. You want this patch to be applied. Then you want to add a new struct member to struct pci_endpoint_test? struct pci_endpoint_test already has a struct member named irq_type, so why do you want to add a new member?
Like Mani suggested, I think it would be nice if we could remove the global irq_type kernel module parameter, and change so that PCITEST_GET_IRQTYPE returns test->irq_type.
Note that your series does not apply to pci/next, and needs to be rebased. (It conflicts with f26d37ee9bda ("misc: pci_endpoint_test: Fix IOCTL return value"))
But I also don't like how since we migrated to selftests: READ_TEST / WRITE_TEST / COPY_TEST unconditionally call ioctl(PCITEST_SET_IRQTYPE, MSI) before doing their thing.
I think that it's better to prepare new patch series.
Here, I was pointing out what I think is a regression with the migration to selftests. (Which is unrelated to this series.)
I do suggest a way to improve things futher down (introducing a PCITEST_SET_IRQTYPE, AUTO), but I don't think that you need to be the one implementing this suggestion, since you did not introduce this regression.
Will this cause the test case to fail for platforms that only support MSI-X? (See e.g. dwc/pci-layerscape-ep.c where this could be the case.)
Sure, before, in pcitest.sh, we would do:
pcitest -i 2 pcitest -x $msix
pcitest -i 1
pcitest -r -s 1 pcitest -r -s 1024 pcitest -r -s 1025 pcitest -r -s 1024000 pcitest -r -s 1024001
Which would probably print an error if: pcitest -i 1 failed.
but the READ_TEST / WRITE_TEST / COPY_TEST tests themselves would not fail.
Perhaps we should rethink this, and introduce a new PCITEST_SET_IRQTYPE, AUTO
I would be fine if READ_TEST / WRITE_TEST / COPY_TEST called PCITEST_SET_IRQTYPE, AUTO before doing their thing.
How I suggest PCITEST_SET_IRQTYPE, AUTO would work:
Since we now have capabilties merged: https://lore.kernel.org/linux-pci/20241203063851.695733-4-cassel@kernel.org/
Add epc_features->msi_capable and epc->features->msix_capable as two new bits in the PCI_ENDPOINT_TEST_CAPS register.
If PCITEST_SET_IRQTYPE, AUTO: if EP CAP has msi_capable set: set IRQ type MSI else if EP CAP has msix_capable set: set IRQ type MSI-X else: set legacy/INTx
There is something ambiguous about the behavior for me.
The test->irq_type has a state "UNDEFINED". After issueing "Clear IRQ", test->irq_type becomes "UNDEFINED" currently, and all tests with IRQs will fail until new test->irq_type is set.
That is fine, no?
If a user calls PCITEST_CLEAR_IRQ without doing a PCITEST_SET_IRQTYPE afterwards, what else can the tests relying on IRQs to other than fail?
FWIW, tools/testing/selftests/pci_endpoint/pci_endpoint_test.c never does an ioctl(PCITEST_CLEAR_IRQ).
If SET_IRQTYPE is AUTO, how will test->irq_type be set?
I was thinking something like this:
pci_endpoint_test_set_irq() { u32 caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
...
if (req_irq_type == IRQ_TYPE_AUTO) { if (caps & MSI_CAPABLE) test->irq_type = IRQ_TYPE_MSI; else if (caps & MSIX_CAPABLE) test->irq_type = IRQ_TYPE_MSIX; else test->irq_type = IRQ_TYPE_INTX;
}
... }
Kind regards, Niklas