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 14:26:30 +1000 Message-ID: In-Reply-To: <20260512002752.2826685-2-maaz.mombasawala@broadcom.com> References: <20260512002752.2826685-1-maaz.mombasawala@broadcom.com> <20260512002752.2826685-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 **C99 mixed declarations issue.** The patch adds early-return guards for nu= ll `new_state` and `old_state`, which is correct defensively, but introduce= s a variable declaration after a statement, which is problematic for kernel= coding style: ```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); ``` The `struct drm_crtc *crtc` declaration appears after the `if (!new_state) = return;` statement. While C99/C11 allows this, mixing declarations and stat= ements like this after control flow is poor style. `crtc` should be declare= d at the top of the function alongside the other variables, and assigned la= ter. **Questionable whether `new_state` can actually be NULL.** In the atomic co= mmit path, `drm_atomic_get_new_plane_state` should always return a valid st= ate for a plane that is part of the atomic update. If this function is bein= g called, the plane was part of the commit. If the null dereference in the = IGT test is real, it might indicate a deeper issue worth investigating rath= er than just papering over with null checks. That said, defensive programmi= ng here is not unreasonable as a crash fix. **The `old_state` null handling is good.** Guarding `old_state->crtc` with = `old_state ? old_state->crtc : NULL` is the right fix for the originally re= ported null dereference =E2=80=94 on first enable there may be no old state= , and the `?:` operator unconditionally dereferences `old_state`. --- Generated by Claude Code Patch Reviewer