Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Synchronous mailbox commands injected during an active pagination loop disrupt stateful firmware communication. - [Medium] Unvalidated firmware-provided `component` ID is used in the `BIT()` macro, leading to out-of-bounds shifts (Undefined Behavior) and truncation that breaks deduplication. - [Medium] Errors of different severities for the same component are incorrectly collapsed, masking critical hardware errors. --
--- Patch [3]: [PATCH v5 3/3] drm/xe/xe_ras: Report uncorrectable error events to userspace --- commit 51a0403024c186a2a443b450caaf0da16723e651 Author: Riana Tauro <[email protected]> drm/xe/xe_ras: Report uncorrectable error events to userspace When the firmware reports uncorrectable errors, this patch delivers a drm-ras error event to userspace for each affected component. Multiple errors for the same component are collapsed to avoid duplicate events. > diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c > index cf85075d62667..462cbb548f07b 100644 > --- a/drivers/gpu/drm/xe/xe_ras.c > +++ b/drivers/gpu/drm/xe/xe_ras.c > @@ -378,6 +378,7 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct > xe_device *xe) > enum xe_ras_recovery_action final_action; > u32 remaining = XE_SYSCTRL_FLOOD_LIMIT; > struct xe_ras_get_soc_error response; > + u8 sent = 0; [ ... ] > @@ -421,6 +422,12 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct > xe_device *xe) > xe_info(xe, "[RAS]: %s %s detected\n", > comp_to_str(component), > sev_to_str(severity)); > > + /* Send event once per component */ > + if (!(sent & BIT(component))) { > + sent |= BIT(component); [Severity: Medium] Is there a risk of undefined behavior or truncation here if the firmware returns an unexpected component ID? Since sent is a u8, any component value of 8 or greater will cause BIT(component) to be truncated to 0 when bitwise OR'ed with sent. The condition !(sent & BIT(component)) would then constantly evaluate to true, bypassing the duplicate suppression. Also, if component is greater than or equal to BITS_PER_LONG, BIT() will cause an out-of-bounds shift. Should component be validated against a maximum value before using it in the bit shift? [Severity: Medium] Does this deduplication logic unintentionally mask uncorrectable errors if multiple errors for the same component arrive with different severities? If the firmware responds with a correctable error followed by an uncorrectable error for the exact same component, the first error sets the bit in the sent mask. The subsequent uncorrectable error would then be skipped, and userspace would only be notified of the correctable severity. > + ras_send_error_event(xe, severity, component); [Severity: High] Can calling ras_send_error_event() here disrupt the firmware's pagination state? The xe_ras_process_errors() function retrieves errors using a stateful XE_SYSCTRL_CMD_GET_SOC_ERROR command inside a do-while loop based on response.additional_errors. However, ras_send_error_event() synchronously sends a different mailbox command (XE_SYSCTRL_CMD_GET_COUNTER): xe_ras.c:ras_send_error_event() { ... ret = get_counter(xe, &counter, &value); ... } If xe_sysctrl_send_command() releases its mutex between calls, injecting this GET_COUNTER command into the GET_SOC_ERROR pagination stream on the wire might reset or corrupt the firmware's internal cursor. This could lead to dropped errors or an infinite loop reading the same initial chunk. > + } > + > switch (component) { > case XE_RAS_COMP_CORE_COMPUTE: > action = handle_core_compute_errors(arr); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
