From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/xe/ras: Add support for error threshold Date: Fri, 05 Jun 2026 06:10:07 +1000 Message-ID: In-Reply-To: <20260604184849.1011985-4-raag.jadav@intel.com> References: <20260604184849.1011985-1-raag.jadav@intel.com> <20260604184849.1011985-4-raag.jadav@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This adds `xe_ras_get_threshold()` and `xe_ras_set_threshold()` using syste= m controller mailbox commands. **Issues:** 1. **`xe_ras_get_threshold` doesn't check `response.status`**: Same issue a= s `get_counter` in patch 2. The `xe_ras_get_threshold_response` struct does= n't have an explicit `status` field, so either the firmware doesn't report = errors in get responses (which seems unlikely), or the status is embedded i= n the response somehow but not checked. Meanwhile `xe_ras_set_threshold` pr= operly checks `response.status`. This inconsistency is suspicious =E2=80=94= if get-threshold can fail (invalid component, hardware error), how does th= e firmware signal that? 2. **Inconsistent `guard(xe_pm_runtime)` placement**: In `xe_ras_get_thresh= old()`, the guard is placed just before the `xe_sysctrl_send_command` call: ```c guard(xe_pm_runtime)(xe); ret =3D xe_sysctrl_send_command(&xe->sc, &command, &len); ``` But the subsequent code that reads from `response` (which was populated by = the command) runs after the guard scope. Actually, since `guard()` holds un= til scope exit, this is fine =E2=80=94 but it's worth noting that the runti= me PM is held across the entire rest of the function including the debug pr= int. 3. **`xe_ras_get_threshold_response` field layout concern**: ```c struct xe_ras_get_threshold_response { struct xe_ras_error_class counter; u32 threshold; u32 reserved[4]; } __packed; ``` vs `xe_ras_set_threshold_response`: ```c struct xe_ras_set_threshold_response { struct xe_ras_error_class counter; u32 reserved; u32 threshold; u32 status; u32 reserved1[2]; } __packed; ``` In the get response, `threshold` is right after `counter`. In the set respo= nse, there's a `reserved` field between `counter` and `threshold`. These ar= e different mailbox commands so different layouts are expected, but the inc= onsistency should be verified against the firmware interface spec. --- Generated by Claude Code Patch Reviewer