This series solves some issues about global "irq_type" that is used for indicating the current type for users.
In addition, avoid an unexpected warning that occur due to interrupts remaining after displaying an error caused by devm_request_irq().
Patch 1 includes adding GET_IRQTYPE test (check for failure). Patch 2-4 include fixes for stable kernels that have global "irq_type". Patch 5-6 include improvements for the latest.
Changes since v3: - Add GET_IRQTYPE check to pci_endpoint test in selftests - Add the reason why global variables aren't necessary (patch 5/6) - Add Reviewed-by: lines (patch {2, 4, 6}/6)
Changes since v2: - Rebase to v6.14-rc1 - Update message to clarify, and add result of call trace (patch 1/5) - Add Reviewed-by: lines (patch 2/5) - Add new patch to remove global "irq_type" variable (patch 4/5) - Add new patch to replace "devm" version of IRQ functions (patch 5/5)
Changes since v1: - Divide original patch into two - Add an error message example - Add "pcitest" display example - Add a patch to fix an interrupt remaining issue
Kunihiko Hayashi (6): selftests: pci_endpoint: Add GET_IRQTYPE checks to each interrupt test misc: pci_endpoint_test: Avoid issue of interrupts remaining after request_irq error misc: pci_endpoint_test: Fix displaying irq_type after request_irq error misc: pci_endpoint_test: Fix irq_type to convey the correct type misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi' misc: pci_endpoint_test: Do not use managed irq functions
drivers/misc/pci_endpoint_test.c | 31 +++++++------------ .../pci_endpoint/pci_endpoint_test.c | 11 ++++++- 2 files changed, 21 insertions(+), 21 deletions(-)
Add GET_IRQTYPE API checks to each interrupt test. And change pci_ep_ioctl() to get the appropriate return value from ioctl().
Suggested-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- .../selftests/pci_endpoint/pci_endpoint_test.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c index 576c590b277b..d05e107d0698 100644 --- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c +++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c @@ -25,7 +25,7 @@ #define pci_ep_ioctl(cmd, arg) \ ({ \ ret = ioctl(self->fd, cmd, arg); \ - ret = ret < 0 ? -errno : 0; \ + ret = ret < 0 ? -errno : ret; \ })
static const char *test_device = "/dev/pci-endpoint-test.0"; @@ -102,6 +102,9 @@ TEST_F(pci_ep_basic, LEGACY_IRQ_TEST) pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0); ASSERT_EQ(0, ret) TH_LOG("Can't set Legacy IRQ type");
+ pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0); + ASSERT_EQ(0, ret) TH_LOG("Can't get Legacy IRQ type"); + pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0); EXPECT_FALSE(ret) TH_LOG("Test failed for Legacy IRQ"); } @@ -113,6 +116,9 @@ TEST_F(pci_ep_basic, MSI_TEST) pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+ pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0); + ASSERT_EQ(1, ret) TH_LOG("Can't get MSI IRQ type"); + for (i = 1; i <= 32; i++) { pci_ep_ioctl(PCITEST_MSI, i); EXPECT_FALSE(ret) TH_LOG("Test failed for MSI%d", i); @@ -126,6 +132,9 @@ TEST_F(pci_ep_basic, MSIX_TEST) pci_ep_ioctl(PCITEST_SET_IRQTYPE, 2); ASSERT_EQ(0, ret) TH_LOG("Can't set MSI-X IRQ type");
+ pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0); + ASSERT_EQ(2, ret) TH_LOG("Can't get MSI-X IRQ type"); + for (i = 1; i <= 2048; i++) { pci_ep_ioctl(PCITEST_MSIX, i); EXPECT_FALSE(ret) TH_LOG("Test failed for MSI-X%d", i);
After devm_request_irq() fails with error in pci_endpoint_test_request_irq(), pci_endpoint_test_free_irq_vectors() is called assuming that all IRQs have been released.
However some requested IRQs remain unreleased, so there are still /proc/irq/* entries remaining and this results in WARN() with the following message:
remove_proc_entry: removing non-empty directory 'irq/30', leaking at least 'pci-endpoint-test.0' WARNING: CPU: 0 PID: 202 at fs/proc/generic.c:719 remove_proc_entry +0x190/0x19c
To solve this issue, set the number of remaining IRQs to test->num_irqs and release IRQs in advance by calling pci_endpoint_test_release_irq().
Cc: stable@vger.kernel.org Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands") Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- drivers/misc/pci_endpoint_test.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index a3d2caa7a6bb..9e56d200d2f0 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -259,6 +259,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test) break; }
+ test->num_irqs = i; + pci_endpoint_test_release_irq(test); + return ret; }
There are two variables that indicate the interrupt type to be used in the next test execution, global "irq_type" and test->irq_type.
The former is referenced from pci_endpoint_test_get_irq() to preserve the current type for ioctl(PCITEST_GET_IRQTYPE).
In pci_endpoint_test_request_irq(), since this global variable is referenced when an error occurs, the unintended error message is displayed.
For example, the following message shows "MSI 3" even if the current irq type becomes "MSI-X".
# pcitest -i 2 pci-endpoint-test 0000:01:00.0: Failed to request IRQ 30 for MSI 3 SET IRQ TYPE TO MSI-X: NOT OKAY
Fix this issue by using test->irq_type instead of global "irq_type".
Cc: stable@vger.kernel.org Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype") Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- drivers/misc/pci_endpoint_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 9e56d200d2f0..acf3d8dab131 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -242,7 +242,7 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test) return 0;
fail: - switch (irq_type) { + switch (test->irq_type) { case IRQ_TYPE_INTX: dev_err(dev, "Failed to request IRQ %d for Legacy\n", pci_irq_vector(pdev, i));
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 is displayed in old "pcitest" as follows:
# pcitest -i 0 SET IRQ TYPE TO LEGACY: OKAY # pcitest -I GET IRQ TYPE: MSI
And new "pcitest" in kselftest results in an error as follows:
# RUN pci_ep_basic.LEGACY_IRQ_TEST ... # pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Expected 0 (0) == ret (1) # pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Can't get Legacy IRQ type
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") Reviewed-by: Niklas Cassel cassel@kernel.org 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 acf3d8dab131..896392c428de 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -833,6 +833,7 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test, return ret; }
+ irq_type = test->irq_type; return 0; }
The global variable "irq_type" preserves the current value of ioctl(GET_IRQTYPE).
However, 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.
The global variable "irq_type" is never used in the actual test, so remove the variable and replace it with test->irq_type.
And also for the same reason, the variable "no_msi" can be removed.
Initially, test->irq_type has IRQ_TYPE_UNDEFINED, and the ioctl(GET_IRQTYPE) before calling ioctl(SET_IRQTYPE) will return an error.
Suggested-by: Niklas Cassel cassel@kernel.org Suggested-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- drivers/misc/pci_endpoint_test.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 896392c428de..326e8e467c42 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -96,14 +96,6 @@ static DEFINE_IDA(pci_endpoint_test_ida); #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ miscdev)
-static bool no_msi; -module_param(no_msi, bool, 0444); -MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); - -static int irq_type = IRQ_TYPE_MSI; -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)"); - enum pci_barno { BAR_0, BAR_1, @@ -833,7 +825,6 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test, return ret; }
- irq_type = test->irq_type; return 0; }
@@ -882,7 +873,7 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, ret = pci_endpoint_test_set_irq(test, arg); break; case PCITEST_GET_IRQTYPE: - ret = irq_type; + ret = test->irq_type; break; case PCITEST_CLEAR_IRQ: ret = pci_endpoint_test_clear_irq(test); @@ -939,15 +930,12 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, test->pdev = pdev; test->irq_type = IRQ_TYPE_UNDEFINED;
- if (no_msi) - irq_type = IRQ_TYPE_INTX; - data = (struct pci_endpoint_test_data *)ent->driver_data; if (data) { test_reg_bar = data->test_reg_bar; test->test_reg_bar = test_reg_bar; test->alignment = data->alignment; - irq_type = data->irq_type; + test->irq_type = data->irq_type; }
init_completion(&test->irq_raised); @@ -969,7 +957,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
pci_set_master(pdev);
- ret = pci_endpoint_test_alloc_irq_vectors(test, irq_type); + ret = pci_endpoint_test_alloc_irq_vectors(test, test->irq_type); if (ret) goto err_disable_irq;
On Tue, Feb 25, 2025, at 12:02, Kunihiko Hayashi wrote:
The global variable "irq_type" preserves the current value of ioctl(GET_IRQTYPE).
However, 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.
The global variable "irq_type" is never used in the actual test, so remove the variable and replace it with test->irq_type.
And also for the same reason, the variable "no_msi" can be removed.
Initially, test->irq_type has IRQ_TYPE_UNDEFINED, and the ioctl(GET_IRQTYPE) before calling ioctl(SET_IRQTYPE) will return an error.
Suggested-by: Niklas Cassel cassel@kernel.org Suggested-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com
Nice catch, always good to remove global variables with too generic names.
Acked-by: Arnd Bergmann arnd@arndb.de
The pci_endpoint_test_request_irq() and pci_endpoint_test_release_irq() are called repeatedly by the users through pci_endpoint_test_set_irq(). So using the managed version of IRQ functions within these functions has no effect.
Suggested-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com --- drivers/misc/pci_endpoint_test.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 326e8e467c42..7a14114488a0 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -208,10 +208,9 @@ static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) { int i; struct pci_dev *pdev = test->pdev; - struct device *dev = &pdev->dev;
for (i = 0; i < test->num_irqs; i++) - devm_free_irq(dev, pci_irq_vector(pdev, i), test); + free_irq(pci_irq_vector(pdev, i), test);
test->num_irqs = 0; } @@ -224,9 +223,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test) struct device *dev = &pdev->dev;
for (i = 0; i < test->num_irqs; i++) { - ret = devm_request_irq(dev, pci_irq_vector(pdev, i), - pci_endpoint_test_irqhandler, - IRQF_SHARED, test->name, test); + ret = request_irq(pci_irq_vector(pdev, i), + pci_endpoint_test_irqhandler, IRQF_SHARED, + test->name, test); if (ret) goto fail; }
On Tue, Feb 25, 2025, at 12:02, Kunihiko Hayashi wrote:
The pci_endpoint_test_request_irq() and pci_endpoint_test_release_irq() are called repeatedly by the users through pci_endpoint_test_set_irq(). So using the managed version of IRQ functions within these functions has no effect.
Suggested-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Signed-off-by: Kunihiko Hayashi hayashi.kunihiko@socionext.com
Acked-by: Arnd Bergmann arnd@arndb.de
Hello,
This series solves some issues about global "irq_type" that is used for indicating the current type for users.
In addition, avoid an unexpected warning that occur due to interrupts remaining after displaying an error caused by devm_request_irq().
Patch 1 includes adding GET_IRQTYPE test (check for failure). Patch 2-4 include fixes for stable kernels that have global "irq_type". Patch 5-6 include improvements for the latest.
Applied to misc, thank you!
Krzysztof
linux-kselftest-mirror@lists.linaro.org
-
Arnd Bergmann -
Krzysztof Wilczynski -
Kunihiko Hayashi