Re: [PATCH v5 6/7] drm/xe/sysctrl: Log group and command ID on mailbox failure

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Undeclared macro xe_log_err and missing xe_log.h cause a build failure.
- [Low] Missing include for REG_FIELD_GET in header file.

Pre-existing issues:
- [High] Incorrect endianness conversion in mailbox frame read and write functions on big-endian architectures.
--

--- Patch [6]: [PATCH v5 6/7] drm/xe/sysctrl: Log group and command ID on mailbox failure ---
commit 1b3f6de8bafa26d44086972155e85f862cc56e97
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/sysctrl: Log group and command ID on mailbox failure

Include the mailbox group ID and command code in error logs
for command preparation and execution failures.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index 5032d766b3142..f55f72fce9946 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> @@ -373,7 +373,8 @@ int xe_sysctrl_send_command(struct xe_sysctrl *sc,
>  				      cmd->data_in, cmd->data_in_len,
>  				      &mbox_cmd, &cmd_size);
>  	if (ret) {
> -		xe_err(xe, "sysctrl: Failed to prepare command: %pe\n", ERR_PTR(ret));
> +		xe_log_err(xe, SYSCTRL, ret, "Failed to prepare command %#x.%#x\n",
> +			   group_id, command_code);

[Severity: Low]
Does this cause a build failure? I couldn't find the definition for the
xe_log_err macro or the xe_log.h header in the tree.

>  		return ret;
>  	}
>  

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> index fb434cc165b2a..df7dfe3b39e24 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]
Is a #include <drm/intel/reg_bits.h> missing here? Any file including this
header might fail to build if it doesn't already include the definition
for REG_FIELD_GET.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the endianness conversion
in the mailbox frame read and write functions work correctly on big-endian
architectures?

In sysctrl_write_frame(), the byte stream is copied into a host-endian u32
array and then written to the little-endian MMIO register:

	memcpy(val, frame, len);

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

Similarly, in sysctrl_read_frame(), the register is read as little-endian
into a host-endian u32 array and then copied to a byte stream:

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

	memcpy(frame, val, len);

This might cause the bytes to be reversed again by xe_mmio_write32() and
xe_mmio_read32() on big-endian systems, potentially leading to firmware
communication failures.

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