Re: [PATCH 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The shared initialization function nouveau_drm_device_new() incorrectly hardcodes modifications to the global driver_pci struct instead of the provided drm_driver parameter.

Pre-existing issues:
- [High] nouveau_drm_device_del() attempts to release an error pointer if drm_dev_alloc() fails, causing a kernel panic.
--

--- Patch [2]: [PATCH 2/4] drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor() ---
commit c020f75bf537ed725d051dbe5e66bd9bd3dfb6b7
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms: Check nouveau.atomic parameter after nvif_device_ctor()

This commit moves the check for the nouveau.atomic parameter into
nouveau_drm_device_new() so it occurs after the NVIF device is ready.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ad693d2849bec..8899f009f825e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -727,7 +727,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
>  }
>  
>  static struct nouveau_drm *
> -nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
> +nouveau_drm_device_new(struct drm_driver *drm_driver, struct device *parent,
>  		       struct nvkm_device *device)
>  {

[Severity: High]
This is a pre-existing issue, but does the error path in
nouveau_drm_device_new() cause a crash if drm_dev_alloc() fails?

If drm_dev_alloc() fails, it returns an error pointer which is stored
directly in drm->dev:

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_new() {
    ...
    drm->dev = drm_dev_alloc(drm_driver, parent);
    if (IS_ERR(drm->dev)) {
        ret = PTR_ERR(drm->dev);
        goto done;
    }
    ...
done:
    if (ret) {
        nouveau_drm_device_del(drm);
...
}

Then in nouveau_drm_device_del(), the truthiness check will succeed for
the error pointer, leading to a panic when it is passed to drm_dev_put():

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_del() {
    if (drm->dev)
        drm_dev_put(drm->dev);
    ...
}

Could we clear drm->dev on allocation failure, or check for IS_ERR()
in the cleanup function?

[ ... ]

> @@ -770,6 +770,9 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
>  		goto done;
>  	}
>  
> +	if (nouveau_atomic)
> +		driver_pci.driver_features |= DRIVER_ATOMIC;

[Severity: Medium]
Does this unintentionally modify the global driver_pci struct instead of the
drm_driver parameter passed to nouveau_drm_device_new()?

If nouveau_drm_device_new() is called for a platform device (such as Tegra),
the platform driver template would miss the DRIVER_ATOMIC flag, and the
PCI driver template would be unexpectedly mutated instead.

Should this update drm_driver->driver_features rather than driver_pci?

> +
>  	ret = nvif_device_map(&drm->device);
>  	if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730010828.1056397-1-lyude@redhat.com?part=2