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 XE DRM RAS
Date: Tue, 03 Mar 2026 14:32:50 +1000
Message-ID: <review-patch2-20260228080858.3063532-9-riana.tauro@intel.com>
In-Reply-To: <20260228080858.3063532-9-riana.tauro@intel.com>
References: <20260228080858.3063532-7-riana.tauro@intel.com> <20260228080858.3063532-9-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

**Partial registration leak in `register_nodes()`:**
If the second severity node fails to register, only that node is cleaned up. The first (already-registered) node is left dangling:

```c
+	for_each_error_severity(i) {
+		...
+		ret = drm_ras_node_register(node);
+		if (ret) {
+			cleanup_node_param(ras, i);
+			return ret;   /* previously registered node[0] leaked */
+		}
+	}
```

The error path needs to unregister and clean up all previously-succeeded nodes.

**`kasprintf` for PCI BDF name:**
```c
+	device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
+				pci_domain_nr(pdev->bus), pdev->bus->number,
+				PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
```
Consider using `pci_name(pdev)` which returns the same BDF string and avoids the separate allocation. If a separate copy is needed, `kstrdup(pci_name(pdev), GFP_KERNEL)` is simpler.

**`for_each_error_severity` macro in header without necessary include:**
```c
+#define for_each_error_severity(i)	\
+	for (i = 0; i < DRM_XE_RAS_ERR_SEV_MAX; i++)
```
`DRM_XE_RAS_ERR_SEV_MAX` comes from `xe_drm.h` but `xe_drm_ras.h` doesn't include it. Callers must remember to include `xe_drm.h` first, which is fragile.

**uAPI string-table macros (`DRM_XE_RAS_ERROR_SEVERITY_NAMES`, `DRM_XE_RAS_ERROR_COMPONENT_NAMES`):**
Defining string table macros in a uAPI header is unusual. These are kernel-internal lookup tables and should not be part of the userspace ABI. The enum values and their numeric IDs are sufficient for userspace; string names are already provided through the netlink attributes.

---

---
Generated by Claude Code Patch Reviewer