From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: kunit: Add backtrace suppression self-tests Date: Tue, 05 May 2026 08:33:31 +1000 Message-ID: In-Reply-To: <20260504-kunit_add_support-v8-2-3e5957cdd235@redhat.com> References: <20260504-kunit_add_support-v8-0-3e5957cdd235@redhat.com> <20260504-kunit_add_support-v8-2-3e5957cdd235@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Good test coverage.** The tests exercise: - All three API forms - Direct and indirect (through helper function) warning triggers - WARN() and WARN_ON() variants - Counter accuracy (including incremental counting) - Active state transitions - Cross-kthread isolation **Observation 1: The `kunit_skip` guards for CONFIG_DEBUG_BUGVERBOSE.** ```c static void backtrace_suppression_test_warn_on_direct(struct kunit *test) { if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS)) kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS"); ``` vs. ```c static void backtrace_suppression_test_warn_on_indirect(struct kunit *test) { if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)) kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE"); ``` The direct test accepts either `CONFIG_DEBUG_BUGVERBOSE` or `CONFIG_KALLSYM= S`, while the indirect test requires only `CONFIG_DEBUG_BUGVERBOSE`. The as= ymmetry suggests that for indirect WARN_ON() calls, KALLSYMS alone is not s= ufficient =E2=80=94 presumably because without verbose bug info, the `__war= n_printk()` path isn't taken for indirect calls. This is a subtlety that co= uld use a brief comment explaining why the skip conditions differ between d= irect and indirect WARN_ON() tests. **Observation 2: `backtrace_suppression_test_multi_scope` tests sequential = suppression blocks.** ```c sw1 =3D kunit_start_suppress_warning(test); trigger_backtrace_warn_on(); WARN(1, "suppressed by sw1"); kunit_end_suppress_warning(test, sw1); sw2 =3D kunit_start_suppress_warning(test); WARN(1, "suppressed by sw2"); kunit_end_suppress_warning(test, sw2); KUNIT_EXPECT_EQ(test, kunit_suppressed_warning_count(sw1), 2); KUNIT_EXPECT_EQ(test, kunit_suppressed_warning_count(sw2), 1); ``` This accesses `sw1`'s counter after `kunit_end_suppress_warning(test, sw1)`= has been called. That calls `kunit_release_action()` which runs `kunit_sup= press_warning_remove()` and does `list_del_rcu()` + `synchronize_rcu()`. Af= ter that, `sw1` has been removed from the list but the memory is still aliv= e (it was allocated with `kunit_kzalloc`, so it persists until test teardow= n). Reading `sw1->counter` after end is therefore safe. Good =E2=80=94 but = it's worth noting this is a deliberate design property: the kunit_kzalloc'd= memory outlives the suppression lifetime, enabling post-suppression count = reads. --- Generated by Claude Code Patch Reviewer