From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
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: Thu, 12 Mar 2026 07:06:13 +1000
Message-ID: <review-patch2-20260311102913.3387468-8-riana.tauro@intel.com>
In-Reply-To: <20260311102913.3387468-8-riana.tauro@intel.com>
References: <20260311102913.3387468-6-riana.tauro@intel.com> <20260311102913.3387468-8-riana.tauro@intel.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

Straightforward implementation that mirrors the existing query pattern.

**Atomicity concern in `hw_clear_error_counter`:**

```c
atomic_set(&info[error_id].counter, 0);
```

Using `atomic_set` to clear is fine for a simple reset-to-zero. However, if the counter can be incremented concurrently (e.g., from an IRQ handler via `atomic_inc`), the clear could race with an increment and the increment could be silently lost. This may be acceptable for RAS counters where "best effort" clearing is fine, but it's worth documenting the intent. If exact clearing semantics are needed, `atomic_xchg` and acknowledging the return value might be more appropriate.

**Code duplication between correctable/uncorrectable:**

```c
static int clear_uncorrectable_error_counter(struct drm_ras_node *node, u32 error_id)
{
    struct xe_device *xe = node->priv;
    struct xe_drm_ras *ras = &xe->ras;
    struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERR_SEV_UNCORRECTABLE];
    return hw_clear_error_counter(info, error_id);
}

static int clear_correctable_error_counter(struct drm_ras_node *node, u32 error_id)
{
    ...
    struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERR_SEV_CORRECTABLE];
    return hw_clear_error_counter(info, error_id);
}
```

This mirrors the existing duplication in the query callbacks so it's consistent, but the pattern of having two nearly identical functions differing only by a severity constant is a bit unfortunate. Not a blocker since it matches the existing code style.

---
Generated by Claude Code Patch Reviewer