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: verisilicon: subclass drm_plane_state
Date: Wed, 11 Feb 2026 16:24:25 +1000
Message-ID: <review-patch2-20260210141300.749013-3-zhengxingda@iscas.ac.cn>
In-Reply-To: <20260210141300.749013-3-zhengxingda@iscas.ac.cn>
References: <20260210141300.749013-1-zhengxingda@iscas.ac.cn> <20260210141300.749013-3-zhengxingda@iscas.ac.cn>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Purpose:** Introduce custom plane state structure to hold driver-specific state.

**Code Analysis:**

```c
+struct vs_plane_state {
+	struct drm_plane_state base;
+};
```

Empty struct is fine as a placeholder - patch 3 will add the format field.

```c
+struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct vs_plane_state *vs_state;
+
+	if (WARN_ON(!plane->state))
+		return NULL;
+
+	vs_state = kmemdup(plane->state, sizeof(*vs_state), GFP_KERNEL);
```

**Issue 4 - Type confusion in kmemdup:**
This is copying `plane->state` (a `drm_plane_state*`) but using `sizeof(*vs_state)` (size of `vs_plane_state`). While this works because we're early in the transition, it's fragile. The correct pattern is:
```c
vs_state = kmemdup(state_to_vs_plane_state(plane->state), sizeof(*vs_state), GFP_KERNEL);
```
Or more idiomatically:
```c
struct vs_plane_state *old_vs_state = state_to_vs_plane_state(plane->state);
vs_state = kmemdup(old_vs_state, sizeof(*vs_state), GFP_KERNEL);
```

```c
+void vs_plane_reset(struct drm_plane *plane)
+{
+	struct vs_plane_state *vs_state;
+
+	if (plane->state)
+		__drm_atomic_helper_plane_destroy_state(plane->state);
+
+	kfree(plane->state);
```

**CRITICAL Issue 5 - Use-after-free:**
This code calls `__drm_atomic_helper_plane_destroy_state(plane->state)` which may access the memory, then immediately frees it with `kfree(plane->state)`. If `__drm_atomic_helper_plane_destroy_state` holds any references or the memory is accessed after the kfree, this is a use-after-free bug.

Looking at DRM core, `__drm_atomic_helper_plane_destroy_state` decrements refcounts on the framebuffer and fence, but doesn't access the state structure itself after that. So this is technically safe BUT violates common patterns.

The standard pattern in DRM drivers is:
```c
if (plane->state) {
	vs_plane_destroy_state(plane, plane->state);
	plane->state = NULL;
}
```

Or if you want to inline it:
```c
if (plane->state) {
	__drm_atomic_helper_plane_destroy_state(plane->state);
	kfree(state_to_vs_plane_state(plane->state));
	plane->state = NULL;
}
```

**Issue 6 - Missing plane->state = NULL:**
After freeing plane->state, you should set it to NULL before allocating the new state. Otherwise if kzalloc fails, plane->state points to freed memory.

**Issue 7 - Memory leak on allocation failure:**
```c
+	vs_state = kzalloc(sizeof(*vs_state), GFP_KERNEL);
+	if (!vs_state)
+		return;
```
If allocation fails, we've just freed plane->state but don't restore it. The plane is left in an invalid state. This is probably acceptable for reset during init, but should be documented or handled more gracefully.

**Verdict:** Core approach is correct, but implementation has memory management issues that need fixing.

---

---
Generated by Claude Code Patch Reviewer