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/bridge: it66121: Add bridge-private atomic state Date: Sat, 16 May 2026 16:11:55 +1000 Message-ID: In-Reply-To: <20260510191459.90769-3-javierm@redhat.com> References: <20260510191459.90769-1-javierm@redhat.com> <20260510191459.90769-3-javierm@redhat.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 **Status: One issue to address.** The state subclassing is correctly implemented =E2=80=94 `kzalloc_obj(*stat= e)` properly allocates a `struct it66121_bridge_state`, and both `it66121_b= ridge_atomic_duplicate_state` and `it66121_bridge_atomic_reset` correctly c= all the `__drm_atomic_helper_bridge_*` helpers on `&state->base`. **Issue: `atomic_destroy_state` still uses `drm_atomic_helper_bridge_destro= y_state`.** ```c static const struct drm_bridge_funcs it66121_bridge_funcs =3D { .atomic_duplicate_state =3D it66121_bridge_atomic_duplicate_state, .atomic_destroy_state =3D drm_atomic_helper_bridge_destroy_state, .atomic_reset =3D it66121_bridge_atomic_reset, ``` The documentation on `drm_atomic_helper_bridge_destroy_state()` explicitly = says: *"This helper is meant to be used as a bridge atomic_destroy_state ho= ok for bridges that don't subclass the bridge state."* In practice this works because `base` is the first member of `it66121_bridg= e_state`, so `kfree(&state->base)` is equivalent to `kfree(state)`. And the= re is precedent (e.g. `dw-dp.c` does the same). However, it's fragile =E2= =80=94 if someone later adds a member that requires cleanup (e.g., a sub-al= location), the generic destroy won't handle it. I'd recommend adding a triv= ial custom destroy for correctness: ```c static void it66121_bridge_atomic_destroy_state(struct drm_bridge *bridge, struct drm_bridge_state *st= ate) { struct it66121_bridge_state *it66121_state =3D to_it66121_bridge_state(state); kfree(it66121_state); } ``` This is minor but would be the correct pattern given the subclassing. --- Generated by Claude Code Patch Reviewer