Claude review: drm/colorop: Fix blob property reference tracking in state lifecycle

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Correctness — the core fix is right:**

The existing `drm_colorop_atomic_destroy_state()` at line 488-492 of the current source does just `kfree(state)` without releasing the blob reference obtained via `drm_property_blob_get()` in `__drm_atomic_helper_colorop_duplicate_state()`. This is a genuine refcount leak. Adding the `drm_property_blob_put(state->data)` call before `kfree()` is the correct fix.

**`drm_property_blob_put()` with NULL is safe:**

```c
+static void __drm_atomic_helper_colorop_destroy_state(struct drm_colorop_state *state)
+{
+	drm_property_blob_put(state->data);
+}
```

`drm_property_blob_put()` handles NULL gracefully (it's a `drm_property_blob_put` → `kref_put` path guarded by a NULL check), so no explicit NULL check is needed here. This matches the crtc pattern.

**`drm_colorop_reset()` fix:**

```c
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
```

This is correct. The old code leaked the blob on reset. However, note a subtle ordering difference from the `drm_crtc` pattern: in `drm_atomic_helper_crtc_reset()`, the new state is allocated *first*, and then the old state is destroyed, so a failed allocation still leaves the old state intact. Here, the old state is destroyed first (and `colorop->state` is not yet cleared — `__drm_colorop_reset` assigns it later), then the new state is allocated. If `kzalloc_obj` fails, `__drm_colorop_reset` is not called, so `colorop->state` still points to freed memory (a use-after-free / dangling pointer).

Looking at the existing code before this patch, this dangling-pointer-on-alloc-failure bug **already existed** — the old `kfree(colorop->state)` had the same problem. So this patch doesn't introduce a regression, but it also doesn't fix it. This is a pre-existing issue and out of scope for this fix, but worth noting for a follow-up.

**`drm_colorop_cleanup()` simplification:**

```c
-	if (colorop->state && colorop->state->data) {
-		drm_property_blob_put(colorop->state->data);
-		colorop->state->data = NULL;
-	}
-
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
```

Good consolidation. The old code had an ad-hoc blob cleanup that was duplicating what `drm_colorop_atomic_destroy_state()` should be doing. Using the common path is cleaner and less error-prone.

**`__drm_atomic_helper_colorop_destroy_state` is static but documented as if exported:**

The helper is declared `static`, which means drivers subclassing the colorop state cannot use it. The docstring says "This is useful for drivers that subclass the colorop state," but since it's `static`, no external driver can call it. The analogous `__drm_atomic_helper_crtc_destroy_state()` is exported. If subclassing is anticipated, this should be non-static and exported. If not, the docstring should be adjusted. This is a minor nit — making it `static` is fine for now since there are no out-of-tree subclass users, and it can be exported later if needed.

**Overall: The patch is correct and fixes real reference leaks. Recommend applying.**

---
Generated by Claude Code Patch Reviewer