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: Tue, 05 May 2026 08:33:31 +1000 Message-ID: In-Reply-To: <20260504-kunit_add_support-v8-1-3e5957cdd235@redhat.com> References: <20260504-kunit_add_support-v8-0-3e5957cdd235@redhat.com> <20260504-kunit_add_support-v8-1-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 **Architecture and design: Good.** The hook integration is clean =E2=80=94 one new function pointer in `kunit_= hooks_table`, one static-inline wrapper in `test-bug.h`, implementation in = `lib/kunit/bug.c`. **Issue 1 (minor): `w->counter` increment is not atomic under concurrent wa= rnings.** In `__kunit_is_suppressed_warning_impl()`: ```c guard(rcu)(); list_for_each_entry_rcu(w, &suppressed_warnings, node) { if (w->task =3D=3D current) { if (count) w->counter++; return true; } } ``` The `w->counter++` is a non-atomic increment under RCU read-side lock. Sinc= e suppression is task-scoped (`w->task =3D=3D current`), two different CPUs= cannot match the same entry simultaneously for the same `current`, so this= is actually safe. Worth noting that the cover letter's claim that "each KU= nit test runs in its own kthread" is what makes this correct =E2=80=94 if t= wo threads ever shared a suppression entry, this would race. The current de= sign correctly prevents that. **Issue 2 (minor): `synchronize_rcu()` in `kunit_suppress_warning_remove()`= may be called from kunit_release_action context.** ```c static void kunit_suppress_warning_remove(struct kunit_suppressed_warning *= w) { unsigned long flags; spin_lock_irqsave(&suppressed_warnings_lock, flags); list_del_rcu(&w->node); spin_unlock_irqrestore(&suppressed_warnings_lock, flags); synchronize_rcu(); /* Wait for readers to finish */ } ``` `synchronize_rcu()` can sleep. This is called via `kunit_release_action()` = which calls the action function directly. In normal KUnit test teardown con= text this is fine (process context). However, confirm that `kunit_release_a= ction` is never called from atomic context. Given that the existing kunit a= ction infrastructure is designed for process context, this should be safe, = but it's worth noting in a comment. **Issue 3 (question): `kunit_has_active_suppress_warning()` is a public API= but uses the implementation function.** ```c bool kunit_has_active_suppress_warning(void) { return __kunit_is_suppressed_warning_impl(false); } ``` This calls the implementation function directly rather than going through t= he hooks table (`kunit_is_suppressed_warning(false)`). This is correct beca= use `kunit_has_active_suppress_warning()` is only callable from within the = kunit module itself (it's in `lib/kunit/bug.c`), so the hook indirection is= unnecessary. Fine as-is. **Issue 4 (minor): No nesting support =E2=80=94 is this too restrictive?** ```c if (kunit_has_active_suppress_warning()) { KUNIT_FAIL(test, "Another suppression block is already active"); return NULL; } ``` The series explicitly disallows nesting. This makes sense for simplicity, b= ut the `backtrace_suppression_test_multi_scope` test in patch 2 uses sequen= tial (not nested) suppression blocks via the direct API. Consider whether a= test helper that itself needs to suppress warnings while the caller also h= as suppression active would be a valid use case. If so, this restriction ma= y need revisiting. For v8, this is an acceptable simplification =E2=80=94 i= t can always be relaxed later. **Issue 5 (correctness): `__report_bug()` suppression check placement is go= od.** ```c +#ifdef CONFIG_KUNIT + /* + * Before the once logic so suppressed warnings do not consume + * the single-fire budget of WARN_ON_ONCE(). + */ + if (warning && kunit_is_suppressed_warning(true)) + return BUG_TRAP_TYPE_WARN; +#endif ``` This is placed before the `BUGFLAG_ONCE` check, which is correct =E2=80=94 = a suppressed warning should not consume the once-budget. The `#ifdef CONFIG= _KUNIT` guard is appropriate here (matching the style of `kunit_fail_curren= t_test` usage elsewhere in kernel code), though the `kunit_is_suppressed_wa= rning()` stub already returns false when CONFIG_KUNIT is disabled, so the `= #ifdef` is belt-and-suspenders. Not a problem, just noting it's slightly re= dundant. **Issue 6 (nit): The `__cleanup` attribute usage in the scoped API is well = done.** ```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 standard pattern. The `__cleanup` attribute fires if the loop exi= ts via `break`, `return`, or `goto`. The for-loop increment handles normal = exit. `kunit_add_action()` handles the case where the kthread exits abnorma= lly (e.g., `kthread_exit` from a failed assertion). All three cleanup paths= are covered. The `__kunit_suppress =3D NULL` in the increment expression e= nsures `__cleanup` doesn't double-free. Clean. **Issue 7 (minor): `kunit_end_suppress_warning` does not check that suppres= sion is actually active for that specific handle.** ```c void kunit_end_suppress_warning(struct kunit *test, struct kunit_suppressed_warning *w) { if (!w) return; kunit_release_action(test, kunit_suppress_warning_cleanup, w); } ``` Calling this with a stale or already-ended handle would hit `kunit_release_= action` which would fail to find the action. This is fine =E2=80=94 `kunit_= release_action` handles the not-found case gracefully. No issue. --- Generated by Claude Code Patch Reviewer