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] Incorrect forcewake request status extraction due to missing validity mask.

Pre-existing issues:
- [High] Lockless linked list corruption and data race on singleton `devcoredump->snapshot.matched_node`.
--

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

drm/xe/debugfs: Expose per-engine idle status

This commit extends the hw_engines debugfs to iterate over hardware engines
and dump their idle state, including whether the engine pipe is idle or busy,
if C6 entry is allowed, and 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 bb09e70ee44c0..405cc076d8431 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
[ ... ]
> @@ -126,9 +158,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 is a pre-existing issue, but does exposing this in the hw_engines
debugfs risk triggering a lockless list corruption or data race?

Multiple concurrent reads to the hw_engines debugfs file will call into
xe_hw_engine_print(), which can eventually call xe_engine_manual_capture():

xe_hw_engine_print()
  ...
  -> xe_engine_manual_capture()
       -> guc_capture_get_prealloc_node()
          list_del() // on cachelist locklessly

xe_engine_manual_capture() also locklessly mutates outlist and overwrites
the shared devcoredump->snapshot.matched_node without synchronization:

    guc_capture_add_node_to_outlist(guc->capture, new);
    devcoredump->snapshot.matched_node = new;

Could this lead to a kernel panic via linked list corruption or
use-after-free if userspace reads this debugfs file concurrently?

>  
> +		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_REQ, val));

[Severity: Medium]
Does this correctly extract the forcewake request status without applying
the required hardware validity mask?

The hardware uses the upper 16 bits of the register as validity indicators
for the lower bits. Ignoring them and directly reading the MSG_IDLE_FW_REQ
field (bits 13:9) might result in reading potentially garbage or stale state.

Should this check the upper 16-bit validity mask (bits 29:25) before
extracting the lower bits for the debugfs output?

> +		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/20260910165855.1124574-2-sk.anirban@intel.com?part=1