From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: kunit: Add backtrace suppression self-tests
Date: Sat, 16 May 2026 10:56:13 +1000
Message-ID: <review-patch2-20260514-kunit_add_support-v11-2-b36a530a6d8f@redhat.com>
In-Reply-To: <20260514-kunit_add_support-v11-2-b36a530a6d8f@redhat.com>
References: <20260514-kunit_add_support-v11-0-b36a530a6d8f@redhat.com> <20260514-kunit_add_support-v11-2-b36a530a6d8f@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Overall: Good test coverage.** Tests the scoped API, direct API, incremental counting, active state transitions, multi-scope with independent handles, and cross-kthread isolation.

**`backtrace_suppression_test_warn_on_direct` has an asymmetric skip condition:**

```c
+	if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS))
+		kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS");
```

While `backtrace_suppression_test_warn_on_indirect` requires only:

```c
+	if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE))
+		kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE");
```

The difference is intentional: `WARN_ON()` inlined directly can use `KALLSYMS` to resolve the caller, while when called through `noinline` it cannot. This is subtly correct but a brief comment explaining the asymmetry would help future readers.

**`backtrace_suppression_test_cross_kthread` is well-structured:**

```c
+	kunit_warning_suppress(test) {
+		task = kthread_run(cross_kthread_fn, &data, "kunit-cross-test");
+		KUNIT_ASSERT_FALSE(test, IS_ERR(task));
+		wait_for_completion(&data.done);
+		kthread_stop(task);
+	}
+	KUNIT_EXPECT_FALSE(test, data.was_active);
```

The v10 changelog mentions fixing the kthread loop to prevent use-after-free. The test now uses `kthread_should_stop()` + `schedule()` loop in the kthread function and `kthread_stop()` in the test, which is correct.

**`backtrace_suppression_test_multi_scope` tests sequential (not nested) usage:**

```c
+	sw1 = kunit_start_suppress_warning(test);
+	trigger_backtrace_warn_on();
+	WARN(1, "suppressed by sw1");
+	kunit_end_suppress_warning(test, sw1);
+
+	sw2 = kunit_start_suppress_warning(test);
+	WARN(1, "suppressed by sw2");
+	kunit_end_suppress_warning(test, sw2);
```

This validates sequential independent suppression blocks and per-block counts. Good. The count check uses the handles after `end`, which works because the memory is managed by `kunit_kzalloc` (freed at test exit, not at end-of-suppression).

---
Generated by Claude Code Patch Reviewer