Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The early return added to guard against a NULL `nv_encoder->crtc` during inconsistent atomic disables is placed too late. When `CONFIG_DRM_NOUVEAU_BACKLIGHT` is enabled, the code unconditionally initializes and dereferences `nv_connector` (via `nv_connector->backlight`) before the new `!nv_encoder->crtc` safeguard is reached. In the exact state-tracking inconsistencies this patch aims to fix (e.g., disable without a matching enable), `nv50_outp_get_old_connector()` can return NULL, leading to a NULL pointer dereference. -- commit c0ee57413f11f85e156ca80153f2707e59b79de5 Author: Marek Czernohous <[email protected]> drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable() This commit restores a NULL guard for the crtc pointer in nv50_sor_atomic_disable() to prevent a kernel oops during VT switches and Wayland session teardowns. The guard replaces a silent return with a warning to ensure state-tracking inconsistencies remain visible without crashing. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 6c3a8712d38ab..597bc64073a5c 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -1565,14 +1565,28 @@ static void > nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_state > *state) > { > struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); > - struct nv50_head *head = nv50_head(nv_encoder->crtc); > + struct nv50_head *head; > #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT > struct nouveau_connector *nv_connector = > nv50_outp_get_old_connector(state, nv_encoder); > struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); > struct nouveau_backlight *backlight = nv_connector->backlight; > struct drm_dp_aux *aux = &nv_connector->aux; [Severity: High] This isn't a bug introduced by this patch, but does the new early return happen too late if CONFIG_DRM_NOUVEAU_BACKLIGHT is enabled? In the exact disable-without-enable scenario this patch is addressing, nv50_outp_get_old_connector() can return NULL. Because nv_connector is unconditionally dereferenced here to initialize the backlight and aux variables, won't this still cause a NULL pointer dereference in nv50_sor_atomic_disable() before the newly added crtc guard is ever reached? > int ret; > +#endif > + > + /* nv_encoder->crtc is the driver's shadow pointer, set in > + * .atomic_enable and cleared at the end of this function. NULL here > + * means disable-without-enable or a double disable; bail before > + * container_of() turns it into a bogus head pointer (checking the > + * result would not work, container_of(NULL) is never NULL). The > + * encoder release is handled by the commit_tail release loop, so > + * there is nothing to clean up here. > + */ > + if (drm_WARN_ON_ONCE(encoder->dev, !nv_encoder->crtc)) > + return; > + head = nv50_head(nv_encoder->crtc); > > +#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT > if (backlight && backlight->uses_dpcd) { > ret = drm_edp_backlight_disable(aux, &backlight->edp_info); > if (ret < 0) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
