Re: [PATCH] drm/nouveau/disp: Set drm_mode_config_funcs.atomic_(check|commit) (attempt 2)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/nouveau/disp: missing closing quote in Fixes: trailer
  subject
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] drm/nouveau: wrong dtor order and disp leak in
  nouveau_display_create() [1]
--

commit cf4b32d591c8a8a0033562aad54661c60e5b35bf
Author: Dave Airlie <airlied@redhat.com>

> drm/nouveau/disp: Set drm_mode_config_funcs.atomic_(check|commit) (attempt 2)
> 
> commit 604826acb3f5 attempted to add these flags but
> commit 6c65db809796 reverted it due to it
> failing on hw from pre-atomic times.
> 
> So use a second set of functions.
> 
> original commit msg from Lyude:
> "Apparently we never actually filled these in, despite the fact that we do
> in fact technically support atomic modesetting.
> 
> Since not having these filled in causes us to potentially forget to disable
> fbdev and friends during suspend/resume, let's fix it."
> 
> Reported-by: Ruben Wauters <rubenru09@aol.com>
> Closes: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/517
> Fixes: 6c65db809796 ("Revert "drm/nouveau/disp: Set drm_mode_config_funcs.atomic_(check|commit)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dave Airlie <airlied@redhat.com>

[Severity: Low]
The Fixes: trailer is missing the closing quote from the reverted commit's
subject:

  Fixes: 6c65db809796 ("Revert "drm/nouveau/disp: Set drm_mode_config_funcs.atomic_(check|commit)"")

> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index d71dcfc6ee66b..07882624a7f6f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
[ ... ]
> @@ -654,7 +659,11 @@ nouveau_display_create(struct drm_device *dev)
>  	drm_mode_create_scaling_mode_property(dev);
>  	drm_mode_create_dvi_i_properties(dev);
>  
> -	dev->mode_config.funcs = &nouveau_mode_config_funcs;
> +	/* this only gets set if the driver has the ATOMIC flag */
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		dev->mode_config.funcs = &nouveau_mode_config_funcs_atomic;
> +	else
> +		dev->mode_config.funcs = &nouveau_mode_config_funcs;
>  
>  	dev->mode_config.min_width = 0;
>  	dev->mode_config.min_height = 0;

[Severity: High]
This problem wasn't introduced by this patch, but can the error unwind paths
in nouveau_display_create() cause a NULL pointer dereference and leak
disp [1]?

drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_create() {
	...
vblank_err:
	disp->dtor(dev);
disp_create_err:
	drm_kms_helper_poll_fini(dev);
	drm_mode_config_cleanup(dev);
	return ret;
}

When drm_vblank_init() fails, vblank_err calls disp->dtor(dev) before
drm_mode_config_cleanup(dev), which is the opposite of the order in
nouveau_display_destroy().  On pre-NV50 hardware, nv04_display_destroy()
clears nouveau_display(dev)->priv to NULL and frees struct nv04_display; when
drm_mode_config_cleanup() then runs nv_crtc_destroy(), nv04_display(crtc->dev)
returns NULL and dereferencing disp->image[nv_crtc->index] crashes.

In addition, neither error path calls nvif_disp_dtor(&disp->disp) or
kfree(disp), and nouveau_drm_device_init() skips nouveau_display_destroy() on
failure, leaking both objects and leaving drm->display dangling.

[1] https://sashiko.dev/#/bug/linux-dd1d9372-c030-413b-9ce8-94b2029fab64

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-nouveau-atomic-fix-v1-1-118e65d3a2f5@redhat.com?part=1