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: MSI
Fix this issue by propagating the current type to the global "irq_type".
Cc: stable@vger.kernel.org Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype") Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- drivers/misc/pci_endpoint_test.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index a342587fc78a..33058630cd50 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -742,6 +742,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, if (!pci_endpoint_test_request_irq(test)) goto err;
+ irq_type = test->irq_type; return true;
err:
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.
- Mani
Cc: stable@vger.kernel.org Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype") Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com
drivers/misc/pci_endpoint_test.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index a342587fc78a..33058630cd50 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -742,6 +742,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, if (!pci_endpoint_test_request_irq(test)) goto err;
- irq_type = test->irq_type; return true;
err: -- 2.25.1
Hi Manivannan,
On 2025/01/28 23:32, 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.
We can add a new member of 'struct pci_endpoint_test' instead of the global 'irq_type'.
If I remove the global 'irq_type', the following module parameter description will also be removed.
module_param(irq_type, int, 0444); MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
I'm concerned about compatibility. Is there any problem with removing this?
Thank you,
--- Best Regards Kunihiko Hayashi
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
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.
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
Kind regards, Niklas
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.
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.
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.
If SET_IRQTYPE is AUTO, how will test->irq_type be set?
Thank you,
--- Best Regards Kunihiko Hayashi
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
On Fri, Jan 31, 2025 at 01:10:54PM +0100, Niklas Cassel wrote:
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;
}
... }
Or even simpler (since it requires less changes to pci_endpoint_test_set_irq()):
if (req_irq_type == IRQ_TYPE_AUTO) { if (caps & MSI_CAPABLE) req_irq_type = IRQ_TYPE_MSI; else if (caps & MSIX_CAPABLE) req_irq_type = IRQ_TYPE_MSIX; else req_irq_type = IRQ_TYPE_INTX;
}
...
/* Sets test->irq_type = req_irq_type; on success */ pci_endpoint_test_alloc_irq_vectors();
Kind regards, Niklas
On Fri, Jan 31, 2025 at 01:20:38PM +0100, Niklas Cassel wrote:
On Fri, Jan 31, 2025 at 01:10:54PM +0100, Niklas Cassel wrote:
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;
}
... }
Or even simpler (since it requires less changes to pci_endpoint_test_set_irq()):
if (req_irq_type == IRQ_TYPE_AUTO) { if (caps & MSI_CAPABLE) req_irq_type = IRQ_TYPE_MSI; else if (caps & MSIX_CAPABLE) req_irq_type = IRQ_TYPE_MSIX; else req_irq_type = IRQ_TYPE_INTX;
}
...
/* Sets test->irq_type = req_irq_type; on success */ pci_endpoint_test_alloc_irq_vectors();
See attached patch.
Mani, removing the global irq_type would go on top of this.
Kind regards, Niklas
Hi Niklas,
On 2025/01/31 21:10, Niklas Cassel wrote:
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?
Sorry for confusion.
The internal state (test->irq_type) has the state IRQ_TYPE_UNDEFINED and the global irq_type doesn't. Then I've thought that ioctl(GET_IRQTYPE) should not return UNDEFINED state.
However, ioctl(GET_IRQTYPE) can return with an error if the state is UNDEFINED. This is new behavior, but not inconsistent. So the additional irq_type is no longer necessary.
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.
I see. I'll add new patch to remove the global irq_type and replace with 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"))
Yes, I've confirmed the conflict and I'll rebase it next.
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.
Okay, I expect another topic after we remove the global variables.
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.or g/
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).
As mentioned above, PCITEST_GET_IRQTYPE can fail with an error, and I understand that this will not affect the test.
Thank you,
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
--- Best Regards Kunihiko Hayashi
linux-stable-mirror@lists.linaro.org
-
Kunihiko Hayashi -
Manivannan Sadhasivam -
Niklas Cassel