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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 25-08-2026 23:23, sashiko-bot@kernel.org wrote:
> 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?
This will be fixed in next rev.
>
>> +{
>> +	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?

Yes, safe with xe kmd, which runs on x86 architectures.

Thanks,
Badal

>
> 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;
>> +
> [ ... ]
>