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/nouveau/kms: add NULL check for CRTC in nv50_sor_atomic_disable Date: Sun, 12 Apr 2026 10:25:01 +1000 Message-ID: In-Reply-To: <20260409172126.115441-3-marek@czernohous.de> References: <20260409172126.115441-1-marek@czernohous.de> <20260409172126.115441-3-marek@czernohous.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: Defensive fix that prevents a crash, but the explanation is susp= ect and the early-return may skip necessary cleanup.** The change: ```c + if (!nv_encoder->crtc) { + nvif_outp_release(&nv_encoder->outp); + return; + } + head =3D nv50_head(nv_encoder->crtc); ``` **Issue 1: The race condition explanation is dubious.** The commit says: *"= a race condition can occur between atomic_check and atomic_commit: the enco= der's CRTC reference may become NULL after validation but before the disabl= e callback runs."* In the DRM atomic framework, `atomic_commit` holds all t= he necessary modeset locks. The `nv_encoder->crtc` field is nouveau's own i= nternal tracking, set at the end of `nv50_sor_atomic_disable` itself (line = 1604: `nv_encoder->crtc =3D NULL`). If `crtc` is NULL entering this functio= n, it is more likely that the encoder was never attached, or that there's a= double-disable scenario =E2=80=94 not an atomic check/commit race. The com= mit message should explain the actual root cause or at least describe it as= "crtc may be NULL in certain teardown paths" without speculating about a r= ace that the locking should prevent. **Issue 2: The early return skips significant cleanup.** When `nv_encoder->= crtc` is NULL, the function returns after only calling `nvif_outp_release()= `. But the full path also does: - Backlight disable via DPCD (`drm_edp_backlight_disable`) - HDMI state cleanup (`nvif_outp_hdmi(..., false, ...)`) - DP power-down (`nouveau_dp_power_down`) - Display ID clearing (`head->func->display_id(head, 0)`) - Encoder update and audio disable Some of these (like DP power-down, backlight disable) operate on the encode= r/connector, not the CRTC, and might still be needed even when the CRTC is = gone. If the CRTC being NULL is truly expected, the commit should justify w= hy none of that other cleanup matters. **Issue 3: The note about container_of is correct** =E2=80=94 `container_of= (NULL, ...)` produces a bogus non-NULL pointer, so the check must be done b= efore calling `nv50_head()`. This is well-explained. --- Generated by Claude Code Patch Reviewer