Claude review: drm/atomic: reject colorop update from inactive color pipeline

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

Patch Review

**Purpose:** Adds `drm_atomic_colorop_check()` to validate that any colorop being modified in an atomic commit belongs to an active (or transitioning) pipeline, called from `drm_atomic_check_only()`.

**Issue 1 (Significant) — Direct access to `plane->state->color_pipeline` without the atomic state:**

```c
/* No changes in the plane state. Check current-committed plane state */
if (!new_plane_state) {
    for (colorop = plane->state->color_pipeline; colorop; colorop = colorop->next)
        if (colorop == new_colorop_state->colorop)
            return 0;
    return -EINVAL;
}
```

Accessing `plane->state` directly (the currently committed hardware state) is problematic. In the DRM atomic framework, `plane->state` is the committed state pointer and is generally only safe to dereference when holding the appropriate locks. Within `drm_atomic_check_only()`, we're in an atomic check context and the plane's lock may not be held if the plane wasn't added to this atomic commit. The `drm_atomic_get_new_plane_state()` returning NULL means this plane wasn't part of the commit at all — but the colorop somehow is, which is an unusual state.

Consider whether this case can actually occur given the call flow. If userspace sets a colorop property, `drm_atomic_get_colorop_state()` should have been called, which calls `drm_atomic_get_plane_state()` on `colorop->plane` (looking at the standard implementation). If that's the case, the plane would always be in the atomic state when a colorop is, making this `!new_plane_state` branch dead code. If it can't be reached, a `WARN_ON` return would be more appropriate than silently walking `plane->state`.

If this branch *can* be reached, then `plane->state` access needs careful consideration regarding locking.

**Issue 2 — Coding style:**

```c
if (WARN_ON(!old_plane_state)) return -EINVAL;
```

The kernel coding style requires the return statement on a separate line:

```c
if (WARN_ON(!old_plane_state))
    return -EINVAL;
```

**Issue 3 — Placement of the colorop check in `drm_atomic_check_only`:**

```c
+	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
+		ret = drm_atomic_colorop_check(new_colorop_state);
+		if (ret) {
+			drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pipeline.\n",
+				       colorop->base.id, colorop->type);
+			return ret;
+		}
+	}
```

The check is placed *before* the plane checks (`for_each_oldnew_plane_in_state`). This is intentional per the commit message ("performing this check later in drm_atomic_check_only() to remove the ordering dependency that would exist if done at the time of colorop property setting"). The placement before plane checks seems reasonable — it's a precondition check.

**Issue 4 — `drm_dbg_atomic` format string:**

```c
drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pipeline.\n",
               colorop->base.id, colorop->type);
```

Logging `colorop->type` (an enum) as `%d` works but is unusual. Other DRM debug messages typically print `[OBJECT:id:name]`. Since colorops don't have a name field, this is acceptable, but using the type enum integer may not be very informative for debugging. Consider whether there's a type-to-string helper.

**Issue 5 — Should the old pipeline check allow modifications on deactivation?**

The check allows modifying colorops that belong to the old pipeline (the one being deactivated):

```c
/* Check if the colorop was active in the old plane state */
for (colorop = old_plane_state->color_pipeline; colorop; colorop = colorop->next)
    if (colorop == new_colorop_state->colorop)
        return 0;
```

This means during pipeline deactivation (old has pipeline, new is NULL), userspace can still modify colorops of the old pipeline in the same commit. The cover letter says this is intentional ("when activating or deactivating its pipeline in the same commit"). This seems correct — userspace might want to set up colorop values while deactivating, so they're ready for future reactivation (relying on persistence from Patch 2).

**Overall for Patch 3:** The main concern is the `plane->state` direct access and whether it can actually be reached. The coding style issue with the single-line `if (WARN_ON(...)) return` should be fixed.

---
Generated by Claude Code Patch Reviewer