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: Sat, 16 May 2026 12:29:13 +1000
Message-ID: <review-patch1-20260513-kunit_add_support-v10-1-e379d206c8cd@redhat.com>
In-Reply-To: <20260513-kunit_add_support-v10-1-e379d206c8cd@redhat.com>
References: <20260513-kunit_add_support-v10-0-e379d206c8cd@redhat.com> <20260513-kunit_add_support-v10-1-e379d206c8cd@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

**test-bug.h changes =E2=80=94 Good:**

The `kunit_is_suppressed_warning()` inline correctly gates on the existing =
`kunit_running` static branch before dereferencing the hook pointer:

```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 `kunit_hooks.is_suppressed_warning` before calling is imp=
ortant for the window between kunit_running being enabled and hooks being i=
nstalled. The `#include <linux/types.h>` for `bool` is the right fix for `C=
ONFIG_KUNIT=3Dn` builds.

**kernel/panic.c changes =E2=80=94 Good:**

```c
if (kunit_is_suppressed_warning(true)) {
	warn_rcu_exit(rcu);
	return;
}
```

In `warn_slowpath_fmt()`, the check with `count=3Dtrue` is placed after `wa=
rn_rcu_enter()` and correctly exits via `warn_rcu_exit()` before returning.=
 Same pattern in `__warn_printk()` but with `count=3Dfalse` to avoid double=
-counting. This is correct =E2=80=94 on architectures with `__WARN_FLAGS`, =
both `__warn_printk()` and `__report_bug()` fire for the same warning; coun=
ting only in `__report_bug()` ensures one increment per warning.

**lib/bug.c changes =E2=80=94 Correct but worth noting:**

```c
#ifdef CONFIG_KUNIT
	if (warning && kunit_is_suppressed_warning(true))
		return BUG_TRAP_TYPE_WARN;
#endif

	disable_trace_on_warning();
```

The `disable_trace_on_warning()` is moved after the suppression check, whic=
h means suppressed warnings won't disable tracing. This is the right behavi=
or =E2=80=94 suppressed warnings should have zero side effects. The `#ifdef=
 CONFIG_KUNIT` guard avoids a function call in non-KUnit builds.

Placing the check before the `once` logic is correct: suppressed WARN_ON_ON=
CE() calls don't consume the single-fire budget, so they remain armed for n=
on-test contexts.

**bug.c (new file) =E2=80=94 Minor observations:**

The `synchronize_rcu()` in `kunit_suppress_warning_remove()` is correct to =
ensure no readers see a stale pointer, but it means removal is a blocking c=
all. This is fine for test teardown paths but means `kunit_end_suppress_war=
ning()` (which calls `kunit_release_action()` =E2=86=92 `kunit_suppress_war=
ning_remove()`) blocks on a grace period. In test contexts this is acceptab=
le.

The `__kunit_is_suppressed_warning_impl()` function:
```c
bool __kunit_is_suppressed_warning_impl(bool count)
{
	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;
}
```

This is called from `__warn_printk()` which can be in `noinstr` context on =
some architectures. The cover letter mentions this was addressed ("Made __k=
unit_is_suppressed_warning nice to noinstr functions"), but the static bran=
ch check in the inline wrapper ensures this function is only reached when K=
Unit is running, which won't happen in early boot noinstr paths. The `guard=
(rcu)()` use is clean.

**One subtle issue:** `kunit_start_suppress_warning()` calls `kunit_has_act=
ive_suppress_warning()` which calls `__kunit_is_suppressed_warning_impl(fal=
se)`. This does an RCU-protected list walk to check if suppression is activ=
e for `current`. But the caller hasn't taken any locks for the list yet =E2=
=80=94 this is fine because the check is inherently racy (the list could ch=
ange between check and `list_add_rcu()`), but since all calls are from the =
same kthread and suppression is per-task, no other thread would be adding e=
ntries for our task. So the check-then-act pattern is safe here.

---
Generated by Claude Code Patch Reviewer