From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions Date: Fri, 27 Feb 2026 15:09:14 +1000 Message-ID: In-Reply-To: <20260224132506.2631528-2-sanjay.kumar.yadav@intel.com> References: <20260224132506.2631528-2-sanjay.kumar.yadav@intel.com> <20260224132506.2631528-2-sanjay.kumar.yadav@intel.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 **Positive aspects:** - The macro is well-documented with a kernel-doc comment. - Correctly avoids double-evaluation of the condition =E2=80=94 it's evalua= ted once inside `WARN_ON(!(condition))`. - The decision to leave the `WARN_ON` in `gpu_buddy_block_trim()` alone is = sensible and explicitly called out in the commit message. - The `#include ` is now properly guarded behind `IS_ENAB= LED(CONFIG_KUNIT)`, which avoids pulling in kunit headers in production bui= lds. **Minor observations:** 1. **Return value of WARN_ON discarded in non-KUNIT path**: In the `#else` = branch: ```c #define gpu_buddy_assert(condition) WARN_ON(!(condition)) ``` `WARN_ON()` returns a value (the condition), but all call sites discard it = (the macro is used as a statement, not in an `if`). This is fine for the cu= rrent usage, but if someone later tries to use `if (gpu_buddy_assert(...))`= , the KUNIT path (wrapped in `do { } while(0)`) would not compile while the= non-KUNIT path would. This asymmetry is minor since the macro name `_asser= t` doesn't suggest a return value, but for consistency you could wrap the `= #else` case in `do { } while(0)` as well: ```c #define gpu_buddy_assert(condition) do { WARN_ON(!(condition)); } while (0) ``` 2. **`#include ` addition**: The old code included `` unconditionally, which transitively provided `WARN_ON` via its in= clude chain. Now that `kunit/test-bug.h` is conditional, adding `` explicitly is correct. However, it's worth noting that `WARN_ON` is li= kely already available through other transitive includes (e.g., `` via ``), so the explicit include is defensive =E2=80= =94 which is good practice. 3. **Stringification readability for the negated case**: At the `__gpu_budd= y_free_list` site: ```c gpu_buddy_assert(!(mark_dirty && mark_clear)); ``` The kunit failure message will read `"gpu_buddy_assert(!(mark_dirty && mark= _clear))"`, which is clear enough, though slightly verbose. This is a trivi= al aesthetic point. 4. **Scope of the macro**: The macro is defined locally in `buddy.c` rather= than in the `` header. This is appropriate since all cu= rrent users are in this single file, following the principle of narrowest s= cope. **Correctness of each hunk:** - **`__force_merge`**: `WARN_ON(clear(block) =3D=3D clear(buddy))` =E2=86= =92 `gpu_buddy_assert(clear(block) !=3D clear(buddy))` =E2=80=94 logically = equivalent, correct. - **`gpu_buddy_fini` root check**: `if (WARN_ON(!is_free(...))) kunit_fail_= current_test(...)` =E2=86=92 `gpu_buddy_assert(is_free(...))` =E2=80=94 equ= ivalent, cleaner. - **`gpu_buddy_fini` avail check**: `WARN_ON(mm->avail !=3D mm->size)` =E2= =86=92 `gpu_buddy_assert(mm->avail =3D=3D mm->size)` =E2=80=94 correct inve= rsion, adds kunit reporting that was previously missing. - **`__gpu_buddy_free_list`**: `WARN_ON(mark_dirty && mark_clear)` =E2=86= =92 `gpu_buddy_assert(!(mark_dirty && mark_clear))` =E2=80=94 correct inver= sion, adds kunit reporting that was previously missing. **Verdict:** The patch is straightforward, correct, and improves consistenc= y. The only actionable suggestion is wrapping the `#else` path in `do { } w= hile(0)` for macro hygiene symmetry. Otherwise: Reviewed-by worthy with the minor `do { } while(0)` nit optionally addresse= d. --- Generated by Claude Code Patch Reviewer