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

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

Patch Review

**Architecture and design: Good.**

The hook integration is clean — one new function pointer in `kunit_hooks_table`, one static-inline wrapper in `test-bug.h`, implementation in `lib/kunit/bug.c`.

**Issue 1 (minor): `w->counter` increment is not atomic under concurrent warnings.**

In `__kunit_is_suppressed_warning_impl()`:
```c
	guard(rcu)();
	list_for_each_entry_rcu(w, &suppressed_warnings, node) {
		if (w->task == current) {
			if (count)
				w->counter++;
			return true;
		}
	}
```

The `w->counter++` is a non-atomic increment under RCU read-side lock. Since suppression is task-scoped (`w->task == current`), two different CPUs cannot match the same entry simultaneously for the same `current`, so this is actually safe. Worth noting that the cover letter's claim that "each KUnit test runs in its own kthread" is what makes this correct — if two threads ever shared a suppression entry, this would race. The current design correctly prevents that.

**Issue 2 (minor): `synchronize_rcu()` in `kunit_suppress_warning_remove()` may be called from kunit_release_action context.**

```c
static void kunit_suppress_warning_remove(struct kunit_suppressed_warning *w)
{
	unsigned long flags;

	spin_lock_irqsave(&suppressed_warnings_lock, flags);
	list_del_rcu(&w->node);
	spin_unlock_irqrestore(&suppressed_warnings_lock, flags);
	synchronize_rcu(); /* Wait for readers to finish */
}
```

`synchronize_rcu()` can sleep. This is called via `kunit_release_action()` which calls the action function directly. In normal KUnit test teardown context this is fine (process context). However, confirm that `kunit_release_action` is never called from atomic context. Given that the existing kunit action infrastructure is designed for process context, this should be safe, but it's worth noting in a comment.

**Issue 3 (question): `kunit_has_active_suppress_warning()` is a public API but uses the implementation function.**

```c
bool kunit_has_active_suppress_warning(void)
{
	return __kunit_is_suppressed_warning_impl(false);
}
```

This calls the implementation function directly rather than going through the hooks table (`kunit_is_suppressed_warning(false)`). This is correct because `kunit_has_active_suppress_warning()` is only callable from within the kunit module itself (it's in `lib/kunit/bug.c`), so the hook indirection is unnecessary. Fine as-is.

**Issue 4 (minor): No nesting support — is this too restrictive?**

```c
	if (kunit_has_active_suppress_warning()) {
		KUNIT_FAIL(test, "Another suppression block is already active");
		return NULL;
	}
```

The series explicitly disallows nesting. This makes sense for simplicity, but the `backtrace_suppression_test_multi_scope` test in patch 2 uses sequential (not nested) suppression blocks via the direct API. Consider whether a test helper that itself needs to suppress warnings while the caller also has suppression active would be a valid use case. If so, this restriction may need revisiting. For v8, this is an acceptable simplification — it can always be relaxed later.

**Issue 5 (correctness): `__report_bug()` suppression check placement is good.**

```c
+#ifdef CONFIG_KUNIT
+	/*
+	 * 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;
+#endif
```

This is placed before the `BUGFLAG_ONCE` check, which is correct — a suppressed warning should not consume the once-budget. The `#ifdef CONFIG_KUNIT` guard is appropriate here (matching the style of `kunit_fail_current_test` usage elsewhere in kernel code), though the `kunit_is_suppressed_warning()` stub already returns false when CONFIG_KUNIT is disabled, so the `#ifdef` is belt-and-suspenders. Not a problem, just noting it's slightly redundant.

**Issue 6 (nit): The `__cleanup` attribute usage in the scoped API is well done.**

```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 standard pattern. The `__cleanup` attribute fires if the loop exits via `break`, `return`, or `goto`. The for-loop increment handles normal exit. `kunit_add_action()` handles the case where the kthread exits abnormally (e.g., `kthread_exit` from a failed assertion). All three cleanup paths are covered. The `__kunit_suppress = NULL` in the increment expression ensures `__cleanup` doesn't double-free. Clean.

**Issue 7 (minor): `kunit_end_suppress_warning` does not check that suppression is actually active for that specific handle.**

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

Calling this with a stale or already-ended handle would hit `kunit_release_action` which would fail to find the action. This is fine — `kunit_release_action` handles the not-found case gracefully. No issue.

---
Generated by Claude Code Patch Reviewer