From mboxrd@z Thu Jan  1 00:00:00 1970
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
Message-ID: <review-patch1-20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com>
In-Reply-To: <20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com>
References: <20260420-kunit_add_support-v7-0-e8bc6e0f70de@redhat.com> <20260420-kunit_add_support-v7-1-e8bc6e0f70de@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

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