Re: [PATCH v2 3/7] drm/xe/guc: Print register addresses in capture snapshot output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect register address calculation for 64-bit registers
- [Low] Format string type mismatch for size_t
--

--- Patch [3]: [PATCH v2 3/7] drm/xe/guc: Print register addresses in capture snapshot output ---
commit baf33cc78a6f2c33ba405aaad2eb334575f207ed
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/guc: Print register addresses in capture snapshot output
    
This commit modifies the GuC capture snapshot output to include register
offsets alongside their names for both 32-bit and 64-bit registers.

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 82df19b304e17..324f25af3b0d0 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -1774,7 +1774,9 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
>  			XE_WARN_ON(!low32_ready);
>  			low32_ready = false;
>  
> -			drm_printf(p, "\t%s: 0x%016llx\n", reg_desc->regname, value_qw);
> +			drm_printf(p, "\t%s (0x%lx): 0x%016llx\n", reg_desc->regname,
> +				   reg_desc->reg.addr - sizeof(u32),
> +				   value_qw);

[Severity: Medium]
Does this correctly calculate the base address for all 64-bit registers?

In the Intel Xe engine MMIO layout, the upper and lower 32-bit registers are
not always exactly 4 bytes apart.

For example, RING_ACTHD (lower 32-bit) is located at offset 0x74, while
RING_ACTHD_UDW (upper 32-bit) is at 0x5c. RING_BBADDR is at 0x140 and
RING_BBADDR_UDW is at 0x168.

By subtracting 4 from 0x5c, the output would incorrectly print 0x58 for ACTHD,
which is actually the address of RING_HWSTAM.

[Severity: Low]
Will this format string trigger a compiler warning on 32-bit platforms?

The expression reg_desc->reg.addr - sizeof(u32) subtracts a size_t from a u32,
promoting the result to size_t. The format string uses %lx, which expects an
unsigned long.

On 32-bit platforms (where the driver can be compile-tested), size_t is
typically unsigned int. Passing an unsigned int to %lx triggers a -Wformat
compiler warning, which could break builds using -Werror.

Should this use %zx for size_t, or explicitly cast the result to u32 and
use %x?

>  			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824184356.1960708-1-naresh.kumar.g@intel.com?part=3