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/gud: fix NULL crtc dereference on display disable Date: Mon, 23 Feb 2026 05:53:49 +1000 Message-ID: In-Reply-To: <20260222054551.80864-1-me@shenghaoyang.info> References: <20260222054551.80864-1-me@shenghaoyang.info> <20260222054551.80864-1-me@shenghaoyang.info> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 =3D crtc->dev; > + struct gud_device *gdrm =3D 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 wa= s only sent when `mode_changed || connectors_changed`. For the initial enab= le 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 a= s intentional in the v5 changelog. Worth confirming with the GUD protocol m= aintainer (Noralf Tr=C3=B8nnes) that the device firmware handles a `SET_STA= TE_COMMIT` without a prior `SET_STATE_CHECK` gracefully -- though re-commit= ting 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 pe= r the GUD protocol. Note that `atomic_disable` is called for DPMS-off trans= itions 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 bo= th on DPMS-on. The switch to `drm_atomic_helper_commit_tail_rpm`: > +static const struct drm_mode_config_helper_funcs gud_mode_config_helpers= =3D { > + .atomic_commit_tail =3D drm_atomic_helper_commit_tail_rpm, > +}; This changes the commit ordering from the default (disables =E2=86=92 plane= s =E2=86=92 enables) to (disables =E2=86=92 enables =E2=86=92 planes with `= ACTIVE_ONLY`). This ensures that `gud_crtc_atomic_enable` runs before `gud_= plane_atomic_update`, matching the GUD protocol requirement that the contro= ller and display must be enabled before framebuffer data is sent. The `DRM_= PLANE_COMMIT_ACTIVE_ONLY` flag also means planes on inactive CRTCs are skip= ped, 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 di= sabled (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 `crt= c` passes the NULL check, `fb` is guaranteed non-NULL and the subsequent `d= rm_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_u= pdate` 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 G= UD protocol maintainer, but they should be benign. --- Generated by Claude Code Patch Reviewer