Re: [PATCH v4 6/7] drm/xe: capture L3 node status registers in devcoredump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: NULL pointer dereference of q in devcoredump_snapshot()
--

commit 6161ac10e80fa8c54c67e54542cd9c6bcb0ef507
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
drm/xe: capture L3 node status registers in devcoredump

This commit adds L3_NODE_IDLE and L3STAT_LBCF_GFX registers and captures
them in the devcoredump snapshot. It also extends the register descriptor
to support reading MCR registers.

> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index a2a3437ec2f62..db7ccee12006a 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
[ ... ]
> @@ -639,13 +663,15 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  						       other_gt,
>  						       XE_DBG_REGSET_GAM_PF,
>  						       XE_GT_TYPE_MAIN);
> -
> +	ss->ns = xe_dbg_gt_snapshot_capture(q->gt,
> +					    other_gt,
> +					    XE_DBG_REGSET_L3_NODE,
> +					    XE_GT_TYPE_MAIN);

[Severity: High]
If q is NULL, will accessing q->gt cause a NULL pointer dereference here?

When a devcoredump is triggered for a GT-level hang not tied to an exec queue
(such as a TLB invalidation timeout via xe_devcoredump_gt()), q can be NULL,
which is why the code immediately below checks if (q).

Should this capture use the gt parameter that is passed into
devcoredump_snapshot() instead of q->gt, or should it be moved inside the
if (q) block?

>  	if (q) {
>  		ss->ge = xe_guc_exec_queue_snapshot_capture(q);
>  		if (job)
>  			ss->job = xe_sched_job_snapshot_capture(job);
>  		ss->vm = xe_vm_snapshot_capture(q->vm);
> -
>  		xe_engine_snapshot_capture_for_queue(q);
>  	}

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