Re: [PATCH v4 1/7] drm/xe/devcoredump: Capture GT fuse registers in devcoredump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] xe: NULL pointer dereference of q when triggering devcoredump without exec queue
- [Medium] xe: premature release of forcewake reference for other_gt due to scoping
--

commit 2590b539053b321d9d5162da600031b95e05357f
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/devcoredump: Capture GT fuse registers in devcoredump

This commit captures fuse-related GT register state in the Xe devcoredump
for postmortem debugging. It adds generic register snapshot storage and
helpers to capture, print, and free MMIO register dumps, recording primary
and media GT fuse and RPM register states.

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index ab681b148869f..1dbee725f55a1 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
[ ... ]
> @@ -345,8 +534,34 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  	/* keep going if fw fails as we still want to save the memory and SW data */
>  	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>  
> +	other_gt = q->gt->tile->media_gt == q->gt ? q->gt->tile->primary_gt
> +					    : q->gt->tile->media_gt;

[Severity: High]
Does this code trigger a NULL pointer dereference if q is NULL?

For hangs not tied to an exec queue (e.g., TLB invalidation timeouts), the
q parameter to __xe_devcoredump() and devcoredump_snapshot() is explicitly
documented as potentially being NULL. 

Unconditionally dereferencing q here to calculate other_gt and later 
passing q->gt to xe_dbg_gt_snapshot_capture() would cause a kernel panic 
during the crash dumping process.

Should this use the gt parameter passed directly to the function instead
of q->gt?

> +	if (other_gt) {
> +		CLASS(xe_force_wake, fw_ref_other)(gt_to_fw(other_gt),
> +						   XE_FORCEWAKE_ALL);
> +		if (!xe_force_wake_ref_has_domain(fw_ref_other.domains, XE_FORCEWAKE_ALL))
> +			other_gt = NULL;
> +	}

[Severity: Medium]
Are we dropping the forcewake reference for other_gt prematurely here?

The CLASS(xe_force_wake, fw_ref_other) macro uses __cleanup() to
automatically release the forcewake lock when the variable goes out of scope.
Since it is declared inside the if (other_gt) { ... } block, its scope
ends at the closing brace of the if block, immediately running the
destructor (xe_force_wake_put).

Consequently, when xe_dbg_gt_snapshot_capture() and xe_mmio_read32() are
later called to capture registers for other_gt, the device power domain
has already lost its active forcewake reference. Accessing GT MMIO registers
without holding forcewake can lead to hardware timeouts or read failures.

Could the forcewake lock scope be extended to cover the register capture
functions?

> +
>  	cookie = dma_fence_begin_signalling();
>  
> +	ss->gt_fuse      = xe_dbg_gt_snapshot_capture(q->gt,
> +						      other_gt,
> +						      XE_DBG_REGSET_FUSE,
> +						      XE_GT_TYPE_MAIN);

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