Re: [PATCH v2 1/4] Revert "nouveau/gsp: fix suspend/resume regression on r570 firmware"

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Failing to restore hardware state if `r570_fbsr_suspend` encounters an error midway through execution.
--

commit b9938c05e690ef9ccc55ef6c3f7541c371717b40
Author: Lyude Paul <lyude@redhat.com>

Revert "nouveau/gsp: fix suspend/resume regression on r570 firmware"

This reverts commit 8302d0afeaec0bc57d951dd085e0cffe997d4d18. It turns out
this looked like the right fix on some systems, but it causes runtime PM to
actually fail on many laptops.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> index 8ef8b4f655883..2945d5b4e5707 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
[ ... ]
> @@ -92,7 +92,7 @@ r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size, bool runtim
>  }
>  
>  static int
> -r570_fbsr_suspend(struct nvkm_gsp *gsp, bool runtime)
> +r570_fbsr_suspend(struct nvkm_gsp *gsp)
>  {
>  	struct nvkm_subdev *subdev = &gsp->subdev;
>  	struct nvkm_device *device = subdev->device;

[Severity: High]
This is a pre-existing issue, but does r570_fbsr_suspend() need to
unwind hardware state changes if an error occurs?

Looking at the function body, hardware state is mutated upfront:

r570_fbsr_suspend() {
        ...
        /* Stop channel scheduling. */
        r570_fbsr_suspend_channels(gsp, true);

        /* Save BAR2 allocations to system memory. */
        list_for_each_entry(iobj, &imem->list, head) {
                if (iobj->preserve) {
                        ret = nvkm_instobj_save(iobj);
                        if (ret)
                                return ret;
                }
        }
        ...
        /* Disable BAR2 access. */
        device->bar->bar2 = false;
        ...
}

If a failure occurs (such as memory allocation failure in nvkm_instobj_save()
or later in nvkm_gsp_sg()), the function returns an error without reverting
these hardware changes.

The error propagates to nvkm_subdev_fini(), returning early without setting
subdev->use.enabled to false. During a suspend abort recovery,
nvkm_device_fini() attempts to restart failed devices via nvkm_subdev_init().

However, because use.enabled is still true, the restart is skipped, meaning
the device's init and resume callbacks are never executed.

Could this leave the GPU permanently unusable with suspended channels and
disabled BAR2 access if a failure happens during suspend?

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