Re: [PATCH v4 1/4] drm/xe/sysctrl: Add System Controller get application status

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If the firmware responds with more data than expected, `sysctrl_receive_frames()` exits its read loop early, permanently hanging the mailbox state machine.
--

commit b13ca50e46d24c8dd84b548f2ea15d17ffc919a8
Author: Anoop Vijay <anoop.c.vijay@intel.com>

drm/xe/sysctrl: Add System Controller get application status

Add support for querying System Controller firmware application
status. This allows callers to determine whether a given firmware
application has completed boot and post-boot initialization.

Link: https://lore.kernel.org/intel-xe/20260814095306.2482289-5-anoop.c.vijay@intel.com/
Link: https://lore.kernel.org/intel-xe/20260814095306.2482289-6-anoop.c.vijay@intel.com/

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl.c b/drivers/gpu/drm/xe/xe_sysctrl.c
> index 62ccc9be71b45..5da591ade4e4b 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl.c

[ ... ]

> @@ -125,3 +127,83 @@ void xe_sysctrl_pm_resume(struct xe_device *xe)
>  
>  	xe->soc_remapper.set_sysctrl_region(xe, SYSCTRL_MAILBOX_INDEX);
>  }
> +
> +static enum xe_sysctrl_fw_status
> +xe_sysctrl_check_app_status(struct xe_device *xe, enum xe_sysctrl_app_id app_id)
> +{
> +	struct xe_sysctrl_app_status_req req = {};
> +	struct xe_sysctrl_app_status_resp resp = {};
> +	struct xe_sysctrl_mailbox_command cmd = {};
> +	size_t out_len = 0;
> +	u32 flags;
> +	int ret;
> +
> +	if (!xe->info.has_sysctrl)
> +		return XE_SYSCTRL_FIRMWARE_APP_NOTSUPP;
> +
> +	req.app_id = (u8)app_id;
> +
> +	xe_sysctrl_create_command(&cmd, XE_SYSCTRL_GROUP_CORE, XE_SYSCTRL_CMD_GET_APP_STATUS_BY_ID,
> +				  &req, sizeof(req), &resp, sizeof(resp));
> +
> +	ret = xe_sysctrl_send_command(&xe->sc, &cmd, &out_len);

[Severity: High]
This isn't a bug introduced by this patch, but calling
xe_sysctrl_send_command() here exposes a pre-existing severe logic flaw in
the mailbox protocol handler.

If the firmware responds to this command with more data than expected, the
driver will permanently hang the mailbox state machine.

In drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:sysctrl_receive_frames():

	while (!done && remain) {
		frame_size = min_t(size_t, remain, XE_SYSCTRL_MB_FRAME_SIZE);

		ret = sysctrl_process_frame(sc, out, frame_size, timeout_ms,
					    &done);
		if (ret)
			return ret;

		remain -= frame_size;
		out += frame_size;
	}

If the response payload is larger than the requested data_out_len,
sysctrl_receive_frames() strictly bounds its read loop by remain. When
remain drops to 0, the loop terminates before done is set.

This causes the host to stop reading the remaining frames and stop clearing
SYSCTRL_MB_CTRL_RUN_BUSY_OUT. The firmware is left waiting indefinitely
for the host to acknowledge the next frame, which will cause all subsequent
mailbox commands to time out or corrupt the mailbox state.

Does this need to be fixed in sysctrl_receive_frames() to properly drain
unexpected trailing data instead of abandoning the transaction?

> +	if (ret)
> +		return XE_SYSCTRL_FIRMWARE_COMM_FAILURE;
> +
> +	if (out_len != sizeof(resp)) {
> +		xe_err(xe, "sysctrl: unexpected get app status response length %zu (expected %zu)\n",
> +		       out_len, sizeof(resp));
> +		return XE_SYSCTRL_FIRMWARE_COMM_FAILURE;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903140340.83500-6-anoop.c.vijay@intel.com?part=1