Claude review: bug/kunit: Core support for suppressing warning backtraces

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Architecture & correctness: Good**

The hook integration pattern follows the existing `fail_current_test` / `get_static_stub_address` approach exactly, which is the right way to extend this.

**`__report_bug()` changes (lib/bug.c):**

The placement of the suppression check *before* the `WARN_ON_ONCE` single-fire logic is a critical design choice and is correct:

```c
+	if (warning && kunit_is_suppressed_warning(true))
+		return BUG_TRAP_TYPE_WARN;
+
+	disable_trace_on_warning();
+
 	if (warning && once) {
 		if (done)
 			return BUG_TRAP_TYPE_WARN;
 		bug->flags |= BUGFLAG_DONE;
 	}
```

This ensures suppressed warnings don't consume the single-fire budget of `WARN_ON_ONCE()`, which is the right behavior. The movement of `disable_trace_on_warning()` below the suppression check is also correct — no need to disable tracing for a suppressed warning.

**`kunit_is_suppressed_warning()` inline (test-bug.h):**

```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 `is_suppressed_warning` is belt-and-suspenders since `kunit_install_hooks()` sets all pointers at module init, but it's harmless and consistent with defensive coding. Fine.

**`__kunit_is_suppressed_warning_impl()` (lib/kunit/bug.c):**

```c
+bool __kunit_is_suppressed_warning_impl(bool count)
+{
+	if (!in_task())
+		return false;
+
+	guard(rcu)();
+	list_for_each_entry_rcu(w, &suppressed_warnings, node) {
+		if (w->task == current) {
+			if (count)
+				atomic_inc(&w->counter);
+			return true;
+		}
+	}
+
+	return false;
+}
```

The `in_task()` check at the top is essential — a hardirq on the test task's CPU would see `current` pointing to the test task and could falsely suppress a real warning. Good.

**Lifecycle management:**

```c
+static void kunit_suppress_warning_remove(struct kunit_suppressed_warning *w)
+{
+	...
+	list_del_rcu(&w->node);
+	...
+	synchronize_rcu();
+}
```

The `synchronize_rcu()` after `list_del_rcu()` is the standard RCU removal pattern and is correct here since cleanup always runs from process context (kunit test teardown). The cover letter notes v12 tried `call_rcu()` but reverted to `synchronize_rcu()` — this is the simpler and safer choice.

**Minor observation:** `kunit_start_suppress_warning()` calls `kunit_has_active_suppress_warning()` which internally calls `__kunit_is_suppressed_warning_impl(false)`, walking the RCU list. This means every `kunit_start_suppress_warning()` does a list walk to check for nesting. For the expected use case (one suppression per test, short list) this is negligible.

**Potential issue — `kunit_add_action_or_reset` failure path:**

```c
+	spin_lock_irqsave(&suppressed_warnings_lock, flags);
+	list_add_rcu(&w->node, &suppressed_warnings);
+	spin_unlock_irqrestore(&suppressed_warnings_lock, flags);
+
+	ret = kunit_add_action_or_reset(test,
+					kunit_suppress_warning_cleanup, w);
+	if (ret) {
+		KUNIT_FAIL(test, "Failed to add suppression cleanup action.");
+		return NULL;
+	}
```

If `kunit_add_action_or_reset()` fails, the `_or_reset` variant calls the cleanup action itself, which does `list_del_rcu` + `synchronize_rcu`, so the node is properly removed from the list. The memory is managed by `kunit_kzalloc` so it will be freed at test exit. This is correct.

**Scoped macro:**

```c
+#define kunit_warning_suppress(test)					\
+	for (struct kunit_suppressed_warning *__kunit_suppress		\
+	     __cleanup(__kunit_suppress_auto_cleanup) =			\
+	     kunit_start_suppress_warning(test);			\
+	     __kunit_suppress;						\
+	     kunit_end_suppress_warning(test, __kunit_suppress),	\
+	     __kunit_suppress = NULL)
```

This is a well-constructed scoped cleanup pattern. The `for` loop body executes once (while `__kunit_suppress` is non-NULL), then the increment expression calls `kunit_end_suppress_warning` and sets the pointer to NULL, terminating the loop. If the body exits via `break`/`return`/`goto`, `__cleanup` fires `__kunit_suppress_auto_cleanup`. If `kunit_start_suppress_warning` returns NULL (failure), the body is skipped entirely, which silently swallows the failure — but `KUNIT_FAIL` was already called inside `kunit_start_suppress_warning`, so the test is already marked failed. Correct.

**No issues found in this patch.**

---

---
Generated by Claude Code Patch Reviewer