| Message ID | 20250122022446.2898248-4-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix some issues related to an interrupt type in pci_endpoint_test | expand |
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: MSI > > Fix 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: MSI >> >> Fix 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: MSI > > > > Fix 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: MSI >>> >>> Fix 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: MSI > > > > > > > > Fix 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 diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index d5ac71a49386..5e42930124e2 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -32,6 +32,7 @@ #define IRQ_TYPE_INTX 0 #define IRQ_TYPE_MSI 1 #define IRQ_TYPE_MSIX 2 +#define IRQ_TYPE_AUTO 3 #define PCI_ENDPOINT_TEST_MAGIC 0x0 @@ -71,6 +72,8 @@ #define PCI_ENDPOINT_TEST_CAPS 0x30 #define CAP_UNALIGNED_ACCESS BIT(0) +#define CAP_MSI BIT(1) +#define CAP_MSIX BIT(2) #define PCI_DEVICE_ID_TI_AM654 0xb00c #define PCI_DEVICE_ID_TI_J7200 0xb00f @@ -126,6 +129,7 @@ struct pci_endpoint_test { struct miscdevice miscdev; enum pci_barno test_reg_bar; size_t alignment; + u32 ep_caps; const char *name; }; @@ -805,11 +809,20 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test, struct device *dev = &pdev->dev; int ret; - if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_MSIX) { + if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_AUTO) { dev_err(dev, "Invalid IRQ type option\n"); return -EINVAL; } + if (req_irq_type == IRQ_TYPE_AUTO) { + if (test->ep_caps & CAP_MSI) + req_irq_type = IRQ_TYPE_MSI; + else if (test->ep_caps & CAP_MSIX) + req_irq_type = IRQ_TYPE_MSIX; + else + req_irq_type = IRQ_TYPE_INTX; + } + if (test->irq_type == req_irq_type) return 0; @@ -895,13 +908,12 @@ static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test) { struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev; - u32 caps; - caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS); - dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps); + test->ep_caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS); + dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", test->ep_caps); /* CAP_UNALIGNED_ACCESS is set if the EP can do unaligned access */ - if (caps & CAP_UNALIGNED_ACCESS) + if (test->ep_caps & CAP_UNALIGNED_ACCESS) test->alignment = 0; } diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index b94e205ae10b..8917f7c6c741 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -45,6 +45,8 @@ #define TIMER_RESOLUTION 1 #define CAP_UNALIGNED_ACCESS BIT(0) +#define CAP_MSI BIT(1) +#define CAP_MSIX BIT(2) static struct workqueue_struct *kpcitest_workqueue; @@ -753,6 +755,12 @@ static void pci_epf_test_set_capabilities(struct pci_epf *epf) if (epc->ops->align_addr) caps |= CAP_UNALIGNED_ACCESS; + if (epf_test->epc_features->msi_capable) + caps |= CAP_MSI; + + if (epf_test->epc_features->msix_capable) + caps |= CAP_MSIX; + reg->caps = cpu_to_le32(caps); } diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h index acd261f49866..1edbd4357470 100644 --- a/include/uapi/linux/pcitest.h +++ b/include/uapi/linux/pcitest.h @@ -23,6 +23,11 @@ #define PCITEST_BARS _IO('P', 0xa) #define PCITEST_CLEAR_IRQ _IO('P', 0x10) +#define PCITEST_IRQ_TYPE_INTX 0 +#define PCITEST_IRQ_TYPE_MSI 1 +#define PCITEST_IRQ_TYPE_MSIX 2 +#define PCITEST_IRQ_TYPE_AUTO 3 + #define PCITEST_FLAGS_USE_DMA 0x00000001 struct pci_endpoint_test_xfer_param { diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c index c267b822c108..c820a67e6437 100644 --- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c +++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c @@ -97,7 +97,7 @@ TEST_F(pci_ep_basic, LEGACY_IRQ_TEST) { int ret; - pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_INTX); ASSERT_EQ(0, ret) TH_LOG("Can't set Legacy IRQ type"); pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0); @@ -108,7 +108,7 @@ TEST_F(pci_ep_basic, MSI_TEST) { int ret, i; - pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI); ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type"); for (i = 1; i <= 32; i++) { @@ -121,7 +121,7 @@ TEST_F(pci_ep_basic, MSIX_TEST) { int ret, i; - pci_ep_ioctl(PCITEST_SET_IRQTYPE, 2); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSIX); ASSERT_EQ(0, ret) TH_LOG("Can't set MSI-X IRQ type"); for (i = 1; i <= 2048; i++) { @@ -170,8 +170,8 @@ TEST_F(pci_ep_data_transfer, READ_TEST) 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"); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO); + ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type"); for (i = 0; i < ARRAY_SIZE(test_size); i++) { param.size = test_size[i]; @@ -189,8 +189,8 @@ TEST_F(pci_ep_data_transfer, WRITE_TEST) 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"); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO); + ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type"); for (i = 0; i < ARRAY_SIZE(test_size); i++) { param.size = test_size[i]; @@ -208,8 +208,8 @@ TEST_F(pci_ep_data_transfer, COPY_TEST) 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"); + pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO); + ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type"); for (i = 0; i < ARRAY_SIZE(test_size); i++) { param.size = test_size[i];
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: MSI >>>>> >>>>> Fix 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
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:
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(+)