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/atomic: don't set colorop properties of inactive color pipelines
Date: Tue, 05 May 2026 09:26:01 +1000
Message-ID: <review-patch2-20260501132527.522320-3-mwen@igalia.com>
In-Reply-To: <20260501132527.522320-3-mwen@igalia.com>
References: <20260501132527.522320-1-mwen@igalia.com> <20260501132527.522320-3-mwen@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Purpose:** Rejects userspace attempts to set properties on colorops that =
are not part of the currently active pipeline.

**Review:**

The validation logic is correct =E2=80=94 walk the active pipeline to verif=
y the target colorop belongs to it:

```c
for (active_colorop =3D plane_state->color_pipeline;
     active_colorop;
     active_colorop =3D active_colorop->next) {
    if (active_colorop =3D=3D colorop) {
        colorop_state =3D drm_atomic_get_colorop_state(state, colorop);
```

**Concern =E2=80=94 UAPI behavior change:** This introduces a new `-EINVAL`=
 error for a scenario that previously succeeded silently. If any existing u=
serspace was setting properties on colorops not in the active pipeline (eve=
n if it was a no-op), this will now break it. The commit message should ide=
ally note this is an intentional UAPI tightening and explain why it's safe =
(e.g., no known userspace does this, or the old behavior was always a bug).

**Concern =E2=80=94 `goto err` label placement:** The new `err:` label sits=
 just before `drm_property_change_valid_put`:

```c
+err:
 	drm_property_change_valid_put(prop, ref);
 	return ret;
```

This means when `drm_atomic_get_colorop_state` fails (returns IS_ERR), the =
code does `ret =3D PTR_ERR(colorop_state); goto err;` which correctly calls=
 `drm_property_change_valid_put`. For the "not part of active pipeline" cas=
e, `ret =3D -EINVAL; goto err;` also correctly calls the put function. The =
error handling is correct.

**Minor observation:** Getting the plane state (`drm_atomic_get_plane_state=
`) for every colorop property set has a cost, but it's necessary for the pi=
peline walk and is already cheap if the state is already in the atomic stat=
e (common case).

---
Generated by Claude Code Patch Reviewer