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/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update Date: Sun, 22 Mar 2026 03:57:47 +1000 Message-ID: In-Reply-To: <20260320014909.3247540-1-maaz.mombasawala@broadcom.com> References: <20260320014909.3247540-1-maaz.mombasawala@broadcom.com> <20260320014909.3247540-1-maaz.mombasawala@broadcom.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 **Coding style: Declarations after statements (build error / warning)** The patch inserts a `return` statement between variable declarations: ```c struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(state, plane); if (!new_state) return; struct drm_plane_state *old_state =3D drm_atomic_get_old_plane_state(state, plane); ``` This interleaves statements (`if (!new_state) return;`) with declarations (= `struct drm_plane_state *old_state =3D ...`). While C99/C11 technically all= ows this, the Linux kernel coding style strongly discourages mixing declara= tions and statements, and `-Wdeclaration-after-statement` (which is enabled= in kernel builds) will produce a warning. The same issue occurs with the `= if (!crtc) return;` followed by `struct vmw_private *dev_priv =3D ...`. The fix should move all declarations to the top of the function and perform= the null checks afterward. Something like: ```c struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(state, plane); struct drm_plane_state *old_state =3D drm_atomic_get_old_plane_state(state, plane); struct drm_crtc *crtc; struct vmw_private *dev_priv; ... if (!new_state) return; crtc =3D new_state->crtc ? new_state->crtc : (old_state ? old_state->crtc : NULL); if (!crtc) return; dev_priv =3D vmw_priv(plane->dev); ``` **Questionable whether `new_state` can actually be NULL** The `atomic_update` callback is invoked by the atomic commit path only for = planes that are part of the commit. `drm_atomic_get_new_plane_state()` shou= ld always return a valid state for such a plane. If `new_state` is truly NU= LL here, it would indicate a deeper framework bug. The commit message says = this fixes `kms_cursor_legacy@torture-bo` but doesn't explain the actual ro= ot cause =E2=80=94 was the plane being committed without state, or was the = issue specifically with `old_state` being NULL? A more detailed analysis in= the commit message would help reviewers understand if this is papering ove= r a real bug elsewhere. **The `old_state` NULL check is the more plausible fix** The original `old_state->crtc` dereference without a NULL check is the more= likely actual bug. If a cursor plane is being enabled for the first time (= no previous state), `old_state` could legitimately be NULL. Protecting agai= nst that case specifically seems correct. **Minor: ternary replacement** The original `?:` (GNU extension) was replaced with a fully spelled-out ter= nary `? new_state->crtc : ...`. This is fine and arguably more portable, th= ough the GNU extension is widely used in the kernel. **Summary**: The intent is good but the implementation needs rework to comp= ly with kernel coding style (no declarations after statements). The commit = message should also clarify the actual root cause more precisely =E2=80=94 = specifically which pointer was NULL when the crash occurred. --- Generated by Claude Code Patch Reviewer