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: Mon, 25 May 2026 19:32:33 +1000 Message-ID: In-Reply-To: <20260521223756.627311-2-maaz.mombasawala@broadcom.com> References: <20260521223756.627311-1-maaz.mombasawala@broadcom.com> <20260521223756.627311-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 **Purpose:** Fix a null dereference when `old_state` is NULL in `vmw_cursor= _plane_atomic_update`, found by IGT test `kms_cursor_legacy@torture-bo`. **Issues:** 1. **Declaration-after-statement / mixed declarations and code (style):** T= he patch introduces variable declarations after statements, which violates = the kernel coding style (and strict C89 rules still enforced by some kernel= configs): ```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 declarations of `crtc` and `dev_priv` appear after executable statement= s (the `if` guards). While C99/C11 allows this and the kernel has moved to = C11, placing declarations like `struct drm_crtc *crtc` and `struct vmw_priv= ate *dev_priv` after `if` statements with `return` is unusual in kernel cod= e. It would be cleaner to declare `crtc` at the top with `new_state`/`old_s= tate` and assign it after the null checks. 2. **Double space:** Minor nit =E2=80=94 there's a double space before `new= _state->crtc`: ```c struct drm_crtc *crtc =3D new_state->crtc ? new_state->crtc : ``` 3. **Can `new_state` actually be NULL here?** In the atomic commit path, `d= rm_atomic_get_new_plane_state` should always return non-NULL for a plane th= at is in the commit. If the plane is not in the atomic state, it shouldn't = be called at all. The commit message says this fixes `kms_cursor_legacy@tor= ture-bo`, but it would be helpful to understand the exact call path that le= ads to `new_state` being NULL =E2=80=94 this might indicate the real bug is= elsewhere (the plane shouldn't be in an update callback if it's not part o= f the state). 4. **Missing Fixes tag specificity:** The Fixes tag references the refactor= commit but no explanation is given as to why the prior code (which used th= e same `drm_atomic_get_old/new_plane_state` calls) didn't trigger this. A s= entence about the calling context would help reviewers. --- Generated by Claude Code Patch Reviewer