Re: [PATCH v6 6/9] drm/xe/sysctrl: Replace FIELD_GET() with REG_FIELD_GET()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incomplete migration to REG_FIELD_PREP() leaves packing logic inconsistent.

Pre-existing issues:
- [High] Hardware mailbox state machine lockup on oversized firmware response.
- [High] Endianness corruption on Big Endian architectures due to memcpy followed by writel.
--

commit 44f13e369dbf9b94796ecb4d99fe19420e2a9d18
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/sysctrl: Replace FIELD_GET() with REG_FIELD_GET()

Use REG_FIELD_GET() when extracting fields from SysCtrl and
application headers. This better reflects that the masks
represent register-style bitfields and improves consistency.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index 50bbdab072de6..3aa868f60b386 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> @@ -24,16 +24,16 @@ struct xe_sysctrl_mailbox_msg_hdr {
>  } __packed;
>  
>  #define XE_SYSCTRL_HDR_GROUP_ID(hdr) \
> -	FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
> +	REG_FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))

[Severity: Low]
Does this migration leave the packing logic inconsistent?

In sysctrl_prepare_command(), the code continues to use FIELD_PREP() despite
the getters being migrated to REG_FIELD_GET():

	hdr->data = cpu_to_le32(FIELD_PREP(SYSCTRL_HDR_GROUP_ID_MASK, group_id) |
				     FIELD_PREP(SYSCTRL_HDR_COMMAND_MASK, command));

Should this be updated to REG_FIELD_PREP() for a complete migration?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> index f0d5e3d7f5e31..5a3dd14e3a110 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> @@ -15,13 +15,13 @@ struct xe_sysctrl;
>  struct xe_sysctrl_mailbox_command;
>  
>  #define XE_SYSCTRL_APP_HDR_GROUP_ID(hdr) \
> -	FIELD_GET(APP_HDR_GROUP_ID_MASK, (hdr)->data)
> +	REG_FIELD_GET(APP_HDR_GROUP_ID_MASK, (hdr)->data)

[Severity: Low]
Does this incomplete migration leave xe_sysctrl_create_command() inconsistent?

While the getters in the header are updated to REG_FIELD_GET(), the packing
logic in xe_sysctrl_create_command() continues to use FIELD_PREP() for these
same masks:

	header.data = FIELD_PREP(APP_HDR_GROUP_ID_MASK, group_id) |
		      FIELD_PREP(APP_HDR_COMMAND_MASK, cmd_id);

Should this be updated to REG_FIELD_PREP() as well?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does sysctrl_receive_frames() leave the
hardware mailbox state machine wedged if the firmware response is oversized?

Looking at sysctrl_receive_frames() in drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:

	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;
	}

	hdr = (struct xe_sysctrl_mailbox_msg_hdr *)buffer;

	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)) {

If the firmware sends a response larger than expected, the loop exits when
remain reaches 0 even though the firmware indicates more frames are pending
(done == false).

The function then evaluates the header and returns without draining the
remaining frames from the hardware.

Does this leave the firmware stuck waiting for the host to clear
SYSCTRL_MB_CTRL_RUN_BUSY_OUT, preventing any subsequent mailbox communication?

[Severity: High]
This is a pre-existing issue, but can this cause endianness corruption on
Big Endian architectures?

In sysctrl_write_frame() in drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:

	memcpy(val, frame, len);

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

A little-endian byte array is copied into a u32 array with memcpy() and then
written with an endian-swapping MMIO accessor.

On Big Endian platforms, the u32 value constructed via memcpy evaluates with
its bytes logically reversed.

When this is passed to xe_mmio_write32() (which performs a cpu_to_le32()
conversion), the value is byte-swapped again, writing the bytes in reversed
order.

The same issue exists in reverse in sysctrl_read_frame():

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

	memcpy(frame, val, len);

Does this endian-swapped MMIO read being copied directly into a byte buffer
via memcpy corrupt the response byte order on Big Endian?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831104456.1770605-11-mallesh.koujalagi@intel.com?part=6