Re: [PATCH v2 01/11] drm/xe/xe_ras: Add support to retrieve info queue data for CRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unportable use of C bitfields inside a packed structure for a firmware message interface.
- [Low] The patch introduces a static function `get_info_queue_data` without any callers, causing a `-Wunused-function` compiler warning that breaks the build.
--

commit 82a6804e91047f9e3f922cd878001d1bc3e9d8ed
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/xe_ras: Add support to retrieve info queue data for CRI

This commit adds support to retrieve info queue data. It introduces
various structures for the info queue data request and response, and adds
a function to send the command to the system controller.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index d25d25f775310..6830872354825 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -661,6 +661,40 @@ int xe_ras_clear_counter(struct xe_device *xe, u8 severity, u8 component)
>  	return 0;
>  }
>  
> +static int get_info_queue_data(struct xe_device *xe,
> +			       const struct xe_ras_get_info_queue_data_request *req,
> +			       struct xe_ras_get_info_queue_data_response *out)

[Severity: Low]
Does this function have any callers in this patch?

Adding a static function without using it in the same patch will cause a
-Wunused-function compiler warning, which breaks the build when compiling
with -Werror. To maintain git bisect functionality, every intermediate patch
should compile independently.

Could get_info_queue_data() be introduced in the patch that first uses it,
or temporarily marked with __maybe_unused?

> +{
> +	struct xe_ras_get_info_queue_data_response response = {0};
> +	struct xe_sysctrl_mailbox_command command = {0};
> +	size_t rlen;
> +	int ret;
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> index 99b2466e2062c..d87db9f5174a2 100644
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
[ ... ]
> @@ -95,6 +99,109 @@ struct xe_ras_threshold_crossed {
>  	struct xe_ras_error_class counters[XE_RAS_NUM_COUNTERS];
>  } __packed;
>  
> +/**
> + * struct xe_ras_info_queue_header - Metadata for large info queue data transfers
> + *
> + * Provides chunk metadata for commands that support extended info queue
> + * functionality. Used when the total data exceeds a single mailbox response.
> + */
> +struct xe_ras_info_queue_header {
> +	/** @total_size: Total size of the complete info queue data in bytes */
> +	u32 total_size;
> +	/** @chunk_offset: Offset of this chunk within the total data in bytes */
> +	u32 chunk_offset;
> +	/** @chunk_size: Size of the data in this chunk in bytes */
> +	u32 chunk_size;
> +	/** @sequence_number: Sequence number for this chunk, starts at 0 */
> +	u32 sequence_number;
> +	/** @flags: Info queue control flags (RAS_INFO_QUEUE_FLAG_*) */
> +	u32 flags:8;
> +	/** @compression_type: Compression algorithm used; 0 = none */
> +	u32 compression_type:4;
> +	/** @num_headers: Number of detailed counter headers at start of queue_data */
> +	u32 num_headers:5;
> +	/** @reserved: Reserved for future use */
> +	u32 reserved:15;

[Severity: Medium]
Are these C bitfields safe to use for a firmware message interface?

When the driver communicates with the firmware on a big-endian architecture
or using a compiler with different bitfield ordering rules, the bitfields
in xe_ras_info_queue_header will map to the wrong bits in the underlying
32-bit word.

Using C bitfields inside a __packed struct for a hardware/firmware ABI is
generally avoided because their memory layout is compiler-dependent and
endian-dependent. Would it be safer to use fixed-width types like u32
along with FIELD_GET() and FIELD_PREP() macros?

> +	/** @checksum: CRC32 checksum of this chunk data */
> +	u32 checksum;
> +} __packed;
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825175916.1103841-13-badal.nilawar@intel.com?part=1