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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup of RM object (memlist) on error paths in FBSR initialization.
- [High] Error paths in `r570_fbsr_suspend()` leave GPU channel scheduling and BAR2 in a broken state.
--

commit 7d63de5b70c2db9e57a30fb5f72d5fa08bb0b6fa
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
fail on many laptops.

Cc: <stable@vger.kernel.org> # v6.19+

> 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
> @@ -81,7 +81,7 @@ r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size, bool runtim
>  	ctrl->hClient = gsp->internal.client.object.handle;
>  	ctrl->hSysMem = memlist.handle;
>  	ctrl->sysmemAddrOfSuspendResumeData = gsp->sr.meta.addr;
> -	ctrl->bEnteringGcoffState = runtime ? 1 : 0;
> +	ctrl->bEnteringGcoffState = 1;
>  
>  	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
>  	if (ret)

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak the
memlist RM object?

If nvkm_gsp_rm_ctrl_wr() fails here, we return early without calling
nvkm_gsp_rm_free(&memlist).

Because the object uses a hardcoded handle (0xcaf00003), leaking it could
cause subsequent suspend attempts to fail with a name collision error. A
similar leak seems to exist just above this for nvkm_gsp_rm_ctrl_get()
failures, and also in the r535 backend in fbsr_init().

>  		return ret;
>  
>  	nvkm_gsp_rm_free(&memlist);
>  	return 0;
>  }
>  
>  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;

[ ... ]

> @@ -133,7 +133,7 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp, bool runtime)
>  		return ret;
>  
>  	/* Initialise FBSR on RM. */
> -	ret = r570_fbsr_init(gsp, &gsp->sr.fbsr, size, runtime);
> +	ret = r570_fbsr_init(gsp, &gsp->sr.fbsr, size);
>  	if (ret) {
>  		nvkm_gsp_sg_free(device, &gsp->sr.fbsr);
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this return leave the GPU channel
scheduling and BAR2 in a broken state?

Earlier in r570_fbsr_suspend(), we disable channel scheduling and BAR2 access:

r570_fbsr_suspend() {
    ...
	/* Stop channel scheduling. */
	r570_fbsr_suspend_channels(gsp, true);
    ...
	/* Disable BAR2 access. */
	device->bar->bar2 = false;
    ...
}

If r570_fbsr_init() (or the earlier nvkm_gsp_sg and nvkm_instobj_save calls)
fails, we return the error without restoring channel scheduling or BAR2 access.

Since the suspend callback returns an error, the kernel PM core will abort the
suspend process and not invoke the corresponding resume callback, potentially
leaving the GPU in a broken, half-suspended state.

>  	}
>

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