Re: [PATCH v3] drm/xe/debugfs: Expose per-engine idle status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Debugfs output misinterprets fw_req status by ignoring validity mask bits in MSG_IDLE registers.

Pre-existing issues:
- [High] Concurrent execution of `hw_engines` debugfs read corrupts the global device coredump state `devcoredump->snapshot.matched_node`.
--

commit d11426316eb2739da991f6532a22c6e266b48954
Author: Sk Anirban <sk.anirban@intel.com>

drm/xe/debugfs: Expose per-engine idle status
    
Extend the existing "hw_engines" debugfs to iterate over all hardware
engines and dump their idle state: whether the engine pipe is idle or
busy, whether C6 entry is allowed, and (where available) the forcewake
request status.

> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index 361a70234d1f1..27ee20b03d4ae 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
[ ... ]
> @@ -124,9 +156,27 @@ static int hw_engines(struct xe_gt *gt, struct drm_printer *p)
>  	if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL))
>  		return -ETIMEDOUT;
>  
> -	for_each_hw_engine(hwe, gt, id)
> +	for_each_hw_engine(hwe, gt, id) {
> +		struct xe_reg reg = msg_idle_reg[id].reg;
> +		u32 val;
> +
>  		xe_hw_engine_print(hwe, p);

[Severity: High]
This isn't a bug introduced by this patch, but can this debugfs read corrupt
the global device coredump state?

When hw_engines() calls xe_hw_engine_print(), it captures an engine
snapshot via xe_engine_manual_capture(). This unconditionally assigns
devcoredump->snapshot.matched_node = new globally without taking the
&devcoredump->lock.

Afterward, it calls xe_hw_engine_snapshot_free(), which explicitly frees
devcoredump->snapshot.matched_node and nullifies it. If a legitimate GPU
hang triggers xe_devcoredump() concurrently, could its matched_node be
overwritten or freed out from under it?

> +		val = xe_mmio_read32(&gt->mmio, reg);
> +
> +		if (msg_idle_reg[id].has_fw)
> +			drm_printf(p, "\tpipe %s, C6 %s, fw_req 0x%02x\n",
> +				   val & MSG_IDLE_INDICATION ? "idle" : "busy",
> +				   val & MSG_IDLE_C6_ALLOWED ? "allowed" : "not allowed",
> +				   REG_FIELD_GET(MSG_IDLE_FW_STATUS, val));

[Severity: Medium]
Does this field extraction read stale data by ignoring the validity mask?

The MSG_IDLE registers extract the forcewake status bits [13:9] using
REG_FIELD_GET(MSG_IDLE_FW_STATUS, val). However, the hardware design uses
bits [29:25] (val >> 16) as a validity mask for these status bits.

Without validating the status against its corresponding mask bit, could the
output reflect stale or invalid data left over in the register?

> +		else
> +			drm_printf(p, "\tpipe %s, C6 %s\n",
> +				   val & MSG_IDLE_INDICATION ? "idle" : "busy",
> +				   val & MSG_IDLE_C6_ALLOWED ? "allowed" : "not allowed");
> +
> +		drm_puts(p, "\n");
> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908060103.999079-2-sk.anirban@intel.com?part=1