Re: [PATCH v2 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[sashiko-bot@kernel.org]

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 <mczernohous@gmail.com>

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/cover.1781162589.git.marek@czernohous.de?part=2