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: Sat, 16 May 2026 10:17:48 +1000 Message-ID: In-Reply-To: <20260514224819.3631763-2-maaz.mombasawala@broadcom.com> References: <20260514224819.3631763-1-maaz.mombasawala@broadcom.com> <20260514224819.3631763-2-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 issue =E2=80=94 declaration after statement:** ```c + new_state =3D drm_atomic_get_new_plane_state(state, plane); + if (!new_state) + return; + old_state =3D drm_atomic_get_old_plane_state(state, plane); + struct drm_crtc *crtc =3D new_state->crtc ? new_state->crtc : + (old_state ? old_state->crtc : NULL); + if (!crtc) + return; + struct vmw_private *dev_priv =3D vmw_priv(plane->dev); ``` The `struct drm_crtc *crtc` and `struct vmw_private *dev_priv` declarations= are interspersed with code. While C99/GNU extensions allow this, Linux ker= nel style strongly prefers all declarations at the top of the block. Move `= crtc` and `dev_priv` declarations to the top (alongside `new_state`, `old_s= tate`) and assign them after the NULL checks. Also there's a double space (= `=3D new_state`) on the crtc assignment line. **Questionable NULL check on new_state:** In the atomic commit path, `drm_a= tomic_get_new_plane_state()` should always return a valid state for a plane= that is part of the commit =E2=80=94 this function is called from `atomic_= update`, which only runs for planes included in the commit. If `new_state` = is NULL here, something has gone seriously wrong at a higher layer. A `WARN= _ON(!new_state)` plus return might be more appropriate to flag the unexpect= ed condition rather than silently returning. **Logic correctness:** The ternary for `crtc` is correct =E2=80=94 it safel= y handles `old_state` being NULL. The original code `new_state->crtc ?: old= _state->crtc` would crash if `new_state->crtc` was NULL and `old_state` was= also NULL. --- Generated by Claude Code Patch Reviewer