Claude review: drm/xe/drm_ras: Make counter allocation drm managed

Claude review: drm/xe/drm_ras: Make counter allocation drm managed

[Claude Code Review Bot]

Patch Review

**Verdict: Correct fix, minor observation.**

Switching from `kcalloc` to `drmm_kcalloc` ensures the counter array is freed automatically during drm device release, which eliminates the problem where `cleanup_node_param()` wasn't called if counter allocation itself failed in a subsequent iteration — the previous iteration's counter would be leaked.

```c
counter = drmm_kcalloc(&xe->drm, DRM_XE_RAS_ERR_COMP_MAX, sizeof(*counter), GFP_KERNEL);
```

The corresponding removal of `kfree(ras->info[severity])` from `cleanup_node_param()` is correct since drmm now owns the lifetime:

```c
-	kfree(ras->info[severity]);
-	ras->info[severity] = NULL;
```

**Minor observation**: When `allocate_and_copy_counters()` fails, `ras->info[severity]` is left holding an `ERR_PTR` value (set in `assign_node_params()` line 120-121). While this isn't a functional bug since the init path returns an error and nothing will dereference it, it would be cleaner to set `ras->info[severity] = NULL` after the `IS_ERR` check in `assign_node_params()`. Not a blocking issue.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/4] DRM RAS Fixes

[Raag Jadav]

Detailed description in commit message.

v2: Use goto (Riana)
    Retain info as NULL on failure (Riana)

Raag Jadav (4):
  drm/ras: Cancel and free message on get counter failure
  drm/xe/drm_ras: Make counter allocation drm managed
  drm/xe/drm_ras: Add per node cleanup action
  drm/xe/hw_error: Use HW_ERR prefix in log

 drivers/gpu/drm/drm_ras.c        | 19 +++++-----
 drivers/gpu/drm/xe/xe_drm_ras.c  | 62 +++++++++++++-------------------
 drivers/gpu/drm/xe/xe_hw_error.c | 12 +++----
 3 files changed, 41 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] drm/ras: Cancel and free message on get counter failure

[Raag Jadav]

doit_reply_value() directly returns on get counter failure, which results
in stale sk_buff and genetlink header that aren't cleaned up. Fix it and
while at it, consolidate error handling using goto.

Fixes: c36218dc49f5 ("drm/ras: Introduce the DRM RAS infrastructure over generic netlink")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
v2: Use goto (Riana)
---
 drivers/gpu/drm/drm_ras.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
index d6eab29a1394..467a169026fc 100644
--- a/drivers/gpu/drm/drm_ras.c
+++ b/drivers/gpu/drm/drm_ras.c
@@ -201,25 +201,28 @@ static int doit_reply_value(struct genl_info *info, u32 node_id,
 
 	hdr = genlmsg_iput(msg, info);
 	if (!hdr) {
-		nlmsg_free(msg);
-		return -EMSGSIZE;
+		ret = -EMSGSIZE;
+		goto free_msg;
 	}
 
 	ret = get_node_error_counter(node_id, error_id,
 				     &error_name, &value);
 	if (ret)
-		return ret;
+		goto cancel_msg;
 
 	ret = msg_reply_value(msg, error_id, error_name, value);
-	if (ret) {
-		genlmsg_cancel(msg, hdr);
-		nlmsg_free(msg);
-		return ret;
-	}
+	if (ret)
+		goto cancel_msg;
 
 	genlmsg_end(msg, hdr);
 
 	return genlmsg_reply(msg, info);
+
+cancel_msg:
+	genlmsg_cancel(msg, hdr);
+free_msg:
+	nlmsg_free(msg);
+	return ret;
 }
 
 /**
-- 
2.43.0

[PATCH v2 2/4] drm/xe/drm_ras: Make counter allocation drm managed

[Raag Jadav]

cleanup_node_param() is not registered in case of counter allocation
failure, which results in stale memory of previous node that isn't
cleaned up on unwind. Fix this using drm managed allocation, which is
guaranteed to be cleaned up on unwind.

Fixes: b40db12b542f ("drm/xe/xe_drm_ras: Add support for XE DRM RAS")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
v2: Retain info as NULL on failure (Riana)
---
 drivers/gpu/drm/xe/xe_drm_ras.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
index c21c8b428de6..c1d5ac198a7c 100644
--- a/drivers/gpu/drm/xe/xe_drm_ras.c
+++ b/drivers/gpu/drm/xe/xe_drm_ras.c
@@ -80,7 +80,7 @@ static struct xe_drm_ras_counter *allocate_and_copy_counters(struct xe_device *x
 	struct xe_drm_ras_counter *counter;
 	int i;
 
-	counter = kcalloc(DRM_XE_RAS_ERR_COMP_MAX, sizeof(*counter), GFP_KERNEL);
+	counter = drmm_kcalloc(&xe->drm, DRM_XE_RAS_ERR_COMP_MAX, sizeof(*counter), GFP_KERNEL);
 	if (!counter)
 		return ERR_PTR(-ENOMEM);
 
@@ -135,7 +135,6 @@ static void cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_err
 {
 	struct drm_ras_node *node = &ras->node[severity];
 
-	kfree(ras->info[severity]);
 	ras->info[severity] = NULL;
 
 	kfree(node->device_name);
-- 
2.43.0

[PATCH v2 3/4] drm/xe/drm_ras: Add per node cleanup action

[Raag Jadav]

cleanup_node_param() is not registered for previous node in case of counter
allocation failure, which results in stale memory of previous node that
isn't cleaned up on unwind. Add per node cleanup action which guarantees
cleanup on unwind and also simplifies the cleanup logic.

Fixes: b40db12b542f ("drm/xe/xe_drm_ras: Add support for XE DRM RAS")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpu/drm/xe/xe_drm_ras.c | 59 +++++++++++++--------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
index c1d5ac198a7c..b901976203fc 100644
--- a/drivers/gpu/drm/xe/xe_drm_ras.c
+++ b/drivers/gpu/drm/xe/xe_drm_ras.c
@@ -131,53 +131,46 @@ static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
 	return 0;
 }
 
-static void cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_error_severity severity)
+static void cleanup_node_param(struct drm_ras_node *node)
 {
-	struct drm_ras_node *node = &ras->node[severity];
-
-	ras->info[severity] = NULL;
-
 	kfree(node->device_name);
 	node->device_name = NULL;
 }
 
+static void cleanup_node(struct drm_device *drm, void *node)
+{
+	drm_ras_node_unregister(node);
+	cleanup_node_param(node);
+}
+
 static int register_nodes(struct xe_device *xe)
 {
 	struct xe_drm_ras *ras = &xe->ras;
-	int i;
+	struct drm_ras_node *node;
+	int i, ret;
 
 	for_each_error_severity(i) {
-		struct drm_ras_node *node = &ras->node[i];
-		int ret;
+		node = &ras->node[i];
 
 		ret = assign_node_params(xe, node, i);
-		if (ret) {
-			cleanup_node_param(ras, i);
-			return ret;
-		}
+		if (ret)
+			break;
 
 		ret = drm_ras_node_register(node);
-		if (ret) {
-			cleanup_node_param(ras, i);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-static void xe_drm_ras_unregister_nodes(struct drm_device *device, void *arg)
-{
-	struct xe_device *xe = arg;
-	struct xe_drm_ras *ras = &xe->ras;
-	int i;
+		if (ret)
+			break;
 
-	for_each_error_severity(i) {
-		struct drm_ras_node *node = &ras->node[i];
+		ret = drmm_add_action_or_reset(&xe->drm, cleanup_node, node);
+		if (ret)
+			break;
+	}
 
-		drm_ras_node_unregister(node);
-		cleanup_node_param(ras, i);
+	if (ret) {
+		cleanup_node_param(node);
+		ras->info[i] = NULL;
 	}
+
+	return ret;
 }
 
 /**
@@ -206,11 +199,5 @@ int xe_drm_ras_init(struct xe_device *xe)
 		return err;
 	}
 
-	err = drmm_add_action_or_reset(&xe->drm, xe_drm_ras_unregister_nodes, xe);
-	if (err) {
-		drm_err(&xe->drm, "Failed to add action for Xe DRM RAS (%pe)\n", ERR_PTR(err));
-		return err;
-	}
-
 	return 0;
 }
-- 
2.43.0

[PATCH v2 4/4] drm/xe/hw_error: Use HW_ERR prefix in log

[Raag Jadav]

Hardware errors should be logged with HW_ERR prefix. Make them
consistent with existing logs.

Fixes: 01aab7e1c9d4 ("drm/xe/xe_hw_error: Add support for PVC SoC errors")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_error.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
index 5135e8e4093f..4b72959b2276 100644
--- a/drivers/gpu/drm/xe/xe_hw_error.c
+++ b/drivers/gpu/drm/xe/xe_hw_error.c
@@ -223,9 +223,9 @@ static void log_hw_error(struct xe_tile *tile, const char *name,
 	struct xe_device *xe = tile_to_xe(tile);
 
 	if (severity == DRM_XE_RAS_ERR_SEV_CORRECTABLE)
-		drm_warn(&xe->drm, "%s %s detected\n", name, severity_str);
+		drm_warn(&xe->drm, HW_ERR "%s %s detected\n", name, severity_str);
 	else
-		drm_err_ratelimited(&xe->drm, "%s %s detected\n", name, severity_str);
+		drm_err_ratelimited(&xe->drm, HW_ERR "%s %s detected\n", name, severity_str);
 }
 
 static void log_gt_err(struct xe_tile *tile, const char *name, int i, u32 err,
@@ -235,10 +235,10 @@ static void log_gt_err(struct xe_tile *tile, const char *name, int i, u32 err,
 	struct xe_device *xe = tile_to_xe(tile);
 
 	if (severity == DRM_XE_RAS_ERR_SEV_CORRECTABLE)
-		drm_warn(&xe->drm, "%s %s detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
+		drm_warn(&xe->drm, HW_ERR "%s %s detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
 			 name, severity_str, i, err);
 	else
-		drm_err_ratelimited(&xe->drm, "%s %s detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
+		drm_err_ratelimited(&xe->drm, HW_ERR "%s %s detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
 				    name, severity_str, i, err);
 }
 
@@ -255,9 +255,9 @@ static void log_soc_error(struct xe_tile *tile, const char * const *reg_info,
 
 	if (strcmp(name, "Undefined")) {
 		if (severity == DRM_XE_RAS_ERR_SEV_CORRECTABLE)
-			drm_warn(&xe->drm, "%s SOC %s detected", name, severity_str);
+			drm_warn(&xe->drm, HW_ERR "%s SOC %s detected", name, severity_str);
 		else
-			drm_err_ratelimited(&xe->drm, "%s SOC %s detected", name, severity_str);
+			drm_err_ratelimited(&xe->drm, HW_ERR "%s SOC %s detected", name, severity_str);
 		atomic_inc(&info[index].counter);
 	}
 }
-- 
2.43.0

Claude review: DRM RAS Fixes

[Claude Code Review Bot]

Overall Series Review

Subject: DRM RAS Fixes
Author: Raag Jadav <raag.jadav@intel.com>
Patches: 5
Reviewed: 2026-05-25T17:57:40.177702

---

This is a clean, well-structured 4-patch series fixing real resource leak bugs in the DRM RAS subsystem and xe driver. The core issue is that error paths in the original code leaked sk_buffs, genetlink headers, and per-node registrations/allocations when partial failures occurred during setup.

Patches 1-3 fix genuine resource leaks and are tagged with appropriate `Fixes:` lines. Patch 4 is a cosmetic log prefix change — the `Fixes:` tag on it is debatable since it's a convention improvement rather than a bug fix.

The series applies cleanly, the patches are logically ordered, and the v2 changes (goto-based cleanup, retaining NULL on failure) are good responses to review feedback. I have a few minor observations below but no blocking issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/ras: Cancel and free message on get counter failure

[Claude Code Review Bot]

Patch Review

**Verdict: Good fix, clean implementation.**

This fixes a real leak: when `get_node_error_counter()` failed, the function returned directly without calling `genlmsg_cancel()` or `nlmsg_free()`, leaking the sk_buff and genetlink header.

The goto structure is correct:

```c
cancel_msg:
	genlmsg_cancel(msg, hdr);
free_msg:
	nlmsg_free(msg);
	return ret;
```

- The `hdr` allocation failure path jumps to `free_msg` directly, correctly skipping `genlmsg_cancel` (since there's no valid header to cancel).
- The `get_node_error_counter` and `msg_reply_value` failure paths jump to `cancel_msg`, which cancels the header then falls through to free the message. `hdr` is guaranteed valid at this point.
- The existing `msg_reply_value` error handling had the same cancel+free sequence inline — consolidating it into the goto is a good cleanup.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/xe/drm_ras: Make counter allocation drm managed

[Claude Code Review Bot]

Patch Review

**Verdict: Correct, minimal change.**

Switching from `kcalloc` to `drmm_kcalloc` for counter allocation:

```c
-	counter = kcalloc(DRM_XE_RAS_ERR_COMP_MAX, sizeof(*counter), GFP_KERNEL);
+	counter = drmm_kcalloc(&xe->drm, DRM_XE_RAS_ERR_COMP_MAX, sizeof(*counter), GFP_KERNEL);
```

And removing the corresponding manual free:

```c
-	kfree(ras->info[severity]);
 	ras->info[severity] = NULL;
```

This ensures counter memory is freed during DRM device teardown regardless of which error path is taken. The original code's `cleanup_node_param()` was the only caller of `kfree(ras->info[severity])`, and that function wasn't always reached on unwind — exactly the bug described in the commit message.

The `ras->info[severity] = NULL` is correctly retained to clear the dangling pointer.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/xe/drm_ras: Add per node cleanup action

[Claude Code Review Bot]

Patch Review

**Verdict: Correct fix for a real leak, two minor observations.**

This is the most substantial patch. The old code registered a single `xe_drm_ras_unregister_nodes` action in `xe_drm_ras_init()` *after* `register_nodes()` succeeded. If `register_nodes()` failed partway through (e.g., node 0 succeeds but node 1 fails), previously registered nodes were never cleaned up because the batch cleanup action was never registered. The new design registers a `drmm_add_action_or_reset` per node immediately after successful registration, guaranteeing cleanup on unwind.

The error path after the loop is correct:

```c
if (ret) {
    cleanup_node_param(node);
    ras->info[i] = NULL;
}
```

This handles the current (failed) iteration's resources. Previously completed iterations are covered by their already-registered drmm actions.

**Minor observation 1:** If `drmm_add_action_or_reset()` fails, it calls `cleanup_node()` itself (the `_or_reset` semantics), which already runs `cleanup_node_param(node)`. The post-loop `cleanup_node_param(node)` is then a redundant second call. This is **safe** because `cleanup_node_param` sets `device_name = NULL` after freeing, so `kfree(NULL)` on the second call is a no-op. Just worth noting — not a bug.

**Minor observation 2:** `ret` is declared uninitialized:

```c
int i, ret;
```

If `DRM_XE_RAS_ERR_SEV_MAX` were ever 0, `ret` would be used uninitialized in `if (ret)`. In practice this can't happen (there are always correctable and uncorrectable severities), but initializing `ret = 0` would silence potential compiler warnings and be more defensive. Very minor.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/xe/hw_error: Use HW_ERR prefix in log

[Claude Code Review Bot]

Patch Review

**Verdict: Correct change, questionable Fixes tag.**

The change itself is straightforward and correct — `HW_ERR` is defined in `include/linux/printk.h` as `"[Hardware Error]: "` and is the standard kernel convention for hardware error reporting. The string concatenation with the format literal is fine:

```c
drm_warn(&xe->drm, HW_ERR "%s %s detected\n", name, severity_str);
```

**On the Fixes tag:** The referenced commit `01aab7e1c9d4` ("drm/xe/xe_hw_error: Add support for PVC SoC errors") simply didn't use the `HW_ERR` prefix. Missing a log prefix isn't a functional bug — this is a convention/consistency improvement. It might be more appropriate without a `Fixes:` tag, or at least should not be backported to stable trees.

**Pre-existing issue (not introduced by this patch):** The `log_soc_error` messages are missing a trailing `\n`:

```c
drm_warn(&xe->drm, HW_ERR "%s SOC %s detected", name, severity_str);
```

Compare with `log_hw_error` and `log_gt_err`, which both have `\n`. This inconsistency predates this patch but could be worth fixing in a follow-up.

---
Generated by Claude Code Patch Reviewer