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 09:18:00 +1000 Message-ID: In-Reply-To: <20260515-kunit_add_support-v13-1-18ee42f96e7b@redhat.com> References: <20260515-kunit_add_support-v13-0-18ee42f96e7b@redhat.com> <20260515-kunit_add_support-v13-1-18ee42f96e7b@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 **Architecture & correctness: Good** The hook integration pattern follows the existing `fail_current_test` / `ge= t_static_stub_address` approach exactly, which is the right way to extend t= his. **`__report_bug()` changes (lib/bug.c):** The placement of the suppression check *before* the `WARN_ON_ONCE` single-f= ire logic is a critical design choice and is correct: ```c + if (warning && kunit_is_suppressed_warning(true)) + return BUG_TRAP_TYPE_WARN; + + disable_trace_on_warning(); + if (warning && once) { if (done) return BUG_TRAP_TYPE_WARN; bug->flags |=3D BUGFLAG_DONE; } ``` This ensures suppressed warnings don't consume the single-fire budget of `W= ARN_ON_ONCE()`, which is the right behavior. The movement of `disable_trace= _on_warning()` below the suppression check is also correct =E2=80=94 no nee= d to disable tracing for a suppressed warning. **`kunit_is_suppressed_warning()` inline (test-bug.h):** ```c +static inline bool kunit_is_suppressed_warning(bool count) +{ + if (!static_branch_unlikely(&kunit_running)) + return false; + + return kunit_hooks.is_suppressed_warning && + kunit_hooks.is_suppressed_warning(count); +} ``` The NULL check on `is_suppressed_warning` is belt-and-suspenders since `kun= it_install_hooks()` sets all pointers at module init, but it's harmless and= consistent with defensive coding. Fine. **`__kunit_is_suppressed_warning_impl()` (lib/kunit/bug.c):** ```c +bool __kunit_is_suppressed_warning_impl(bool count) +{ + if (!in_task()) + return false; + + guard(rcu)(); + list_for_each_entry_rcu(w, &suppressed_warnings, node) { + if (w->task =3D=3D current) { + if (count) + atomic_inc(&w->counter); + return true; + } + } + + return false; +} ``` The `in_task()` check at the top is essential =E2=80=94 a hardirq on the te= st task's CPU would see `current` pointing to the test task and could false= ly suppress a real warning. Good. **Lifecycle management:** ```c +static void kunit_suppress_warning_remove(struct kunit_suppressed_warning = *w) +{ + ... + list_del_rcu(&w->node); + ... + synchronize_rcu(); +} ``` The `synchronize_rcu()` after `list_del_rcu()` is the standard RCU removal = pattern and is correct here since cleanup always runs from process context = (kunit test teardown). The cover letter notes v12 tried `call_rcu()` but re= verted to `synchronize_rcu()` =E2=80=94 this is the simpler and safer choic= e. **Minor observation:** `kunit_start_suppress_warning()` calls `kunit_has_ac= tive_suppress_warning()` which internally calls `__kunit_is_suppressed_warn= ing_impl(false)`, walking the RCU list. This means every `kunit_start_suppr= ess_warning()` does a list walk to check for nesting. For the expected use = case (one suppression per test, short list) this is negligible. **Potential issue =E2=80=94 `kunit_add_action_or_reset` failure path:** ```c + spin_lock_irqsave(&suppressed_warnings_lock, flags); + list_add_rcu(&w->node, &suppressed_warnings); + spin_unlock_irqrestore(&suppressed_warnings_lock, flags); + + ret =3D kunit_add_action_or_reset(test, + kunit_suppress_warning_cleanup, w); + if (ret) { + KUNIT_FAIL(test, "Failed to add suppression cleanup action."); + return NULL; + } ``` If `kunit_add_action_or_reset()` fails, the `_or_reset` variant calls the c= leanup action itself, which does `list_del_rcu` + `synchronize_rcu`, so the= node is properly removed from the list. The memory is managed by `kunit_kz= alloc` so it will be freed at test exit. This is correct. **Scoped macro:** ```c +#define kunit_warning_suppress(test) \ + for (struct kunit_suppressed_warning *__kunit_suppress \ + __cleanup(__kunit_suppress_auto_cleanup) =3D \ + kunit_start_suppress_warning(test); \ + __kunit_suppress; \ + kunit_end_suppress_warning(test, __kunit_suppress), \ + __kunit_suppress =3D NULL) ``` This is a well-constructed scoped cleanup pattern. The `for` loop body exec= utes once (while `__kunit_suppress` is non-NULL), then the increment expres= sion calls `kunit_end_suppress_warning` and sets the pointer to NULL, termi= nating the loop. If the body exits via `break`/`return`/`goto`, `__cleanup`= fires `__kunit_suppress_auto_cleanup`. If `kunit_start_suppress_warning` r= eturns NULL (failure), the body is skipped entirely, which silently swallow= s the failure =E2=80=94 but `KUNIT_FAIL` was already called inside `kunit_s= tart_suppress_warning`, so the test is already marked failed. Correct. **No issues found in this patch.** --- --- Generated by Claude Code Patch Reviewer