Re: [PATCH v4 2/4] 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] Concurrent reads and writes to the loopback debugfs entry cause data races and expose inconsistent state.
--

commit b7da09a9e8ead758b95d7f60c29e7f7b90c922ce
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 which sends an
arbitrary byte payload 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..2144d9d43fba5
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> @@ -0,0 +1,157 @@
[ ... ]
> +static ssize_t xe_sysctrl_loopback_write(struct file *file, const char __user *ubuf,
> +					 size_t len, loff_t *offp)
> +{
[ ... ]
> +	xe_sysctrl_create_command(&cmd, entry->group, entry->command,
> +				  input, input_len, entry->response_buf, input_len);
> +
> +	guard(xe_pm_runtime)(xe);
> +	entry->status = xe_sysctrl_send_command(entry->sc, &cmd, &out_len);
> +	entry->response_len = entry->status ? 0 : out_len;

[Severity: Medium]
Does this code leave the shared debugfs entry fields vulnerable to data races
if multiple userspace processes write to the loopback file concurrently?

It appears that entry->status, entry->response_len, and entry->response_buf
are modified here directly without any synchronization protecting the entry
structure itself, as sc->cmd_lock inside xe_sysctrl_send_command() only
serializes the hardware mailbox sequence.

> +
> +	return entry->status ? entry->status : len;
> +}
> +
> +static int xe_sysctrl_loopback_show(struct seq_file *m, void *data)
> +{
> +	struct xe_sysctrl_debugfs_entry *entry = m->private;
> +	size_t i;
> +
> +	seq_printf(m, "Command: group=0x%02x cmd=0x%02x\n", entry->group, entry->command);
> +	seq_printf(m, "Status: %d (%s)\n", entry->status, entry->status ? "FAILED" : "SUCCESS");
> +	seq_printf(m, "Response: %zu bytes\n", entry->response_len);
> +
> +	if (entry->response_len) {
> +		seq_puts(m, "Response data:\n");
> +		for (i = 0; i < entry->response_len; i++) {
> +			if (i && (i % 16) == 0)
> +				seq_putc(m, '\n');
> +			seq_printf(m, "%02x ", entry->response_buf[i]);

[Severity: Medium]
Can a concurrent write to the loopback file cause torn reads or inconsistent
state to be returned here? 

Since the entry fields are read locklessly, it seems possible to observe a new
buffer's contents alongside an old buffer's length, or a mismatched status,
returning garbage diagnostic output to userspace.

> +		}
> +		seq_putc(m, '\n');
> +	}
> +
> +	return 0;
> +}
[ ... ]

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