From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: bug/kunit: Core support for suppressing warning backtraces Date: Sat, 16 May 2026 10:56:13 +1000 Message-ID: In-Reply-To: <20260514-kunit_add_support-v11-1-b36a530a6d8f@redhat.com> References: <20260514-kunit_add_support-v11-0-b36a530a6d8f@redhat.com> <20260514-kunit_add_support-v11-1-b36a530a6d8f@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 **Overall: Good.** This is the core infrastructure patch and is well-design= ed. **`disable_trace_on_warning()` move in `lib/bug.c`:** The patch moves `disable_trace_on_warning()` to after the suppression check= and after the flag extraction block: ```c +#ifdef CONFIG_KUNIT + if (warning && kunit_is_suppressed_warning(true)) + return BUG_TRAP_TYPE_WARN; +#endif + + disable_trace_on_warning(); + if (warning && once) { ``` This is correct: suppressed warnings should not disable tracing, and import= antly the comment says the check is placed "before the once logic so suppre= ssed warnings do not consume the single-fire budget of WARN_ON_ONCE()." Goo= d design choice. However, the `disable_trace_on_warning()` move also affects non-KUnit code = paths. Previously it ran unconditionally early; now it runs after the flag = extraction. This is a minor semantic change for BUG (non-warning) traps =E2= =80=94 `disable_trace_on_warning()` now runs slightly later but still befor= e any output. Should be harmless but worth noting. **`#ifdef CONFIG_KUNIT` in `lib/bug.c`:** ```c +#ifdef CONFIG_KUNIT + if (warning && kunit_is_suppressed_warning(true)) + return BUG_TRAP_TYPE_WARN; +#endif ``` Using `#ifdef CONFIG_KUNIT` rather than `IS_ENABLED(CONFIG_KUNIT)` is sligh= tly inconsistent with the inline in `test-bug.h` which uses `IS_ENABLED`. H= owever, since `kunit_is_suppressed_warning()` isn't available without the h= eader, and `test-bug.h` provides a stub when `!CONFIG_KUNIT`, this is fine = =E2=80=94 the `#ifdef` avoids the header include cost for the `!CONFIG_KUNI= T` case. But the `#include ` is already added uncondition= ally at the top of `lib/bug.c`, so the `#ifdef` guard is redundant =E2=80= =94 you could just use the inline stub. This is a style nit, not a bug. **Race in `kunit_start_suppress_warning`:** ```c + if (kunit_has_active_suppress_warning()) { + KUNIT_FAIL(test, "Another suppression block is already active"); + return NULL; + } ``` The check-then-act between `kunit_has_active_suppress_warning()` and `list_= add_rcu()` is not atomic, but this is fine in practice because KUnit tests = run in their own kthread and nesting is explicitly disallowed =E2=80=94 two= concurrent calls for the same task can't happen. **`kunit_has_active_suppress_warning()` is exported but not declared in `te= st.h`:** The function is declared in `test.h` (line 597 in the patch). Confirmed, th= is is fine. **Lifetime management is correct:** `get_task_struct()` pins the task to pr= event ABA reuse, and `kunit_add_action_or_reset` ensures cleanup. The `kuni= t_end_suppress_warning` -> `kunit_release_action` path removes from the lis= t and `put_task_struct`. Good. **`__kunit_suppress_auto_cleanup` accessing `(*wp)->test`:** ```c +void __kunit_suppress_auto_cleanup(struct kunit_suppressed_warning **wp) +{ + if (*wp) + kunit_end_suppress_warning((*wp)->test, *wp); +} ``` This works because `w->test` is set during `kunit_start_suppress_warning` a= nd remains valid =E2=80=94 the test struct outlives the suppression handle.= Clean. **No `EXPORT_SYMBOL_GPL` for `__kunit_is_suppressed_warning_impl`:** This f= unction is not exported, which is correct since it's only called through th= e hook table indirection. The hook pointer is set in `kunit_install_hooks()= ` which runs at module init. --- Generated by Claude Code Patch Reviewer