From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/atomic: reject colorop update from inactive color pipeline Date: Wed, 27 May 2026 14:45:25 +1000 Message-ID: In-Reply-To: <20260526142940.504911-4-mwen@igalia.com> References: <20260526142940.504911-1-mwen@igalia.com> <20260526142940.504911-4-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:** 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) =E2=80=94 Direct access to `plane->state->color_pip= eline` without the atomic state:** ```c /* No changes in the plane state. Check current-committed plane state */ if (!new_plane_state) { for (colorop =3D plane->state->color_pipeline; colorop; colorop =3D col= orop->next) if (colorop =3D=3D 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 committe= d state pointer and is generally only safe to dereference when holding the = appropriate locks. Within `drm_atomic_check_only()`, we're in an atomic che= ck context and the plane's lock may not be held if the plane wasn't added t= o this atomic commit. The `drm_atomic_get_new_plane_state()` returning NULL= means this plane wasn't part of the commit at all =E2=80=94 but the coloro= p somehow is, which is an unusual state. Consider whether this case can actually occur given the call flow. If users= pace 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 wo= uld always be in the atomic state when a colorop is, making this `!new_plan= e_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 c= onsideration regarding locking. **Issue 2 =E2=80=94 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 =E2=80=94 Placement of the colorop check in `drm_atomic_check_onl= y`:** ```c + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) { + ret =3D drm_atomic_colorop_check(new_colorop_state); + if (ret) { + drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pip= eline.\n", + colorop->base.id, colorop->type); + return ret; + } + } ``` The check is placed *before* the plane checks (`for_each_oldnew_plane_in_st= ate`). This is intentional per the commit message ("performing this check l= ater in drm_atomic_check_only() to remove the ordering dependency that woul= d exist if done at the time of colorop property setting"). The placement be= fore plane checks seems reasonable =E2=80=94 it's a precondition check. **Issue 4 =E2=80=94 `drm_dbg_atomic` format string:** ```c drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pipelin= e.\n", colorop->base.id, colorop->type); ``` Logging `colorop->type` (an enum) as `%d` works but is unusual. Other DRM d= ebug 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-strin= g helper. **Issue 5 =E2=80=94 Should the old pipeline check allow modifications on de= activation?** The check allows modifying colorops that belong to the old pipeline (the on= e being deactivated): ```c /* Check if the colorop was active in the old plane state */ for (colorop =3D old_plane_state->color_pipeline; colorop; colorop =3D colo= rop->next) if (colorop =3D=3D new_colorop_state->colorop) return 0; ``` This means during pipeline deactivation (old has pipeline, new is NULL), us= erspace can still modify colorops of the old pipeline in the same commit. T= he cover letter says this is intentional ("when activating or deactivating = its pipeline in the same commit"). This seems correct =E2=80=94 userspace m= ight 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 acce= ss 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