Re: [PATCH v4 3/4] drm/xe/sysctrl: Add RAS error injection debugfs interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing PM runtime resume before accessing hardware in debugfs open and show callbacks causes system hangs.
- [Low] Jumping over a `guard()` declaration using `goto` causes compilation errors and violates cleanup subsystem rules.
- [Low] Concurrent writes to the debugfs file will race on `entry->status`, causing the `write` syscall to return the result of another thread's command.
--

commit 69962a1ffd2b865557dbedee272e0256b239b328
Author: Anoop Vijay <anoop.c.vijay@intel.com>
drm/xe/sysctrl: Add RAS error injection debugfs interface

This commit adds a debugfs interface for exercising the System Controller's
RAS error injection command. The command is used to validate RAS error
detection and recovery paths, and its access is gated on the diagnostic
firmware being ready.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> index 2144d9d43fba5..258479d605cbe 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
[ ... ]
> @@ -122,6 +123,127 @@ static const struct file_operations xe_sysctrl_loopback_fops = {
> +static ssize_t xe_sysctrl_ras_error_inject_write(struct file *file, const char __user *ubuf,
> +						 size_t len, loff_t *offp)
> +{
[ ... ]
> +	tmp = kbuf;
> +	while ((token = strsep(&tmp, " \t\n")) != NULL) {
> +		if (*token == '\0')
> +			continue;
> +
> +		if (kstrtoul(token, 0, &val))
> +			goto inval;
[ ... ]
> +	xe_sysctrl_create_command(&cmd, entry->group, entry->command,
> +				  &req, sizeof(req), resp_hdr_only,
> +				  sizeof(resp_hdr_only));
> +
> +	guard(xe_pm_runtime)(xe);
> +	entry->status = xe_sysctrl_send_command(entry->sc, &cmd, &out_len);
> +
> +	return entry->status ? entry->status : len;

[Severity: Low]
Can concurrent writes race on the shared entry->status field here?

Because multiple threads can write to the ras_error_inject debugfs file at
the same time, Thread A's status could be overwritten by Thread B before
Thread A returns. This would cause an incorrect syscall return value for one
of the threads. Could this status be stored in a local variable instead of
the shared entry struct?

> +
> +inval:
> +	xe_err(xe, "sysctrl: invalid ras_error_inject token '%s'\n", token);
> +	return -EINVAL;

[Severity: Low]
Does this goto inval branch jump over the guard(xe_pm_runtime)(xe)
initialization?

Jumping into the scope of a variable declared with __attribute__((cleanup))
leaves it uninitialized, causing the cleanup function to execute on garbage
memory when returning. GCC and Clang will typically reject this with a build
error. This also violates the kernel cleanup subsystem guidelines against
mixing goto and guard() in the same function.

> +}
> +
> +static int xe_sysctrl_ras_error_inject_show(struct seq_file *m, void *data)
> +{
> +	struct xe_sysctrl_debugfs_entry *entry = m->private;
> +	struct xe_device *xe = sc_to_xe(entry->sc);
> +
> +	seq_printf(m, "Command: group=0x%02x cmd=0x%02x\n", entry->group, entry->command);
> +	seq_printf(m, "Diag firmware ready: %s\n",
> +		   str_yes_no(xe_sysctrl_is_diag_fw_ready(xe)));

[Severity: High]
Does checking xe_sysctrl_is_diag_fw_ready(xe) here access hardware MMIO
without first resuming the device?

The xe_sysctrl_is_diag_fw_ready() function sends a mailbox command, which
uses guard(xe_pm_runtime_noresume). Without a prior xe_pm_runtime_get_sync()
(which is correctly used in the .write callback via guard(xe_pm_runtime)),
accessing this when the GPU is in D3cold can cause a system hang or abort.

> +	seq_printf(m, "Status: %d (%s)\n", entry->status, entry->status ? "FAILED" : "SUCCESS");
> +
> +	seq_puts(m, "\nUsage:\n");
> +	seq_puts(m, "  echo \"<ras_block_id> <ras_sub_block_id> <err_type> [params]\" > ras_error_inject\n");
> +	seq_puts(m, "  cat ras_error_inject\n");
> +
> +	return 0;
> +}
> +
> +static int xe_sysctrl_ras_error_inject_open(struct inode *inode, struct file *file)
> +{
> +	struct xe_sysctrl_debugfs_entry *entry = inode->i_private;
> +	struct xe_device *xe = sc_to_xe(entry->sc);
> +
> +	if (!xe_sysctrl_is_diag_fw_ready(xe)) {

[Severity: High]
Are we also missing a PM runtime resume here in the open callback?

Similar to the .show callback above, this calls xe_sysctrl_is_diag_fw_ready()
which issues an MMIO command. A user opening this file when the device is
suspended could trigger a hang.

> +		xe_err(xe, "sysctrl: diag firmware not ready, ras_error_inject unavailable\n");
> +		return -ENODEV;
> +	}
> +
> +	return single_open(file, xe_sysctrl_ras_error_inject_show, inode->i_private);
> +}

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