Claude review: drm/gud: fix NULL crtc dereference on display disable

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

Patch Review

The new crtc enable/disable callbacks are straightforward:

> +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
> +			   struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct gud_device *gdrm = to_gud_device(drm);
> +	int idx;
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> +	gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 1);
> +
> +	drm_dev_exit(idx);
> +}

The `SET_STATE_COMMIT` is sent unconditionally here. In the old code, it was only sent when `mode_changed || connectors_changed`. For the initial enable and mode change paths, `SET_STATE_CHECK` will have been sent during `gud_plane_atomic_check`, so the commit is expected. For DPMS-on (where `active_changed` triggers `atomic_enable` but there's no mode change), `SET_STATE_COMMIT` is sent without a preceding `SET_STATE_CHECK`. This is documented as intentional in the v5 changelog. Worth confirming with the GUD protocol maintainer (Noralf Trønnes) that the device firmware handles a `SET_STATE_COMMIT` without a prior `SET_STATE_CHECK` gracefully -- though re-committing the existing state should be a no-op in practice.

> +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> +			   struct drm_atomic_state *state)
> +{
> +	...
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 0);
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> +	...
> +}

The disable ordering (display off first, then controller off) is correct per the GUD protocol. Note that `atomic_disable` is called for DPMS-off transitions too (where `old_state->active` is true), not just full disables. The old code only disabled the controller when `!crtc->state->enable`, leaving it enabled during DPMS-off. The new code always disables both, which is a behavioral change but a reasonable one -- `atomic_enable` will re-enable both on DPMS-on.

The switch to `drm_atomic_helper_commit_tail_rpm`:

> +static const struct drm_mode_config_helper_funcs gud_mode_config_helpers = {
> +	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};

This changes the commit ordering from the default (disables → planes → enables) to (disables → enables → planes with `ACTIVE_ONLY`). This ensures that `gud_crtc_atomic_enable` runs before `gud_plane_atomic_update`, matching the GUD protocol requirement that the controller and display must be enabled before framebuffer data is sent. The `DRM_PLANE_COMMIT_ACTIVE_ONLY` flag also means planes on inactive CRTCs are skipped, so DPMS-off doesn't trigger an unnecessary framebuffer flush. Good.

The core fix in `gud_plane_atomic_update`:

> -	if (!drm_dev_enter(drm, &idx))
> +	if (!crtc || !drm_dev_enter(drm, &idx))
>  		return;

This is the actual fix for the NULL dereference. When the plane is being disabled (detached from CRTC), `new_state->crtc` is NULL. The DRM atomic core enforces that `crtc` and `fb` are either both set or both NULL, so if `crtc` passes the NULL check, `fb` is guaranteed non-NULL and the subsequent `drm_gem_fb_begin_cpu_access(fb, ...)` is safe.

The removal of the inline enable/disable logic:

> -	if (!old_state->fb)
> -		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> -
> -	if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
> -		gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> -
> -	if (crtc->state->active_changed)
> -		gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
> -
> -	if (!fb)
> -		goto ctrl_disable;

> -ctrl_disable:
> -	if (!crtc->state->enable)
> -		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);

This simplification is clean. All the crtc state management is now properly delegated to the `atomic_enable`/`atomic_disable` callbacks, and `atomic_update` only handles framebuffer flushing.

No issues found that would block this patch. The behavioral changes around `SET_STATE_COMMIT` on DPMS-on are the only thing worth a comment from the GUD protocol maintainer, but they should be benign.

---
Generated by Claude Code Patch Reviewer