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/xe_drm_ras: Add support for clear-error-counter in XE drm_ras Date: Sun, 12 Apr 2026 11:34:40 +1000 Message-ID: In-Reply-To: <20260409073318.2909379-6-riana.tauro@intel.com> References: <20260409073318.2909379-4-riana.tauro@intel.com> <20260409073318.2909379-6-riana.tauro@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 **Overall**: Straightforward driver-side implementation that closely follow= s the existing query callback pattern. No issues. **`hw_clear_error_counter()`** correctly mirrors `hw_query_error_counter()`: ```c static int hw_clear_error_counter(struct xe_drm_ras_counter *info, u32 erro= r_id) { if (!info || !info[error_id].name) return -ENOENT; atomic_set(&info[error_id].counter, 0); return 0; } ``` The bounds safety is guaranteed by the core framework's range check (agains= t `error_counter_range.first`/`.last`) before the callback is invoked, so t= he array access `info[error_id]` is safe. The `!info[error_id].name` check = handles non-contiguous IDs gracefully. **`atomic_set` for clearing** is appropriate =E2=80=94 it matches the `atom= ic_read` in the query path and any `atomic_inc` in the error recording path= . There is an inherent TOCTOU between clearing and the next read (a new err= or could arrive), but that's fundamental to the operation and not a bug. **Code structure observation**: The `clear_uncorrectable_error_counter()` a= nd `clear_correctable_error_counter()` functions are near-identical, differ= ing only in the severity index used to select `ras->info[severity]`. The sa= me duplication exists in the query callbacks. This is fine for two severiti= es, but if more are added, a single parameterized function with a severity = argument stored in the node's `priv` (or a wrapper struct) would be cleaner= . Not a blocking concern for this series. **`assign_node_params()` modification** is clean =E2=80=94 adding braces ar= ound the existing if/else and inserting the callback assignment alongside t= he query callback: ```c if (severity =3D=3D DRM_XE_RAS_ERR_SEV_CORRECTABLE) { node->query_error_counter =3D query_correctable_error_counter; node->clear_error_counter =3D clear_correctable_error_counter; } else { node->query_error_counter =3D query_uncorrectable_error_counter; node->clear_error_counter =3D clear_uncorrectable_error_counter; } ``` No issues with this patch. --- Generated by Claude Code Patch Reviewer