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: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
Date: Wed, 11 Feb 2026 16:31:48 +1000
Message-ID: <review-patch7-20260210-atomic-v9-7-525c88fd2402@intel.com>
In-Reply-To: <20260210-atomic-v9-7-525c88fd2402@intel.com>
References: <20260210-atomic-v9-0-525c88fd2402@intel.com> <20260210-atomic-v9-7-525c88fd2402@intel.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Issues:**

```c
+	case DRM_CAP_ATOMIC_ERROR_REPORTING:
+		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) && 1;
+		break;
```
- **Redundant && 1**: `drm_core_check_feature` already returns bool/int. The `&& 1` is unnecessary.
- **Incomplete**: This cap says "I support error reporting" but doesn't tell userspace:
  - What error codes are supported
  - What fields in the error struct are valid
  - Which drivers actually implement it (only i915 in this series)

```c
+/**
+ * DRM_CAP_ATOMIC_ERROR_REPORTING
+ *
+ * If set to 1, the driver supports reporting of failure codes on error in
+ * atomic ioctl().
+ */
+#define DRM_CAP_ATOMIC_ERROR_REPORTING	0x16
```
- **Vague Documentation**: Doesn't explain:
  - How to use the feature (set `reserved` field)
  - What the error struct contains
  - Version/compatibility information

**Summary of Major Issues:**

1. Memory leak in patch 2 (kvasprintf never freed)
2. Resource leak in patch 5 (early return bypasses cleanup)
3. ABI breakage by repurposing `reserved` field
4. Missing input validation (userspace pointer not checked)
5. Inconsistent error handling patterns
6. Typos in documentation
7. Incomplete implementation (Intel only)
8. Performance regression (allocate state for invalid requests)

---
Generated by Claude Code Patch Reviewer