Re: [PATCH v8 1/3] drm/xe/sysctrl: Add sysctrl debugfs infrastructure and loopback test interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/sysctrl: mailbox hang due to truncated loopback response
--

commit ab3eed44be77711e6bb0819ac8607e94eabdf3d2
Author: Anoop Vijay <anoop.c.vijay@intel.com>
Subject: drm/xe/sysctrl: Add sysctrl debugfs infrastructure and loopback test interface

This commit adds debugfs support for exercising the System Controller mailbox
interface from userspace. It introduces the "sc/loopback" entry to send
arbitrary byte payloads to the Core group's inverted-loopback command.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> new file mode 100644
> index 0000000000000..0620a010198c5
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c

[ ... ]

> +static ssize_t xe_sysctrl_loopback_write(struct file *file, const char __user *ubuf,
> +					 size_t len, loff_t *offp)
> +{

[ ... ]

> +	if (input_len == 0) {
> +		xe_err(xe, "sysctrl: no loopback payload given\n");
> +		return -EINVAL;
> +	}
> +
> +	xe_sysctrl_create_command(&cmd, entry->group, entry->command,
> +				  input, input_len, entry->response_buf, input_len);

[Severity: Medium]
Could passing input_len as the response_len cause a permanent mailbox hang?

If the firmware pads the loopback response such that it occupies more
16-byte frames than expected (e.g., input_len is 12, but the firmware
responds with a 16-byte payload requiring an additional frame), the
low-level mailbox routines might prematurely terminate the read loop.

In sysctrl_receive_frames(), if the hardware sends more frames than
input_len covers, the loop could exit early, leaving unread frames in
the mailbox with SYSCTRL_MB_CTRL_RUN_BUSY_OUT set. This would
permanently stall the state machine and cause all subsequent
communication to fail with -EBUSY.

Would it be safer to pass XE_SYSCTRL_MB_MAX_DATA_SIZE as the response
buffer length to safely drain any firmware response?

> +
> +	scoped_guard(mutex, &entry->lock) {
> +		guard(xe_pm_runtime)(xe);
> +		entry->status = xe_sysctrl_send_command(entry->sc, &cmd, &out_len);
> +		entry->response_len = entry->status ? 0 : out_len;
> +		status = entry->status;
> +	}
> +
> +	return status ? status : len;
> +}

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