public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/xe/tests: avoid build warning on 32-bit targets
@ 2026-03-04  8:37 Arnd Bergmann
  2026-03-04  8:51 ` Michal Wajdeczko
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Arnd Bergmann @ 2026-03-04  8:37 UTC (permalink / raw)
  To: Matthew Brost, Thomas Hellström, Rodrigo Vivi, David Airlie,
	Simona Vetter, Michal Wajdeczko, Piotr Piórkowski
  Cc: Arnd Bergmann, intel-xe, dri-devel, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

Building the test case on 32-bit targets produces an integer overflow warning,
as a constant value is assigned to a 32-bit resource_size_t variable:

In file included from drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c:3329:
drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c: In function 'pf_gt_config_test_init':
drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c:14:25: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '14940110848' to '2055208960' [-Werror=overflow]
   14 | #define TEST_VRAM       0x37a800000ull
      |                         ^~~~~~~~~~~~~~
drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c:71:29: note: in expansion of macro 'TEST_VRAM'
   71 |         vram->usable_size = TEST_VRAM;
      |                             ^~~~~~~~~

Shut up the warning with an extra cast that marks this truncation as intentional.
This is probably not the right fix here, but I could not figure out where the
constant value actually comes from, or if a smaller number would be appropriate
on a 32-bit system. It's possible that the test case or the driver is just not
useful on 32-bit machines because of other parts of the logic here.

Fixes: cbe29da6f7c0 ("drm/xe/tests: Add KUnit tests for new VRAM fair provisioning")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
If there is a better way to fix this, please treat this as a bug report and
just add a Reported-by tag in the commit.
---
 drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c b/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
index 305dbd4e5d1a..86cd15834bac 100644
--- a/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
+++ b/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
@@ -11,7 +11,7 @@
 #include "xe_pci_test.h"
 
 #define TEST_MAX_VFS	63
-#define TEST_VRAM	0x37a800000ull
+#define TEST_VRAM	(resource_size_t)0x37a800000ull
 
 static void pf_set_admin_mode(struct xe_device *xe, bool enable)
 {
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/xe/tests: avoid build warning on 32-bit targets
  2026-03-04  8:37 [PATCH] drm/xe/tests: avoid build warning on 32-bit targets Arnd Bergmann
@ 2026-03-04  8:51 ` Michal Wajdeczko
  2026-03-04  9:06   ` Arnd Bergmann
  2026-03-05  3:42 ` Claude review: " Claude Code Review Bot
  2026-03-05  3:42 ` Claude Code Review Bot
  2 siblings, 1 reply; 5+ messages in thread
From: Michal Wajdeczko @ 2026-03-04  8:51 UTC (permalink / raw)
  To: Arnd Bergmann, Matthew Brost, Thomas Hellström, Rodrigo Vivi,
	David Airlie, Simona Vetter, Piotr Piórkowski
  Cc: Arnd Bergmann, intel-xe, dri-devel, linux-kernel

Hi Arnd,


On 3/4/2026 9:37 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Building the test case on 32-bit targets produces an integer overflow warning,
> as a constant value is assigned to a 32-bit resource_size_t variable:
> 
> In file included from drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c:3329:
> drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c: In function 'pf_gt_config_test_init':
> drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c:14:25: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '14940110848' to '2055208960' [-Werror=overflow]
>    14 | #define TEST_VRAM       0x37a800000ull
>       |                         ^~~~~~~~~~~~~~
> drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c:71:29: note: in expansion of macro 'TEST_VRAM'
>    71 |         vram->usable_size = TEST_VRAM;
>       |                             ^~~~~~~~~
> 
> Shut up the warning with an extra cast that marks this truncation as intentional.
> This is probably not the right fix here, but I could not figure out where the
> constant value actually comes from, or if a smaller number would be appropriate
> on a 32-bit system. It's possible that the test case or the driver is just not
> useful on 32-bit machines because of other parts of the logic here.
> 
> Fixes: cbe29da6f7c0 ("drm/xe/tests: Add KUnit tests for new VRAM fair provisioning")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> If there is a better way to fix this, please treat this as a bug report and
> just add a Reported-by tag in the commit.

this was already fixed and pushed [1]

[1] https://gitlab.freedesktop.org/drm/xe/kernel/-/commit/4f18a79b3585a28c9f73f859fe83f12d0eccc736

> ---
>  drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c b/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
> index 305dbd4e5d1a..86cd15834bac 100644
> --- a/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
> +++ b/drivers/gpu/drm/xe/tests/xe_gt_sriov_pf_config_kunit.c
> @@ -11,7 +11,7 @@
>  #include "xe_pci_test.h"
>  
>  #define TEST_MAX_VFS	63
> -#define TEST_VRAM	0x37a800000ull
> +#define TEST_VRAM	(resource_size_t)0x37a800000ull
>  
>  static void pf_set_admin_mode(struct xe_device *xe, bool enable)
>  {


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/xe/tests: avoid build warning on 32-bit targets
  2026-03-04  8:51 ` Michal Wajdeczko
@ 2026-03-04  9:06   ` Arnd Bergmann
  0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2026-03-04  9:06 UTC (permalink / raw)
  To: Michal Wajdeczko, Arnd Bergmann, Matthew Brost,
	Thomas Hellström, Rodrigo Vivi, Dave Airlie, Simona Vetter,
	Piotr Piórkowski
  Cc: intel-xe, dri-devel, linux-kernel

On Wed, Mar 4, 2026, at 09:51, Michal Wajdeczko wrote:
> On 3/4/2026 9:37 AM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> ---
>> If there is a better way to fix this, please treat this as a bug report and
>> just add a Reported-by tag in the commit.
>
> this was already fixed and pushed [1]
>
> [1] 
> https://gitlab.freedesktop.org/drm/xe/kernel/-/commit/4f18a79b3585a28c9f73f859fe83f12d0eccc736
>

Right, that looks better than my version. Apparently it just
missed the current linux-next that I was testing on.

     Arnd

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/xe/tests: avoid build warning on 32-bit targets
  2026-03-04  8:37 [PATCH] drm/xe/tests: avoid build warning on 32-bit targets Arnd Bergmann
  2026-03-04  8:51 ` Michal Wajdeczko
  2026-03-05  3:42 ` Claude review: " Claude Code Review Bot
@ 2026-03-05  3:42 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-03-05  3:42 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/xe/tests: avoid build warning on 32-bit targets
Author: Arnd Bergmann <arnd@kernel.org>
Patches: 3
Reviewed: 2026-03-05T13:42:12.647626

---

This is a single-patch fix for a 32-bit build warning in the Xe KUnit tests. The problem is real: `TEST_VRAM` (0x37a800000, ~14.9 GiB) is assigned to a `resource_size_t` field (`usable_size`), which is only 32 bits wide on 32-bit targets, causing silent truncation and a `-Werror=overflow` build failure.

However, the proposed fix of casting with `(resource_size_t)` is a band-aid that **silences the warning while making the tests silently broken on 32-bit**. The value truncates to ~1.9 GiB, which would change the semantics of multiple test cases that use `TEST_VRAM`. The author acknowledges this in the commit message and cover letter, essentially offering it as a bug report.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/xe/tests: avoid build warning on 32-bit targets
  2026-03-04  8:37 [PATCH] drm/xe/tests: avoid build warning on 32-bit targets Arnd Bergmann
  2026-03-04  8:51 ` Michal Wajdeczko
@ 2026-03-05  3:42 ` Claude Code Review Bot
  2026-03-05  3:42 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-03-05  3:42 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The problem is correctly identified** — `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 relies on `TEST_VRAM`:

   - At line 71, `vram->usable_size = TEST_VRAM;` would set a much smaller 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_fair_lmem(gt, num_vfs));` would compare against the truncated value

   Wait — actually looking again at line 219:
   ```c
   static const u64 vram_sizes[] = {
       ...
       TEST_VRAM,
   };
   ```
   With the proposed change, `TEST_VRAM` expands to `(resource_size_t)0x37a800000ull`, 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 to wrap it: `((resource_size_t)0x37a800000ull)`. Without the outer parens, the 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 x86 platforms, the entire test file could be gated with `#if BITS_PER_LONG == 64` or a Kconfig dependency. The Xe driver itself likely already depends on 64-bit (`CONFIG_64BIT`).
   
   - **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 == 64
     #define TEST_VRAM    0x37a800000ull
     #else
     #define TEST_VRAM    0x7a800000ull
     #endif
     ```
   
   - **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 warning may only appear in contrived build configurations and the fix may be unnecessary.

**Recommendation:** Check whether `CONFIG_DRM_XE` already depends on `CONFIG_64BIT`. If so, this patch is unnecessary. If not, the better fix is to add that dependency or skip the test on 32-bit, rather than silently truncating test data. The author's instinct in the cover letter — "please treat this as a bug report" — is the right call here.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-03-05  3:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-04  8:37 [PATCH] drm/xe/tests: avoid build warning on 32-bit targets Arnd Bergmann
2026-03-04  8:51 ` Michal Wajdeczko
2026-03-04  9:06   ` Arnd Bergmann
2026-03-05  3:42 ` Claude review: " Claude Code Review Bot
2026-03-05  3:42 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox