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 09:36:17 +1000
Message-ID: <review-patch1-20260515-kunit_add_support-v12-1-a216dc228be8@redhat.com>
In-Reply-To: <20260515-kunit_add_support-v12-1-a216dc228be8@redhat.com>
References: <20260515-kunit_add_support-v12-0-a216dc228be8@redhat.com> <20260515-kunit_add_support-v12-1-a216dc228be8@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

**Design and correctness look good.** The hook integration is clean and fol=
lows the existing pattern exactly.

**1. Suppression check placement in `__report_bug()` is well-placed:**

```c
+	/*
+	 * 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;
+
+	disable_trace_on_warning();
```

Good decision to return before both the `once` flag consumption and `disabl=
e_trace_on_warning()`. A suppressed warning shouldn't eat the `WARN_ON_ONCE=
()` budget or disable tracing.

**2. `kunit_start_suppress_warning` uses plain `kzalloc` (not `kunit_kzallo=
c`):**

```c
+	w =3D kzalloc(sizeof(*w), GFP_KERNEL);
```

This is correct since the lifetime is manually managed via `call_rcu` -> `k=
free`, but it's worth noting the cover letter says "dynamically allocated v=
ia kunit_kzalloc()" which is inaccurate =E2=80=94 only the overall cover le=
tter, not the patch 1 commit message which correctly says `kzalloc()`.

**3. The `kunit_is_suppressed_warning` inline checks the function pointer b=
efore calling:**

```c
+	return kunit_hooks.is_suppressed_warning &&
+	       kunit_hooks.is_suppressed_warning(count);
```

This NULL check is correct =E2=80=94 the hook may not be installed if the k=
unit module hasn't loaded yet but `kunit_running` was somehow set. Defensiv=
e and appropriate.

**4. `kunit_end_suppress_warning` silently returns on NULL:**

```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);
+}
```

This makes the direct API forgiving when `kunit_start_suppress_warning` ret=
urned NULL on failure. That's the right choice since `kunit_start_suppress_=
warning` already calls `KUNIT_FAIL`.

**5. The `__kunit_suppress_auto_cleanup` accesses `(*wp)->test`:**

```c
+void __kunit_suppress_auto_cleanup(struct kunit_suppressed_warning **wp)
+{
+	if (*wp)
+		kunit_end_suppress_warning((*wp)->test, *wp);
+}
```

This is fine =E2=80=94 the `__cleanup` attribute fires on scope exit, befor=
e the action teardown, so the handle is still valid at this point.

**6. Minor: `__kunit_is_suppressed_warning_impl` is not exported:**

```c
+bool __kunit_is_suppressed_warning_impl(bool count)
+{
```

No `EXPORT_SYMBOL_GPL`. This is correct since it's only called indirectly v=
ia the hook function pointer, and `kunit_has_active_suppress_warning()` (wh=
ich calls it) IS exported. But `kunit_has_active_suppress_warning` is defin=
ed in `bug.c` and calls `__kunit_is_suppressed_warning_impl` directly, so t=
hey're in the same compilation unit =E2=80=94 no export needed.

---
Generated by Claude Code Patch Reviewer