Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Hardware/firmware error payload structures use non-portable C bitfields and host-native integer types instead of explicitly sized little-endian types. - [Low] The newly added function `handle_soc_internal_errors` is marked as `__maybe_unused`, has no callers, and contains incomplete PUNIT error handling. --
--- Patch [1]: [PATCH v10 1/4] drm/xe/xe_ras: Do not Review, compilation only. --- commit 8b53e475734bf697402ee545f94eb93bf4f28f2b Author: Riana Tauro <[email protected]> drm/xe/xe_ras: Do not Review, compilation only. DO NOT REVIEW. COMPILATION ONLY This patch is from https://patchwork.freedesktop.org/series/160482/ Added only for Compilation. > diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c > index 74d5016d9ffe7..3d39bb95e94d6 100644 > --- a/drivers/gpu/drm/xe/xe_ras.c > +++ b/drivers/gpu/drm/xe/xe_ras.c [ ... ] > @@ -193,6 +194,48 @@ static void ras_usp_aer_init(struct xe_device *xe) > dev_dbg(&usp->dev, "Uncorrectable Internal Errors downgraded and > unmasked\n"); > } > > +static __maybe_unused enum xe_ras_recovery_action > +handle_soc_internal_errors(struct xe_device *xe, > + struct xe_ras_error_array *arr) [Severity: Low] Does this introduce dead, incomplete code into the tree? This function is marked __maybe_unused, has no callers, and contains an incomplete PUNIT error handling TODO below. > +{ > + struct xe_ras_soc_error *info = (void *)arr->details; > + struct xe_ras_soc_error_source *source = &info->source; [Severity: Medium] Can this cause misinterpretation of firmware error messages on big-endian architectures? This code reads firmware values via the bitfield struct without endianness conversion. The hardware/firmware error payload structures are defined using non-portable C bitfields and host-native integer types instead of explicitly sized little-endian types. Using C bitfields inside __packed structs for hardware/firmware interface definitions relies on compiler-specific bit-packing rules. Explicitly sized little-endian types (e.g., __le32) and bitwise macros (like FIELD_GET()) should probably be used to parse these hardware structures. > + struct xe_ras_error_class *counter = &arr->counter; > + > + if (source->csc) { > + struct xe_ras_csc_error *csc_error = (void *)info->details; [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_ras_types.h > b/drivers/gpu/drm/xe/xe_ras_types.h > index 6688e11f57a86..3c165096ea20c 100644 > --- a/drivers/gpu/drm/xe/xe_ras_types.h > +++ b/drivers/gpu/drm/xe/xe_ras_types.h [ ... ] > +/** > + * struct xe_ras_soc_error_source - Source of SoC error > + */ > +struct xe_ras_soc_error_source { > + /** @csc: CSC */ > + u32 csc:1; > + /** @ieh: IEH (Integrated Error Handler) */ > + u32 ieh:1; > + /** @reserved: Reserved for future use */ > + u32 reserved:30; > +} __packed; [Severity: Medium] Will this bitfield layout work correctly across different architectures? This struct defines hardware/firmware error source flags using brittle C bitfields. As noted above, relying on compiler-specific bit-packing inside __packed structs for hardware interfaces can lead to issues. [ ... ] > +/** > + * struct xe_ras_ieh_error - SoC IEH (Integrated Error Handler) error details > + */ > +struct xe_ras_ieh_error { > + /** @ieh_instance: IEH instance */ > + u32 ieh_instance:2; > + /** @reserved: Reserved for future use */ > + u32 reserved:30; > + /** @global_error_status: Global error status */ > + u32 global_error_status; > + /** @local_error_status: Local error status */ > + u32 local_error_status; > + /** @gerr_mask: Global error mask */ > + u32 gerr_mask; > + /** @info: Additional information */ > + u32 info[10]; > +} __packed; [Severity: Medium] Could this bitfield layout be problematic for similar reasons? This struct defines hardware/firmware IEH error layout using C bitfields and host-native types instead of explicitly sized little-endian types. > #endif -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
