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

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

Pre-existing issues:
- [High] Failing to drain all response frames leaves the sysctrl mailbox in an incomplete state.
- [High] Incorrect endianness conversion when reading/writing sysctrl mailbox frames on big-endian systems.
--

--- Patch [5]: [PATCH v4 5/7] drm/xe/sysctrl: Improve firmware response error logging ---
commit f799e2637d5118be81198e34aa2cb7d8ca6d55fc
Author: Mallesh Koujalagi <[email protected]>

drm/xe/sysctrl: Improve firmware response error logging

This commit updates the sysctrl mailbox driver to use the xe_log_err helper
and includes additional details when a response header mismatch occurs.

[Severity: High]
This isn't a bug introduced by this patch, but does the frame processing
code handle endianness correctly on big-endian systems?

In sysctrl_read_frame():

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

	memcpy(frame, val, len);

And in sysctrl_write_frame():

	memcpy(val, frame, len);

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

Since xe_mmio_read32 and xe_mmio_write32 handle CPU-endian u32 values,
won't using memcpy to move data between a little-endian byte stream and
a CPU-endian u32 array inadvertently byte-swap each 32-bit word on
big-endian architectures, resulting in corrupted headers and payloads?

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index c043b51cacc89..9d7915c9d4ae8 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,

[Severity: High]
This is a pre-existing issue, but what happens if the firmware sends more
frames than expected?

Earlier in sysctrl_receive_frames(), the loop reads frames based on remain:

	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 caller provides a data_out_len that is smaller than the firmware's
actual response size, the driver will stop reading when remain drops to 0
while done is still false.

Will this return success while leaving the hardware waiting for the host to
read and acknowledge the remaining frames, potentially causing subsequent
sysctrl communications to fail with mailbox busy timeouts?

>  	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=0x%02lx cmd=0x%02lx is_resp=%lu, expected group=0x%02lx cmd=0x%02lx\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));
>  		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: 0x%02lx\n",
> +			   XE_SYSCTRL_HDR_RESULT(hdr));
>  		return -EIO;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5