From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: bug/kunit: Core support for suppressing warning backtraces
Date: Thu, 23 Apr 2026 09:52:26 +1000 [thread overview]
Message-ID: <review-patch1-20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com> (raw)
In-Reply-To: <20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com>
Patch Review
**Missing write-side lock for RCU list (bug)**
The `suppressed_warnings` list is read under `rcu_read_lock()` correctly, but writes have no mutual exclusion:
```c
+static LIST_HEAD(suppressed_warnings);
...
+ list_add_rcu(&warning->node, &suppressed_warnings);
...
+ list_del_rcu(&warning->node);
```
The standard RCU list pattern requires a spinlock (or equivalent) protecting all write-side operations. Two concurrent calls to `__kunit_start_suppress_warning()` (e.g., from parallel test suites, or concurrent tests with `--run_concurrently`) would corrupt the list. This should be:
```c
static DEFINE_SPINLOCK(suppressed_warnings_lock);
// in __kunit_start_suppress_warning():
spin_lock(&suppressed_warnings_lock);
list_add_rcu(&warning->node, &suppressed_warnings);
spin_unlock(&suppressed_warnings_lock);
// in __kunit_suppress_warning_remove():
spin_lock(&suppressed_warnings_lock);
list_del_rcu(&warning->node);
spin_unlock(&suppressed_warnings_lock);
synchronize_rcu();
```
**`disable_trace_on_warning()` side effect for suppressed warnings**
In `__report_bug()`, the suppression check is placed after `disable_trace_on_warning()`:
```c
disable_trace_on_warning();
bug_get_file_line(bug, &file, &line);
fmt = bug_get_format(bug);
warning = bug->flags & BUGFLAG_WARNING;
...
+ if (warning && __kunit_is_suppressed_warning())
+ return BUG_TRAP_TYPE_WARN;
```
A suppressed warning will still call `disable_trace_on_warning()` and decode the bug table entry, which is wasted work and potentially disruptive. Consider moving the suppression check earlier, or at least before `disable_trace_on_warning()`. That said, this would require restructuring since `warning` is derived from bug flags parsed afterward, so this may be intentional given the "minimum changes to generic code" goal.
**WARN_ON_ONCE behavior is correct**
The placement of the suppression check before the `once` logic is well-considered:
```c
+ if (warning && __kunit_is_suppressed_warning())
+ return BUG_TRAP_TYPE_WARN;
+
if (warning && once) {
if (done)
return BUG_TRAP_TYPE_WARN;
bug->flags |= BUGFLAG_DONE;
}
```
This correctly prevents suppressed warnings from consuming the single-fire budget of `WARN_ON_ONCE()`, and the comment explaining this is helpful.
**No failure reporting on allocation failure**
In `__kunit_start_suppress_warning()`, if `kunit_kzalloc()` fails, the function silently returns NULL and suppression is disabled. The macros handle NULL gracefully, but the test would proceed without suppression (potentially spewing backtraces) with no indication to the user. Consider adding a `kunit_warn()` or `KUNIT_ASSERT_NOT_NULL()` so that allocation failure is visible.
**Header design is clean**
The `#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE` / `#else` stub pattern in `include/kunit/bug.h` is well-done, with the disabled path providing no-op macros and an inline false-returning function.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-04-22 23:52 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-20 12:28 [PATCH v7 0/5] kunit: Add support for suppressing warning backtraces Albert Esteve
2026-04-20 12:28 ` [PATCH v7 1/5] bug/kunit: Core " Albert Esteve
2026-04-20 14:39 ` Peter Zijlstra
2026-04-21 8:22 ` Albert Esteve
2026-04-20 14:45 ` Peter Zijlstra
2026-04-21 8:29 ` Albert Esteve
2026-04-22 12:19 ` David Gow
2026-04-22 23:52 ` Claude Code Review Bot [this message]
2026-04-20 12:28 ` [PATCH v7 2/5] bug/kunit: Reduce runtime impact of warning backtrace suppression Albert Esteve
2026-04-20 14:44 ` Peter Zijlstra
2026-04-21 8:41 ` Albert Esteve
2026-04-22 12:19 ` David Gow
2026-04-22 23:52 ` Claude review: " Claude Code Review Bot
2026-04-20 12:28 ` [PATCH v7 3/5] kunit: Add backtrace suppression self-tests Albert Esteve
2026-04-22 12:20 ` David Gow
2026-04-22 23:52 ` Claude review: " Claude Code Review Bot
2026-04-20 12:28 ` [PATCH v7 4/5] drm: Suppress intentional warning backtraces in scaling unit tests Albert Esteve
2026-04-20 14:47 ` Peter Zijlstra
2026-04-21 8:49 ` Albert Esteve
2026-04-21 11:50 ` Jani Nikula
2026-04-22 12:20 ` David Gow
2026-04-22 23:52 ` Claude review: " Claude Code Review Bot
2026-04-20 12:28 ` [PATCH v7 5/5] kunit: Add documentation for warning backtrace suppression API Albert Esteve
2026-04-22 12:20 ` David Gow
2026-04-22 23:52 ` Claude review: " Claude Code Review Bot
2026-04-22 23:52 ` Claude review: kunit: Add support for suppressing warning backtraces Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-05-04 7:41 [PATCH v8 0/4] " Albert Esteve
2026-05-04 7:41 ` [PATCH v8 1/4] bug/kunit: Core " Albert Esteve
2026-05-04 22:33 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch1-20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox