Claude review: drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Claude Code Review Bot <claude-review@example.com>]

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 =
    drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;
struct drm_plane_state *old_state =
    drm_atomic_get_old_plane_state(state, plane);
```

This interleaves statements (`if (!new_state) return;`) with declarations (`struct drm_plane_state *old_state = ...`). While C99/C11 technically allows this, the Linux kernel coding style strongly discourages mixing declarations 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 = ...`.

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 =
    drm_atomic_get_new_plane_state(state, plane);
struct drm_plane_state *old_state =
    drm_atomic_get_old_plane_state(state, plane);
struct drm_crtc *crtc;
struct vmw_private *dev_priv;
...

if (!new_state)
    return;

crtc = new_state->crtc ? new_state->crtc :
    (old_state ? old_state->crtc : NULL);
if (!crtc)
    return;

dev_priv = 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()` should always return a valid state for such a plane. If `new_state` is truly NULL 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 root cause — 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 over 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 against that case specifically seems correct.

**Minor: ternary replacement**

The original `?:` (GNU extension) was replaced with a fully spelled-out ternary `? new_state->crtc : ...`. This is fine and arguably more portable, though the GNU extension is widely used in the kernel.

**Summary**: The intent is good but the implementation needs rework to comply with kernel coding style (no declarations after statements). The commit message should also clarify the actual root cause more precisely — specifically which pointer was NULL when the crash occurred.

---
Generated by Claude Code Patch Reviewer