Re: [PATCH v5 5/7] drm/xe/sysctrl: Improve firmware response error logging

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Build failure due to missing `xe_log.h` and undeclared `xe_log_err` function.

Pre-existing issues:
- [High] Endianness bug in MMIO data transfer causes hardware state machine corruption on big-endian hosts.
- [High] Mailbox response reader truncates oversized responses without draining the mailbox, leading to hardware state machine corruption.
--

--- Patch [5]: [PATCH v5 5/7] drm/xe/sysctrl: Improve firmware response error logging ---
commit aca0eba486b26ff6a9ae8a95c07e0dfce167281d
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/sysctrl: Improve firmware response error logging

Use xe_log_err() helper and include additional details when a
response header mismatch occurs.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index f8b518dcafa24..5032d766b3142 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
[ ... ]
> @@ -237,13 +237,17 @@ static int sysctrl_receive_frames(struct xe_sysctrl *sc,
>  	if (!XE_SYSCTRL_HDR_IS_RESPONSE(hdr) ||
>  	    XE_SYSCTRL_HDR_GROUP_ID(hdr) != XE_SYSCTRL_HDR_GROUP_ID(req) ||
>  	    XE_SYSCTRL_HDR_COMMAND(hdr) != XE_SYSCTRL_HDR_COMMAND(req)) {
> -		xe_err(xe, "sysctrl: Response header mismatch\n");
> +		xe_log_err(xe, SYSCTRL, -EPROTO,
> +			   "Response header mismatch: got group=%#x cmd=%#x is_resp=%u, expected group=%#x cmd=%#x\n",
> +			   XE_SYSCTRL_HDR_GROUP_ID(hdr), XE_SYSCTRL_HDR_COMMAND(hdr),
> +			   XE_SYSCTRL_HDR_IS_RESPONSE(hdr),
> +			   XE_SYSCTRL_HDR_GROUP_ID(req), XE_SYSCTRL_HDR_COMMAND(req));

[Severity: Low]
Does this code fail to compile? It looks like xe_log.h is missing from the
source tree, meaning xe_log_err and SYSCTRL might be undeclared identifiers.

>  		return -EPROTO;
>  	}
>  
>  	if (XE_SYSCTRL_HDR_RESULT(hdr) != 0) {
> -		xe_err(xe, "sysctrl: Firmware error: 0x%02lx\n",
> -		       XE_SYSCTRL_HDR_RESULT(hdr));
> +		xe_log_err(xe, SYSCTRL, -EIO, "Firmware error: %#04x\n",
> +			   XE_SYSCTRL_HDR_RESULT(hdr));
>  		return -EIO;
>  	}

[Severity: High]
This is a pre-existing issue, but does this loop truncate oversized responses
without draining the mailbox?

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 firmware sends more frames than expected by data_out_len, remain
becomes 0 before done is set to true. The loop exits silently and returns
success, leaving the unread frames in the mailbox. 

Will subsequent mailbox commands write to the control register while it is
still busy, potentially corrupting the hardware state machine?

[Severity: High]
This is a pre-existing issue, but is there an endianness mismatch in the MMIO
data transfers here?

drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:sysctrl_write_frame() {
	...
	memcpy(val, frame, len);

	for (i = 0; i < dw; i++)
		xe_mmio_write32(sc->mmio, regs[i], val[i]);
	...
}

And similarly:

drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:sysctrl_read_frame() {
	...
	for (i = 0; i < dw; i++)
		val[i] = xe_mmio_read32(sc->mmio, regs[i]);

	memcpy(frame, val, len);
	...
}

When little-endian bytes are copied into native CPU-endian u32 arrays, won't
this result in an incorrect byte order on big-endian hosts? 

The bytes could be placed in the wrong order inside the CPU-endian u32
values, and when the MMIO accessors byte-swap these values again, the
resulting byte stream on the wire might be reversed compared to the
intended payload.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825110542.1359776-9-mallesh.koujalagi@intel.com?part=5