linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] genirq: Add kunit tests for depth counts
@ 2025-05-22 21:08 Brian Norris
  2025-06-13 13:34 ` [tip: irq/core] " tip-bot2 for Brian Norris
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Brian Norris @ 2025-05-22 21:08 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Tsai Sung-Fu, Douglas Anderson, linux-kernel, Brian Norris

There have been a few bugs and/or misunderstandings about the reference
counting, and startup/shutdown behaviors in the IRQ core and related CPU
hotplug code. These 4 test cases try to capture a few interesting cases.

* irq_disable_depth_test: basic request/disable/enable sequence

* irq_free_disabled_test: request/disable/free/re-request sequence -
  this catches errors on previous revisions of my work

* irq_cpuhotplug_test: exercises managed-affinity IRQ + CPU hotplug.
  This captures a problematic test case that I've fixed.
  This test requires CONFIG_SMP and a hotpluggable CPU#1.

* irq_shutdown_depth_test: exercises similar behavior from
  irq_cpuhotplug_test, but directly using irq_*() APIs instead of going
  through CPU hotplug. This still requires CONFIG_SMP, because
  managed-affinity is stubbed out (and not all APIs are even present)
  without it.

Note the use of 'imply SMP': ARCH=um doesn't support SMP, and kunit is
often exercised there. Thus, 'imply' will force SMP on where possible
(such as ARCH=x86_64), but leave it off where it's not.

Behavior on various SMP and ARCH configurations:

  $ tools/testing/kunit/kunit.py run 'irq_test_cases*' --arch x86_64 --qemu_args '-smp 2'
  [...]
  [11:12:24] Testing complete. Ran 4 tests: passed: 4

  $ tools/testing/kunit/kunit.py run 'irq_test_cases*' --arch x86_64
  [...]
  [11:13:27] [SKIPPED] irq_cpuhotplug_test
  [11:13:27] ================= [PASSED] irq_test_cases ==================
  [11:13:27] ============================================================
  [11:13:27] Testing complete. Ran 4 tests: passed: 3, skipped: 1

  # default: ARCH=um
  $ tools/testing/kunit/kunit.py run 'irq_test_cases*'
  [11:14:26] [SKIPPED] irq_shutdown_depth_test
  [11:14:26] [SKIPPED] irq_cpuhotplug_test
  [11:14:26] ================= [PASSED] irq_test_cases ==================
  [11:14:26] ============================================================
  [11:14:26] Testing complete. Ran 4 tests: passed: 2, skipped: 2

Without commit 788019eb559f ("genirq: Retain disable depth for managed
interrupts across CPU hotplug"), we fail as follows:

  [11:18:55] =============== irq_test_cases (4 subtests) ================
  [11:18:55] [PASSED] irq_disable_depth_test
  [11:18:55] [PASSED] irq_free_disabled_test
  [11:18:55]     # irq_shutdown_depth_test: EXPECTATION FAILED at kernel/irq/irq_test.c:147
  [11:18:55]     Expected desc->depth == 1, but
  [11:18:55]         desc->depth == 0 (0x0)
  [11:18:55] ------------[ cut here ]------------
  [11:18:55] Unbalanced enable for IRQ 26
  [11:18:55] WARNING: CPU: 1 PID: 36 at kernel/irq/manage.c:792 __enable_irq+0x36/0x60
  ...
  [11:18:55] [FAILED] irq_shutdown_depth_test
  [11:18:55]  #1
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:202
  [11:18:55]     Expected irqd_is_activated(data) to be false, but is true
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:203
  [11:18:55]     Expected irqd_is_started(data) to be false, but is true
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:204
  [11:18:55]     Expected desc->depth == 1, but
  [11:18:55]         desc->depth == 0 (0x0)
  [11:18:55] ------------[ cut here ]------------
  [11:18:55] Unbalanced enable for IRQ 27
  [11:18:55] WARNING: CPU: 0 PID: 38 at kernel/irq/manage.c:792 __enable_irq+0x36/0x60
  ...
  [11:18:55] [FAILED] irq_cpuhotplug_test
  [11:18:55]     # module: irq_test
  [11:18:55] # irq_test_cases: pass:2 fail:2 skip:0 total:4
  [11:18:55] # Totals: pass:2 fail:2 skip:0 total:4
  [11:18:55] ================= [FAILED] irq_test_cases ==================
  [11:18:55] ============================================================
  [11:18:55] Testing complete. Ran 4 tests: passed: 2, failed: 2

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v4:
 * depend on KUNIT=y: CONFIG_IRQ_KUNIT_TEST=y is incompatible with
   CONFIG_KUNIT=m -- same problem as in commit 35c57fc3f8ea ("kunit:
   building kunit as a module breaks allmodconfig")

Changes in v3:
 * send as standalone patch, since dependent patch was merged
 * make IRQ_KUNIT_TEST bool; we depend on a few internal functions that
   are not exported to modules. Reported:
   https://lore.kernel.org/oe-kbuild-all/202505152136.y04AHovS-lkp@intel.com/

Changes in v2:
 * add request_irq()/disable_irq()/free_irq()/request_irq() test
   sequence
 * clean up more resources in tests
 * move tests to patch 2 (i.e., after bugs are fixed and tests pass)
 * adapt to irq_startup_managed() (new API)

 kernel/irq/Kconfig    |  11 ++
 kernel/irq/Makefile   |   1 +
 kernel/irq/irq_test.c | 229 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100644 kernel/irq/irq_test.c

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 3f02a0e45254..1da5e9d9da71 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -144,6 +144,17 @@ config GENERIC_IRQ_DEBUGFS
 config GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD
 	bool
 
+config IRQ_KUNIT_TEST
+	bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y
+	default KUNIT_ALL_TESTS
+	imply SMP
+	help
+	  This option enables KUnit tests for the IRQ subsystem API. These are
+	  only for development and testing, not for regular kernel use cases.
+
+	  If unsure, say N.
+
 endmenu
 
 config GENERIC_IRQ_MULTI_HANDLER
diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
index c0f44c06d69d..6ab3a4055667 100644
--- a/kernel/irq/Makefile
+++ b/kernel/irq/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_GENERIC_IRQ_IPI_MUX) += ipi-mux.o
 obj-$(CONFIG_SMP) += affinity.o
 obj-$(CONFIG_GENERIC_IRQ_DEBUGFS) += debugfs.o
 obj-$(CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR) += matrix.o
+obj-$(CONFIG_IRQ_KUNIT_TEST) += irq_test.o
diff --git a/kernel/irq/irq_test.c b/kernel/irq/irq_test.c
new file mode 100644
index 000000000000..5161b56a12f9
--- /dev/null
+++ b/kernel/irq/irq_test.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: LGPL-2.1+
+
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/irqdomain.h>
+#include <linux/nodemask.h>
+#include <kunit/test.h>
+
+#include "internals.h"
+
+static irqreturn_t noop_handler(int irq, void *data)
+{
+	return IRQ_HANDLED;
+}
+
+static void noop(struct irq_data *data) { }
+static unsigned int noop_ret(struct irq_data *data) { return 0; }
+
+static int noop_affinity(struct irq_data *data, const struct cpumask *dest,
+			 bool force)
+{
+	irq_data_update_effective_affinity(data, dest);
+
+	return 0;
+}
+
+static struct irq_chip fake_irq_chip = {
+	.name           = "fake",
+	.irq_startup    = noop_ret,
+	.irq_shutdown   = noop,
+	.irq_enable     = noop,
+	.irq_disable    = noop,
+	.irq_ack        = noop,
+	.irq_mask       = noop,
+	.irq_unmask     = noop,
+	.irq_set_affinity = noop_affinity,
+	.flags          = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static void irq_disable_depth_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	int virq, ret;
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, NULL);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_free_disabled_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	int virq, ret;
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, NULL);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	free_irq(virq, NULL);
+	KUNIT_EXPECT_GE(test, desc->depth, 1);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_shutdown_depth_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	int virq, ret;
+	struct irq_affinity_desc affinity = {
+		.is_managed = 1,
+		.mask = CPU_MASK_ALL,
+	};
+
+	if (!IS_ENABLED(CONFIG_SMP))
+		kunit_skip(test, "requires CONFIG_SMP for managed shutdown");
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, &affinity);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	data = irq_desc_get_irq_data(desc);
+	KUNIT_ASSERT_PTR_NE(test, data, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_TRUE(test, irqd_affinity_is_managed(data));
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	irq_shutdown_and_deactivate(desc);
+
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+
+	KUNIT_EXPECT_EQ(test, irq_activate(desc), 0);
+#ifdef CONFIG_SMP
+	irq_startup_managed(desc);
+#endif
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_cpuhotplug_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	int virq, ret;
+	struct irq_affinity_desc affinity = {
+		.is_managed = 1,
+	};
+
+	if (!IS_ENABLED(CONFIG_SMP))
+		kunit_skip(test, "requires CONFIG_SMP for CPU hotplug");
+	if (!get_cpu_device(1))
+		kunit_skip(test, "requires more than 1 CPU for CPU hotplug");
+	if (!cpu_is_hotpluggable(1))
+		kunit_skip(test, "CPU 1 must be hotpluggable");
+
+	cpumask_copy(&affinity.mask, cpumask_of(1));
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, &affinity);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &fake_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	data = irq_desc_get_irq_data(desc);
+	KUNIT_ASSERT_PTR_NE(test, data, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_TRUE(test, irqd_affinity_is_managed(data));
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	KUNIT_EXPECT_EQ(test, remove_cpu(1), 0);
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+	KUNIT_EXPECT_GE(test, desc->depth, 1);
+	KUNIT_EXPECT_EQ(test, add_cpu(1), 0);
+
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static struct kunit_case irq_test_cases[] = {
+	KUNIT_CASE(irq_disable_depth_test),
+	KUNIT_CASE(irq_free_disabled_test),
+	KUNIT_CASE(irq_shutdown_depth_test),
+	KUNIT_CASE(irq_cpuhotplug_test),
+	{}
+};
+
+static struct kunit_suite irq_test_suite = {
+	.name = "irq_test_cases",
+	.test_cases = irq_test_cases,
+};
+
+kunit_test_suite(irq_test_suite);
+MODULE_DESCRIPTION("IRQ unit test suite");
+MODULE_LICENSE("GPL");
-- 
2.49.0.1151.ga128411c76-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [tip: irq/core] genirq: Add kunit tests for depth counts
  2025-05-22 21:08 [PATCH v4] genirq: Add kunit tests for depth counts Brian Norris
@ 2025-06-13 13:34 ` tip-bot2 for Brian Norris
  2025-08-05 17:45 ` [PATCH v4] " Guenter Roeck
  2025-08-10 19:37 ` [PATCH v4] genirq: Add kunit tests for depth counts Guenter Roeck
  2 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Brian Norris @ 2025-06-13 13:34 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: Brian Norris, Thomas Gleixner, x86, linux-kernel, maz

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     66067c3c8a1ee097e9c30e8bbd643b12ba54a6b0
Gitweb:        https://git.kernel.org/tip/66067c3c8a1ee097e9c30e8bbd643b12ba54a6b0
Author:        Brian Norris <briannorris@chromium.org>
AuthorDate:    Thu, 22 May 2025 14:08:01 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 13 Jun 2025 15:24:44 +02:00

genirq: Add kunit tests for depth counts

There have been a few bugs and/or misunderstandings about the reference
counting, and startup/shutdown behaviors in the IRQ core and related CPU
hotplug code. These 4 test cases try to capture a few interesting cases.

 * irq_disable_depth_test: basic request/disable/enable sequence

 * irq_free_disabled_test: request/disable/free/re-request sequence -
   this catches errors on previous revisions of my work

 * irq_cpuhotplug_test: exercises managed-affinity IRQ + CPU hotplug.
   This captures a problematic test case which was fixed recently.
   This test requires CONFIG_SMP and a hotpluggable CPU#1.

 * irq_shutdown_depth_test: exercises similar behavior from
   irq_cpuhotplug_test, but directly using irq_*() APIs instead of going
   through CPU hotplug. This still requires CONFIG_SMP, because
   managed-affinity is stubbed out (and not all APIs are even present)
   without it.

Note the use of 'imply SMP': ARCH=um doesn't support SMP, and kunit is
often exercised there. Thus, 'imply' will force SMP on where possible
(such as ARCH=x86_64), but leave it off where it's not.

Behavior on various SMP and ARCH configurations:

  $ tools/testing/kunit/kunit.py run 'irq_test_cases*' --arch x86_64 --qemu_args '-smp 2'
  [...]
  [11:12:24] Testing complete. Ran 4 tests: passed: 4

  $ tools/testing/kunit/kunit.py run 'irq_test_cases*' --arch x86_64
  [...]
  [11:13:27] [SKIPPED] irq_cpuhotplug_test
  [11:13:27] ================= [PASSED] irq_test_cases ==================
  [11:13:27] ============================================================
  [11:13:27] Testing complete. Ran 4 tests: passed: 3, skipped: 1

  # default: ARCH=um
  $ tools/testing/kunit/kunit.py run 'irq_test_cases*'
  [11:14:26] [SKIPPED] irq_shutdown_depth_test
  [11:14:26] [SKIPPED] irq_cpuhotplug_test
  [11:14:26] ================= [PASSED] irq_test_cases ==================
  [11:14:26] ============================================================
  [11:14:26] Testing complete. Ran 4 tests: passed: 2, skipped: 2

Without commit 788019eb559f ("genirq: Retain disable depth for managed
interrupts across CPU hotplug"), this fails as follows:

  [11:18:55] =============== irq_test_cases (4 subtests) ================
  [11:18:55] [PASSED] irq_disable_depth_test
  [11:18:55] [PASSED] irq_free_disabled_test
  [11:18:55]     # irq_shutdown_depth_test: EXPECTATION FAILED at kernel/irq/irq_test.c:147
  [11:18:55]     Expected desc->depth == 1, but
  [11:18:55]         desc->depth == 0 (0x0)
  [11:18:55] ------------[ cut here ]------------
  [11:18:55] Unbalanced enable for IRQ 26
  [11:18:55] WARNING: CPU: 1 PID: 36 at kernel/irq/manage.c:792 __enable_irq+0x36/0x60
  ...
  [11:18:55] [FAILED] irq_shutdown_depth_test
  [11:18:55]  #1
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:202
  [11:18:55]     Expected irqd_is_activated(data) to be false, but is true
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:203
  [11:18:55]     Expected irqd_is_started(data) to be false, but is true
  [11:18:55]     # irq_cpuhotplug_test: EXPECTATION FAILED at kernel/irq/irq_test.c:204
  [11:18:55]     Expected desc->depth == 1, but
  [11:18:55]         desc->depth == 0 (0x0)
  [11:18:55] ------------[ cut here ]------------
  [11:18:55] Unbalanced enable for IRQ 27
  [11:18:55] WARNING: CPU: 0 PID: 38 at kernel/irq/manage.c:792 __enable_irq+0x36/0x60
  ...
  [11:18:55] [FAILED] irq_cpuhotplug_test
  [11:18:55]     # module: irq_test
  [11:18:55] # irq_test_cases: pass:2 fail:2 skip:0 total:4
  [11:18:55] # Totals: pass:2 fail:2 skip:0 total:4
  [11:18:55] ================= [FAILED] irq_test_cases ==================
  [11:18:55] ============================================================
  [11:18:55] Testing complete. Ran 4 tests: passed: 2, failed: 2

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20250522210837.4135244-1-briannorris@chromium.org

---
 kernel/irq/Kconfig    |  11 ++-
 kernel/irq/Makefile   |   1 +-
 kernel/irq/irq_test.c | 229 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 241 insertions(+)
 create mode 100644 kernel/irq/irq_test.c

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 3f02a0e..1da5e9d 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -144,6 +144,17 @@ config GENERIC_IRQ_DEBUGFS
 config GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD
 	bool
 
+config IRQ_KUNIT_TEST
+	bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y
+	default KUNIT_ALL_TESTS
+	imply SMP
+	help
+	  This option enables KUnit tests for the IRQ subsystem API. These are
+	  only for development and testing, not for regular kernel use cases.
+
+	  If unsure, say N.
+
 endmenu
 
 config GENERIC_IRQ_MULTI_HANDLER
diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
index c0f44c0..6ab3a40 100644
--- a/kernel/irq/Makefile
+++ b/kernel/irq/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_GENERIC_IRQ_IPI_MUX) += ipi-mux.o
 obj-$(CONFIG_SMP) += affinity.o
 obj-$(CONFIG_GENERIC_IRQ_DEBUGFS) += debugfs.o
 obj-$(CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR) += matrix.o
+obj-$(CONFIG_IRQ_KUNIT_TEST) += irq_test.o
diff --git a/kernel/irq/irq_test.c b/kernel/irq/irq_test.c
new file mode 100644
index 0000000..5161b56
--- /dev/null
+++ b/kernel/irq/irq_test.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: LGPL-2.1+
+
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/irqdomain.h>
+#include <linux/nodemask.h>
+#include <kunit/test.h>
+
+#include "internals.h"
+
+static irqreturn_t noop_handler(int irq, void *data)
+{
+	return IRQ_HANDLED;
+}
+
+static void noop(struct irq_data *data) { }
+static unsigned int noop_ret(struct irq_data *data) { return 0; }
+
+static int noop_affinity(struct irq_data *data, const struct cpumask *dest,
+			 bool force)
+{
+	irq_data_update_effective_affinity(data, dest);
+
+	return 0;
+}
+
+static struct irq_chip fake_irq_chip = {
+	.name           = "fake",
+	.irq_startup    = noop_ret,
+	.irq_shutdown   = noop,
+	.irq_enable     = noop,
+	.irq_disable    = noop,
+	.irq_ack        = noop,
+	.irq_mask       = noop,
+	.irq_unmask     = noop,
+	.irq_set_affinity = noop_affinity,
+	.flags          = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static void irq_disable_depth_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	int virq, ret;
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, NULL);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_free_disabled_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	int virq, ret;
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, NULL);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	free_irq(virq, NULL);
+	KUNIT_EXPECT_GE(test, desc->depth, 1);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_shutdown_depth_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	int virq, ret;
+	struct irq_affinity_desc affinity = {
+		.is_managed = 1,
+		.mask = CPU_MASK_ALL,
+	};
+
+	if (!IS_ENABLED(CONFIG_SMP))
+		kunit_skip(test, "requires CONFIG_SMP for managed shutdown");
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, &affinity);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	data = irq_desc_get_irq_data(desc);
+	KUNIT_ASSERT_PTR_NE(test, data, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_TRUE(test, irqd_affinity_is_managed(data));
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	irq_shutdown_and_deactivate(desc);
+
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+
+	KUNIT_EXPECT_EQ(test, irq_activate(desc), 0);
+#ifdef CONFIG_SMP
+	irq_startup_managed(desc);
+#endif
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static void irq_cpuhotplug_test(struct kunit *test)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	int virq, ret;
+	struct irq_affinity_desc affinity = {
+		.is_managed = 1,
+	};
+
+	if (!IS_ENABLED(CONFIG_SMP))
+		kunit_skip(test, "requires CONFIG_SMP for CPU hotplug");
+	if (!get_cpu_device(1))
+		kunit_skip(test, "requires more than 1 CPU for CPU hotplug");
+	if (!cpu_is_hotpluggable(1))
+		kunit_skip(test, "CPU 1 must be hotpluggable");
+
+	cpumask_copy(&affinity.mask, cpumask_of(1));
+
+	virq = irq_domain_alloc_descs(-1, 1, 0, NUMA_NO_NODE, &affinity);
+	KUNIT_ASSERT_GE(test, virq, 0);
+
+	irq_set_chip_and_handler(virq, &fake_irq_chip, handle_simple_irq);
+
+	desc = irq_to_desc(virq);
+	KUNIT_ASSERT_PTR_NE(test, desc, NULL);
+
+	data = irq_desc_get_irq_data(desc);
+	KUNIT_ASSERT_PTR_NE(test, data, NULL);
+
+	ret = request_irq(virq, noop_handler, 0, "test_irq", NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_TRUE(test, irqd_affinity_is_managed(data));
+
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	disable_irq(virq);
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	KUNIT_EXPECT_EQ(test, remove_cpu(1), 0);
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+	KUNIT_EXPECT_GE(test, desc->depth, 1);
+	KUNIT_EXPECT_EQ(test, add_cpu(1), 0);
+
+	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
+	KUNIT_EXPECT_EQ(test, desc->depth, 1);
+
+	enable_irq(virq);
+	KUNIT_EXPECT_TRUE(test, irqd_is_activated(data));
+	KUNIT_EXPECT_TRUE(test, irqd_is_started(data));
+	KUNIT_EXPECT_EQ(test, desc->depth, 0);
+
+	free_irq(virq, NULL);
+}
+
+static struct kunit_case irq_test_cases[] = {
+	KUNIT_CASE(irq_disable_depth_test),
+	KUNIT_CASE(irq_free_disabled_test),
+	KUNIT_CASE(irq_shutdown_depth_test),
+	KUNIT_CASE(irq_cpuhotplug_test),
+	{}
+};
+
+static struct kunit_suite irq_test_suite = {
+	.name = "irq_test_cases",
+	.test_cases = irq_test_cases,
+};
+
+kunit_test_suite(irq_test_suite);
+MODULE_DESCRIPTION("IRQ unit test suite");
+MODULE_LICENSE("GPL");

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] genirq: Add kunit tests for depth counts
  2025-05-22 21:08 [PATCH v4] genirq: Add kunit tests for depth counts Brian Norris
  2025-06-13 13:34 ` [tip: irq/core] " tip-bot2 for Brian Norris
@ 2025-08-05 17:45 ` Guenter Roeck
  2025-08-05 18:32   ` [PATCH] genirq/test: Resolve irq lock inversion warnings Brian Norris
  2025-08-10 19:37 ` [PATCH v4] genirq: Add kunit tests for depth counts Guenter Roeck
  2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2025-08-05 17:45 UTC (permalink / raw)
  To: Brian Norris
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

Hi Brian,

On Thu, May 22, 2025 at 02:08:01PM -0700, Brian Norris wrote:
> There have been a few bugs and/or misunderstandings about the reference
> counting, and startup/shutdown behaviors in the IRQ core and related CPU
> hotplug code. These 4 test cases try to capture a few interesting cases.
> 
> * irq_disable_depth_test: basic request/disable/enable sequence
> 
> * irq_free_disabled_test: request/disable/free/re-request sequence -
>   this catches errors on previous revisions of my work
> 
> * irq_cpuhotplug_test: exercises managed-affinity IRQ + CPU hotplug.
>   This captures a problematic test case that I've fixed.
>   This test requires CONFIG_SMP and a hotpluggable CPU#1.
> 
> * irq_shutdown_depth_test: exercises similar behavior from
>   irq_cpuhotplug_test, but directly using irq_*() APIs instead of going
>   through CPU hotplug. This still requires CONFIG_SMP, because
>   managed-affinity is stubbed out (and not all APIs are even present)
>   without it.
> 
This test triggers warning tracebacks for me.

[    5.766715]     ok 2 irq_free_disabled_test
[    5.769030]
[    5.769106] ========================================================
[    5.769159] WARNING: possible irq lock inversion dependency detected
[    5.769355] 6.16.0-11743-g6bcdbd62bd56 #1 Tainted: G                 N
[    5.769413] --------------------------------------------------------
[    5.769465] kunit_try_catch/122 just changed the state of lock:
[    5.769532] ffffffffb81ace18 (irq_resend_lock){+...}-{2:2}, at: clear_irq_resend+0x14/0x70
[    5.769899] but this lock was taken by another, HARDIRQ-safe lock in the past:
[    5.769967]  (&irq_desc_lock_class){-.-.}-{2:2}
[    5.769989]
[    5.769989]
[    5.769989] and interrupts could create inverse lock ordering between them.
...
[    5.776956]  ret_from_fork_asm+0x1a/0x30
[    5.776983]  </TASK>
[    5.778916]     # irq_shutdown_depth_test: pass:1 fail:0 skip:0 total:1
[    5.778953]     ok 3 irq_shutdown_depth_test

Is this on purpose ?

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] genirq/test: Resolve irq lock inversion warnings
  2025-08-05 17:45 ` [PATCH v4] " Guenter Roeck
@ 2025-08-05 18:32   ` Brian Norris
  2025-08-05 19:54     ` Guenter Roeck
  2025-08-06  8:33     ` [tip: irq/urgent] " tip-bot2 for Brian Norris
  0 siblings, 2 replies; 10+ messages in thread
From: Brian Norris @ 2025-08-05 18:32 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

irq_shutdown_and_deactivate() is normally called with the descriptor
lock held, and interrupts disabled. Nested a few levels down, it grabs
the global irq_resend_lock. Lockdep rightfully complains [1].

Grab the descriptor lock, and disable interrupts, to resolve the
complaint.

Tested with:

  tools/testing/kunit/kunit.py run 'irq_test_cases*' \
      --arch x86_64 --qemu_args '-smp 2' \
      --kconfig_add CONFIG_DEBUG_KERNEL=y \
      --kconfig_add CONFIG_PROVE_LOCKING=y \
      --raw_output=all

[1]
========================================================
WARNING: possible irq lock inversion dependency detected
6.16.0-11743-g6bcdbd62bd56 #2 Tainted: G                 N
--------------------------------------------------------
kunit_try_catch/40 just changed the state of lock:
ffffffff898b1538 (irq_resend_lock){+...}-{2:2}, at: clear_irq_resend+0x14/0x70
but this lock was taken by another, HARDIRQ-safe lock in the past:
 (&irq_desc_lock_class){-.-.}-{2:2}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(irq_resend_lock);
                               local_irq_disable();
                               lock(&irq_desc_lock_class);
                               lock(irq_resend_lock);
  <Interrupt>
    lock(&irq_desc_lock_class);

[...]

 ... key      at: [<ffffffff898b1538>] irq_resend_lock+0x18/0x60
 ... acquired at:
   __lock_acquire+0x82b/0x2620
   lock_acquire+0xc7/0x2c0
   _raw_spin_lock+0x2b/0x40
   clear_irq_resend+0x14/0x70
   irq_shutdown_and_deactivate+0x29/0x80
   irq_shutdown_depth_test+0x1ce/0x600
   kunit_try_run_case+0x90/0x120
   kunit_generic_run_threadfn_adapter+0x1c/0x40
   kthread+0xf3/0x200
   ret_from_fork+0x140/0x1b0
   ret_from_fork_asm+0x1a/0x30

[    5.766715]     ok 2 irq_free_disabled_test
[    5.769030]
[    5.769106] ========================================================
[    5.769159] WARNING: possible irq lock inversion dependency detected
[    5.769355] 6.16.0-11743-g6bcdbd62bd56 #1 Tainted: G                 N
[    5.769413] --------------------------------------------------------
[    5.769465] kunit_try_catch/122 just changed the state of lock:
[    5.769532] ffffffffb81ace18 (irq_resend_lock){+...}-{2:2}, at: clear_irq_resend+0x14/0x70
[    5.769899] but this lock was taken by another, HARDIRQ-safe lock in the past:
[    5.769967]  (&irq_desc_lock_class){-.-.}-{2:2}
[    5.769989]
[    5.769989]
[    5.769989] and interrupts could create inverse lock ordering between them.
...
[    5.776956]  ret_from_fork_asm+0x1a/0x30
[    5.776983]  </TASK>
[    5.778916]     # irq_shutdown_depth_test: pass:1 fail:0 skip:0 total:1
[    5.778953]     ok 3 irq_shutdown_depth_test

Fixes: 66067c3c8a1e ("genirq: Add kunit tests for depth counts")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/lkml/31a761e4-8f81-40cf-aaf5-d220ba11911c@roeck-us.net/
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

On Tue, Aug 05, 2025 at 10:45:33AM -0700, Guenter Roeck wrote:
> Hi Brian,
> 
> On Thu, May 22, 2025 at 02:08:01PM -0700, Brian Norris wrote:
> > * irq_shutdown_depth_test: exercises similar behavior from
> >   irq_cpuhotplug_test, but directly using irq_*() APIs instead of going
> >   through CPU hotplug. This still requires CONFIG_SMP, because
> >   managed-affinity is stubbed out (and not all APIs are even present)
> >   without it.
> > 
> This test triggers warning tracebacks for me.
[...]
> Is this on purpose ?

No. I think it's an artifact of trying to imitate CPU hotplug, but doing
so insufficiently. I believe the surrounding patch fixes things.

Let me know if it helps. Thanks for the report.

 kernel/irq/irq_test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/irq_test.c b/kernel/irq/irq_test.c
index 5161b56a12f9..a75abebed7f2 100644
--- a/kernel/irq/irq_test.c
+++ b/kernel/irq/irq_test.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: LGPL-2.1+
 
+#include <linux/cleanup.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/interrupt.h>
@@ -134,7 +135,8 @@ static void irq_shutdown_depth_test(struct kunit *test)
 	disable_irq(virq);
 	KUNIT_EXPECT_EQ(test, desc->depth, 1);
 
-	irq_shutdown_and_deactivate(desc);
+	scoped_guard(raw_spinlock_irqsave, &desc->lock)
+		irq_shutdown_and_deactivate(desc);
 
 	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
 	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));
-- 
2.50.1.565.gc32cd1483b-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq/test: Resolve irq lock inversion warnings
  2025-08-05 18:32   ` [PATCH] genirq/test: Resolve irq lock inversion warnings Brian Norris
@ 2025-08-05 19:54     ` Guenter Roeck
  2025-08-06  8:33     ` [tip: irq/urgent] " tip-bot2 for Brian Norris
  1 sibling, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2025-08-05 19:54 UTC (permalink / raw)
  To: Brian Norris
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

On 8/5/25 11:32, Brian Norris wrote:
> irq_shutdown_and_deactivate() is normally called with the descriptor
> lock held, and interrupts disabled. Nested a few levels down, it grabs
> the global irq_resend_lock. Lockdep rightfully complains [1].
> 
> Grab the descriptor lock, and disable interrupts, to resolve the
> complaint.
> 
> Tested with:
> 
>    tools/testing/kunit/kunit.py run 'irq_test_cases*' \
>        --arch x86_64 --qemu_args '-smp 2' \
>        --kconfig_add CONFIG_DEBUG_KERNEL=y \
>        --kconfig_add CONFIG_PROVE_LOCKING=y \
>        --raw_output=all
> 
> [1]
> ========================================================
> WARNING: possible irq lock inversion dependency detected
> 6.16.0-11743-g6bcdbd62bd56 #2 Tainted: G                 N
> --------------------------------------------------------
> kunit_try_catch/40 just changed the state of lock:
> ffffffff898b1538 (irq_resend_lock){+...}-{2:2}, at: clear_irq_resend+0x14/0x70
> but this lock was taken by another, HARDIRQ-safe lock in the past:
>   (&irq_desc_lock_class){-.-.}-{2:2}
> 
> and interrupts could create inverse lock ordering between them.
> 
> other info that might help us debug this:
>   Possible interrupt unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(irq_resend_lock);
>                                 local_irq_disable();
>                                 lock(&irq_desc_lock_class);
>                                 lock(irq_resend_lock);
>    <Interrupt>
>      lock(&irq_desc_lock_class);
> 
> [...]
> 
>   ... key      at: [<ffffffff898b1538>] irq_resend_lock+0x18/0x60
>   ... acquired at:
>     __lock_acquire+0x82b/0x2620
>     lock_acquire+0xc7/0x2c0
>     _raw_spin_lock+0x2b/0x40
>     clear_irq_resend+0x14/0x70
>     irq_shutdown_and_deactivate+0x29/0x80
>     irq_shutdown_depth_test+0x1ce/0x600
>     kunit_try_run_case+0x90/0x120
>     kunit_generic_run_threadfn_adapter+0x1c/0x40
>     kthread+0xf3/0x200
>     ret_from_fork+0x140/0x1b0
>     ret_from_fork_asm+0x1a/0x30
> 
> [    5.766715]     ok 2 irq_free_disabled_test
> [    5.769030]
> [    5.769106] ========================================================
> [    5.769159] WARNING: possible irq lock inversion dependency detected
> [    5.769355] 6.16.0-11743-g6bcdbd62bd56 #1 Tainted: G                 N
> [    5.769413] --------------------------------------------------------
> [    5.769465] kunit_try_catch/122 just changed the state of lock:
> [    5.769532] ffffffffb81ace18 (irq_resend_lock){+...}-{2:2}, at: clear_irq_resend+0x14/0x70
> [    5.769899] but this lock was taken by another, HARDIRQ-safe lock in the past:
> [    5.769967]  (&irq_desc_lock_class){-.-.}-{2:2}
> [    5.769989]
> [    5.769989]
> [    5.769989] and interrupts could create inverse lock ordering between them.
> ...
> [    5.776956]  ret_from_fork_asm+0x1a/0x30
> [    5.776983]  </TASK>
> [    5.778916]     # irq_shutdown_depth_test: pass:1 fail:0 skip:0 total:1
> [    5.778953]     ok 3 irq_shutdown_depth_test
> 
> Fixes: 66067c3c8a1e ("genirq: Add kunit tests for depth counts")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/lkml/31a761e4-8f81-40cf-aaf5-d220ba11911c@roeck-us.net/
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks for the quick turnaround!

Guenter


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: irq/urgent] genirq/test: Resolve irq lock inversion warnings
  2025-08-05 18:32   ` [PATCH] genirq/test: Resolve irq lock inversion warnings Brian Norris
  2025-08-05 19:54     ` Guenter Roeck
@ 2025-08-06  8:33     ` tip-bot2 for Brian Norris
  1 sibling, 0 replies; 10+ messages in thread
From: tip-bot2 for Brian Norris @ 2025-08-06  8:33 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Guenter Roeck, Brian Norris, Thomas Gleixner, x86, linux-kernel,
	maz

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     5b65258229117995eb6c4bd74995e15fb5f2cfe3
Gitweb:        https://git.kernel.org/tip/5b65258229117995eb6c4bd74995e15fb5f2cfe3
Author:        Brian Norris <briannorris@chromium.org>
AuthorDate:    Tue, 05 Aug 2025 11:32:20 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 06 Aug 2025 10:29:48 +02:00

genirq/test: Resolve irq lock inversion warnings

irq_shutdown_and_deactivate() is normally called with the descriptor lock
held, and interrupts disabled. Nested a few levels down, it grabs the
global irq_resend_lock. Lockdep rightfully complains when interrupts are
not disabled:

       CPU0                    CPU1
       ----                    ----
  lock(irq_resend_lock);
                               local_irq_disable();
                               lock(&irq_desc_lock_class);
                               lock(irq_resend_lock);
  <Interrupt>
    lock(&irq_desc_lock_class);

...
   _raw_spin_lock+0x2b/0x40
   clear_irq_resend+0x14/0x70
   irq_shutdown_and_deactivate+0x29/0x80
   irq_shutdown_depth_test+0x1ce/0x600
   kunit_try_run_case+0x90/0x120

Grab the descriptor lock and disable interrupts, to resolve the
problem.

Fixes: 66067c3c8a1e ("genirq: Add kunit tests for depth counts")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/all/aJJONEIoIiTSDMqc@google.com
Closes: https://lore.kernel.org/lkml/31a761e4-8f81-40cf-aaf5-d220ba11911c@roeck-us.net/
---
 kernel/irq/irq_test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/irq_test.c b/kernel/irq/irq_test.c
index 5161b56..a75abeb 100644
--- a/kernel/irq/irq_test.c
+++ b/kernel/irq/irq_test.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: LGPL-2.1+
 
+#include <linux/cleanup.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/interrupt.h>
@@ -134,7 +135,8 @@ static void irq_shutdown_depth_test(struct kunit *test)
 	disable_irq(virq);
 	KUNIT_EXPECT_EQ(test, desc->depth, 1);
 
-	irq_shutdown_and_deactivate(desc);
+	scoped_guard(raw_spinlock_irqsave, &desc->lock)
+		irq_shutdown_and_deactivate(desc);
 
 	KUNIT_EXPECT_FALSE(test, irqd_is_activated(data));
 	KUNIT_EXPECT_FALSE(test, irqd_is_started(data));

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] genirq: Add kunit tests for depth counts
  2025-05-22 21:08 [PATCH v4] genirq: Add kunit tests for depth counts Brian Norris
  2025-06-13 13:34 ` [tip: irq/core] " tip-bot2 for Brian Norris
  2025-08-05 17:45 ` [PATCH v4] " Guenter Roeck
@ 2025-08-10 19:37 ` Guenter Roeck
  2025-08-15 17:58   ` Brian Norris
  2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2025-08-10 19:37 UTC (permalink / raw)
  To: Brian Norris
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

Hi Brian,

On Thu, May 22, 2025 at 02:08:01PM -0700, Brian Norris wrote:
> There have been a few bugs and/or misunderstandings about the reference
> counting, and startup/shutdown behaviors in the IRQ core and related CPU
> hotplug code. These 4 test cases try to capture a few interesting cases.
> 
> * irq_disable_depth_test: basic request/disable/enable sequence
> 
> * irq_free_disabled_test: request/disable/free/re-request sequence -
>   this catches errors on previous revisions of my work
> 
> * irq_cpuhotplug_test: exercises managed-affinity IRQ + CPU hotplug.
>   This captures a problematic test case that I've fixed.
>   This test requires CONFIG_SMP and a hotpluggable CPU#1.
> 
> * irq_shutdown_depth_test: exercises similar behavior from
>   irq_cpuhotplug_test, but directly using irq_*() APIs instead of going
>   through CPU hotplug. This still requires CONFIG_SMP, because
>   managed-affinity is stubbed out (and not all APIs are even present)
>   without it.
> 
> Note the use of 'imply SMP': ARCH=um doesn't support SMP, and kunit is
> often exercised there. Thus, 'imply' will force SMP on where possible
> (such as ARCH=x86_64), but leave it off where it's not.
> 
...
>  
> +config IRQ_KUNIT_TEST
> +	bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
> +	depends on KUNIT=y
> +	default KUNIT_ALL_TESTS
> +	imply SMP
> +	help
> +	  This option enables KUnit tests for the IRQ subsystem API. These are
> +	  only for development and testing, not for regular kernel use cases.
> +
> +	  If unsure, say N.
> +

The new code calls irq_domain_alloc_descs(), making it dependent
on IRQ_DOMAIN. However, specifying that dependency directly is not
possible:

 config IRQ_KUNIT_TEST
        bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
        depends on KUNIT=y
+       depends on IRQ_DOMAIN
        default KUNIT_ALL_TESTS
        imply SMP
        help

results in:

*** Default configuration is based on 'defconfig'
error: recursive dependency detected!
	symbol SMP is implied by IRQ_KUNIT_TEST
	symbol IRQ_KUNIT_TEST depends on IRQ_DOMAIN
	symbol IRQ_DOMAIN is selected by IRQ_DOMAIN_HIERARCHY
	symbol IRQ_DOMAIN_HIERARCHY is selected by GENERIC_IRQ_IPI
	symbol GENERIC_IRQ_IPI depends on SMP

This is seen with alpha configurations such as alpha:defconfig after
adding the IRQ_DOMAIN dependency.

I have no idea how to resolve this. For now I disabled IRQ_KUNIT_TEST
for my alpha test builds.

Guenter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] genirq: Add kunit tests for depth counts
  2025-08-10 19:37 ` [PATCH v4] genirq: Add kunit tests for depth counts Guenter Roeck
@ 2025-08-15 17:58   ` Brian Norris
  2025-08-16  0:24     ` Guenter Roeck
  0 siblings, 1 reply; 10+ messages in thread
From: Brian Norris @ 2025-08-15 17:58 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

On Sun, Aug 10, 2025 at 12:37:31PM -0700, Guenter Roeck wrote:
> The new code calls irq_domain_alloc_descs(), making it dependent
> on IRQ_DOMAIN. However, specifying that dependency directly is not
> possible:
> 
>  config IRQ_KUNIT_TEST
>         bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
>         depends on KUNIT=y
> +       depends on IRQ_DOMAIN
>         default KUNIT_ALL_TESTS
>         imply SMP
>         help
> 
> results in:
> 
> *** Default configuration is based on 'defconfig'
> error: recursive dependency detected!
> 	symbol SMP is implied by IRQ_KUNIT_TEST
> 	symbol IRQ_KUNIT_TEST depends on IRQ_DOMAIN
> 	symbol IRQ_DOMAIN is selected by IRQ_DOMAIN_HIERARCHY
> 	symbol IRQ_DOMAIN_HIERARCHY is selected by GENERIC_IRQ_IPI
> 	symbol GENERIC_IRQ_IPI depends on SMP
> 
> This is seen with alpha configurations such as alpha:defconfig after
> adding the IRQ_DOMAIN dependency.
> 
> I have no idea how to resolve this. For now I disabled IRQ_KUNIT_TEST
> for my alpha test builds.

How about 'select'? That's the usual way IRQ_DOMAIN is managed anyway.

It builds for me, but my distro doesn't provide an Alpha QEMU, so I
can't test it.

--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -148,6 +148,7 @@ config IRQ_KUNIT_TEST
 	bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
 	depends on KUNIT=y
 	default KUNIT_ALL_TESTS
+	select IRQ_DOMAIN
 	imply SMP
 	help
 	  This option enables KUnit tests for the IRQ subsystem API. These are

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] genirq: Add kunit tests for depth counts
  2025-08-15 17:58   ` Brian Norris
@ 2025-08-16  0:24     ` Guenter Roeck
  2025-08-16  2:23       ` Brian Norris
  0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2025-08-16  0:24 UTC (permalink / raw)
  To: Brian Norris
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

On 8/15/25 10:58, Brian Norris wrote:
> On Sun, Aug 10, 2025 at 12:37:31PM -0700, Guenter Roeck wrote:
>> The new code calls irq_domain_alloc_descs(), making it dependent
>> on IRQ_DOMAIN. However, specifying that dependency directly is not
>> possible:
>>
>>   config IRQ_KUNIT_TEST
>>          bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
>>          depends on KUNIT=y
>> +       depends on IRQ_DOMAIN
>>          default KUNIT_ALL_TESTS
>>          imply SMP
>>          help
>>
>> results in:
>>
>> *** Default configuration is based on 'defconfig'
>> error: recursive dependency detected!
>> 	symbol SMP is implied by IRQ_KUNIT_TEST
>> 	symbol IRQ_KUNIT_TEST depends on IRQ_DOMAIN
>> 	symbol IRQ_DOMAIN is selected by IRQ_DOMAIN_HIERARCHY
>> 	symbol IRQ_DOMAIN_HIERARCHY is selected by GENERIC_IRQ_IPI
>> 	symbol GENERIC_IRQ_IPI depends on SMP
>>
>> This is seen with alpha configurations such as alpha:defconfig after
>> adding the IRQ_DOMAIN dependency.
>>
>> I have no idea how to resolve this. For now I disabled IRQ_KUNIT_TEST
>> for my alpha test builds.
> 
> How about 'select'? That's the usual way IRQ_DOMAIN is managed anyway.
> 
> It builds for me, but my distro doesn't provide an Alpha QEMU, so I
> can't test it.
> 

I can try, but the irq test code fails for me all over the place, so I am
not sure if it is worth it.

 From my current upstream test results (6.17-rc1):

Build results:
	total: 162 pass: 162 fail: 0
Qemu test results:
	total: 637 pass: 637 fail: 0
Unit test results:
	pass: 640017 fail: 649

The failures are all from the irq test code. I didn't have time to analyze it.
You can find details at https://kerneltests.org/builders in the "master" column
if you have time.

Note that "imply SMP" does not make SMP mandatory. I can still disable it after
enabling IRQ_KUNIT_TEST on x86. Frankly I don't really understand what "imply"
is supposed to be useful for.

Guenter

> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -148,6 +148,7 @@ config IRQ_KUNIT_TEST
>   	bool "KUnit tests for IRQ management APIs" if !KUNIT_ALL_TESTS
>   	depends on KUNIT=y
>   	default KUNIT_ALL_TESTS
> +	select IRQ_DOMAIN
>   	imply SMP
>   	help
>   	  This option enables KUnit tests for the IRQ subsystem API. These are


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] genirq: Add kunit tests for depth counts
  2025-08-16  0:24     ` Guenter Roeck
@ 2025-08-16  2:23       ` Brian Norris
  0 siblings, 0 replies; 10+ messages in thread
From: Brian Norris @ 2025-08-16  2:23 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Thomas Gleixner, Tsai Sung-Fu, Douglas Anderson, linux-kernel

On Fri, Aug 15, 2025 at 05:24:24PM -0700, Guenter Roeck wrote:
> I can try, but the irq test code fails for me all over the place, so I am
> not sure if it is worth it.
> 
> From my current upstream test results (6.17-rc1):
> 
> Build results:
> 	total: 162 pass: 162 fail: 0
> Qemu test results:
> 	total: 637 pass: 637 fail: 0
> Unit test results:
> 	pass: 640017 fail: 649
> 
> The failures are all from the irq test code. I didn't have time to analyze it.
> You can find details at https://kerneltests.org/builders in the "master" column
> if you have time.

I can't replicate all of those easily, since I don't have tooling ready
for some of the more esoteric architectures. But many of those look like
they boil down to a single oversight: that some architectures default to
IRQ_NOREQUEST, and so I need to throw this onto the fake IRQs I set up:

	irq_settings_clr_norequest(desc);

With that, I can pass on ARCH=arm:

  tools/testing/kunit/kunit.py run 'irq_test_cases*' --arch arm \
      --qemu_args '-smp 2' --cross_compile arm-linux-gnueabi-

I'm less sure about the ARCH=parisc{,64} ones, but I think that boils
down to missing CONFIG_SPARSE_IRQ. I think I can skip tests in the
!SPARSE_IRQ case.

> Note that "imply SMP" does not make SMP mandatory. I can still disable it after
> enabling IRQ_KUNIT_TEST on x86.

Right, that's intentional. There are a few CONFIG_SMP-conditional bits
in the tests, since many users likely run on ARCH=um, which does not
have an SMP build.

> Frankly I don't really understand what "imply"
> is supposed to be useful for.

Documentation/kbuild/kconfig-language.rst calls it a "weak reverse
dependency" (i.e., a weak "select") and:

  This is useful e.g. with multiple drivers that want to indicate their
  ability to hook into a secondary subsystem while allowing the user to
  configure that subsystem out without also having to unset these drivers.

Basically, I want SMP when it's available, but I don't want to force it
when not. And it means on KUNIT_ALL_TESTS builds that otherwise didn't
make an opinionated choice for CONFIG_SMP, we get CONFIG_SMP=y. The
latter point is relevant to tools/testing/kunit/kunit.py.

I have a patch series to clean up a little while fixing the errors
you've pointed me at. I'll probably send it out next week.

Thanks,
Brian

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-08-16  2:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-22 21:08 [PATCH v4] genirq: Add kunit tests for depth counts Brian Norris
2025-06-13 13:34 ` [tip: irq/core] " tip-bot2 for Brian Norris
2025-08-05 17:45 ` [PATCH v4] " Guenter Roeck
2025-08-05 18:32   ` [PATCH] genirq/test: Resolve irq lock inversion warnings Brian Norris
2025-08-05 19:54     ` Guenter Roeck
2025-08-06  8:33     ` [tip: irq/urgent] " tip-bot2 for Brian Norris
2025-08-10 19:37 ` [PATCH v4] genirq: Add kunit tests for depth counts Guenter Roeck
2025-08-15 17:58   ` Brian Norris
2025-08-16  0:24     ` Guenter Roeck
2025-08-16  2:23       ` Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).