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/xe/tests: avoid build warning on 32-bit targets Date: Thu, 05 Mar 2026 13:42:12 +1000 Message-ID: In-Reply-To: <20260304083741.725121-1-arnd@kernel.org> References: <20260304083741.725121-1-arnd@kernel.org> <20260304083741.725121-1-arnd@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The problem is correctly identified** =E2=80=94 `resource_size_t` is `u32= ` on 32-bit and the constant `0x37a800000ull` (~14.9 GiB) doesn't fit. **The fix is problematic for several reasons:** 1. **Silent data corruption in tests.** The cast truncates `0x37a800000` to= `0x7a800000` (~1.9 GiB). This changes the behavior of every test that reli= es on `TEST_VRAM`: - At line 71, `vram->usable_size =3D TEST_VRAM;` would set a much smalle= r VRAM size - At line 225, `TEST_VRAM` appears in the `vram_sizes[]` array as a `u64= `, where it would also be truncated (the array is `u64` but the macro would= expand to a truncated value) - At line 280, `KUNIT_EXPECT_GE(test, TEST_VRAM, num_vfs * pf_profile_fa= ir_lmem(gt, num_vfs));` would compare against the truncated value Wait =E2=80=94 actually looking again at line 219: ```c static const u64 vram_sizes[] =3D { ... TEST_VRAM, }; ``` With the proposed change, `TEST_VRAM` expands to `(resource_size_t)0x37a= 800000ull`, which in a `u64` array initializer would store the truncated 32= -bit value, changing test semantics. 2. **The cast syntax is a C-style cast without parentheses around the full = expression.** While it works, the more conventional kernel style would be t= o wrap it: `((resource_size_t)0x37a800000ull)`. Without the outer parens, t= he macro could cause surprises in some expression contexts. 3. **Better alternatives exist:** - **Skip on 32-bit:** Since the Xe driver is only meaningful on 64-bit x= 86 platforms, the entire test file could be gated with `#if BITS_PER_LONG = =3D=3D 64` or a Kconfig dependency. The Xe driver itself likely already dep= ends on 64-bit (`CONFIG_64BIT`). =20 - **Use a smaller value on 32-bit:** An `#ifdef` could select a smaller = VRAM size that fits in 32 bits: ```c #if BITS_PER_LONG =3D=3D 64 #define TEST_VRAM 0x37a800000ull #else #define TEST_VRAM 0x7a800000ull #endif ``` =20 - **Check if Xe even builds on 32-bit:** If `CONFIG_DRM_XE` depends on `= CONFIG_64BIT` (which it likely does for a discrete GPU driver), this warnin= g may only appear in contrived build configurations and the fix may be unne= cessary. **Recommendation:** Check whether `CONFIG_DRM_XE` already depends on `CONFI= G_64BIT`. If so, this patch is unnecessary. If not, the better fix is to ad= d that dependency or skip the test on 32-bit, rather than silently truncati= ng test data. The author's instinct in the cover letter =E2=80=94 "please t= reat this as a bug report" =E2=80=94 is the right call here. --- Generated by Claude Code Patch Reviewer