From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm_buddy: fix power-of-2 rounding errs Date: Tue, 10 Mar 2026 12:46:56 +1000 Message-ID: In-Reply-To: <20260308223538.96729-2-jim.cromie@gmail.com> References: <20260308223538.96729-1-jim.cromie@gmail.com> <20260308223538.96729-2-jim.cromie@gmail.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 **Problem analysis**: Correct. `roundup_pow_of_two()` and `rounddown_pow_of= _two()` both use `unsigned long` internally (confirmed at `linux/include/li= nux/log2.h:55-67` =E2=80=94 `1UL << fls_long(n - 1)` and `1UL << (fls_long(= n) - 1)`). On 32-bit arches, `unsigned long` is 32 bits, so any `u64` value= > 4GB is silently truncated. **Rounddown fix** (line 306): ```c - modify_size =3D rounddown_pow_of_two(size); + modify_size =3D 1ULL << ilog2(size); ``` This is correct. `ilog2()` handles `u64` properly, and `1ULL << ilog2(size)= ` is equivalent to `rounddown_pow_of_two(size)` for non-zero, non-power-of-= two inputs. For inputs that are already powers of two, both return the same= value. **Roundup fix** (line 315): ```c - size =3D roundup_pow_of_two(size); + size =3D 1ULL << (ilog2(size - 1) + 1); ``` **Bug**: This has an edge case problem. When `size` is already a power of t= wo (e.g., `size =3D 4`), `ilog2(size - 1)` =3D `ilog2(3)` =3D 1, so the res= ult is `1ULL << 2` =3D 4, which is correct. When `size =3D 1`, `ilog2(0)` i= s undefined behavior =E2=80=94 `ilog2(0)` returns `-1` on most implementati= ons, so `1ULL << 0` =3D 1, which happens to work but relies on undefined be= havior for `ilog2(0)`. However, given the calling context (this is inside `drm_buddy_alloc_blocks`= with contiguous allocation), `size` should never be 0 or 1 in practice (it= 's a memory allocation size that's been validated), so this is likely fine = in practice. **Suggestion**: The commit message mentions "I'll use it to implement safe = rounding" which is a bit informal for a commit message =E2=80=94 first pers= on and future tense. Consider also adding a `Fixes:` tag =E2=80=94 the orig= inal `rounddown_pow_of_two()` / `roundup_pow_of_two()` calls have been pres= ent since the initial drm_buddy code was merged, and this is a legitimate b= ug on 32-bit architectures. Also, the kernel now has `roundup_pow_of_two_u6= 4()` / `rounddown_pow_of_two_u64()` in some trees =E2=80=94 it might be wor= th checking if those helpers exist or are more appropriate than hand-rollin= g the `ilog2` pattern. **Missing Cc: stable**: For a genuine 32-bit architecture bug, a `Cc: stabl= e@vger.kernel.org` tag would be appropriate. --- Generated by Claude Code Patch Reviewer