Re: [PATCH v4 4/7] drm/xe: dump GAM page fault report registers in devcoredump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NULL pointer dereference of q via q->gt when devcoredump is not tied to an exec queue
- [Medium] Premature release of forcewake for other_gt due to CLASS macro scoping
--

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

drm/xe: dump GAM page fault report registers in devcoredump

This commit adds the PAGE_FLTRPTQ_HEAD_GAMCTRL and PAGE_FLTRPTQ_TAIL_GAMCTRL
registers to the devcoredump snapshot. This state is intended to be available
during cleanup for post-mortem debugging.

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index f4a22e46eda78..a2a3437ec2f62 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
[ ... ]
> @@ -602,6 +635,10 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  
>  	ss->guc.log = xe_guc_log_snapshot_capture(&guc->log, true);
>  	ss->guc.ct = xe_guc_ct_snapshot_capture(&guc->ct);
> +	ss->gam_pf_report = xe_dbg_gt_snapshot_capture(q->gt,
> +						       other_gt,
> +						       XE_DBG_REGSET_GAM_PF,
> +						       XE_GT_TYPE_MAIN);

[Severity: High]
Can q be NULL here?

If a hardware hang is not tied to an exec queue (for example, a TLB
invalidation timeout triggering via xe_devcoredump_gt()), q is passed as NULL.

Will unconditionally dereferencing q->gt here cause a NULL pointer dereference
and prevent the devcoredump from being created? Should the function use the gt
parameter directly instead of q->gt?

[Severity: Medium]
Is the forcewake for other_gt still held when this register capture occurs?

Earlier in devcoredump_snapshot(), the forcewake for other_gt is acquired
using the CLASS(xe_force_wake, ...) macro inside an if (other_gt) { ... }
block. Because CLASS utilizes the C __attribute__((cleanup)) mechanism, the
forcewake is released as soon as that block ends.

Does this mean the new gam_pf_report capture runs after the forcewake has
already been dropped, potentially leading to hardware timeouts or reading
garbage data?

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