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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 25, 2026 at 05:53:16PM +0000, 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.

Although sashiko tags this as low, this might break the bisect for most of
us because -werror...
So, please add the maybe unused mark or squash this with the user.

Sometimes separate small patches help reviews, sometimes it is the other way
around. A bigger but complete patch might be easier to review.

And then perhaps only the ABI itself in a separate patch and in a separate
directory like Michal pointed out.

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