Re: [PATCH v4 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] GSP firmware memory allocation (memlist) is permanently leaked on error paths in r570_fbsr_init() and fbsr_init().
- [High] Incomplete error rollback in r570_fbsr_suspend() leaves hardware disabled and leaks system memory.
--

--- Patch [1]: [PATCH v4 1/3] Revert "nouveau/gsp: fix suspend/resume regression on r570 firmware" ---
commit e90949dff871229c99ce748acd0012d5f702035b
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's not -
as this causes runtime PM to actually fail on many a laptop.

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 permanently
leak the memlist object?

In r570_fbsr_init(), if the allocation of the control structure fails:

    ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.subdevice,
                                NV2080_CTRL_CMD_INTERNAL_FBSR_INIT, sizeof(*ctrl));
    if (IS_ERR(ctrl))
        return PTR_ERR(ctrl);

The function returns directly, leaving the memlist object allocated on the GSP 
firmware with a hardcoded handle. Because the handle is hardcoded, would a 
subsequent suspend attempt fail with a duplicate handle error, permanently 
breaking system suspend until the driver is reloaded?

(A similar pattern also appears to exist in fbsr_init()).

[ ... ]

> @@ -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 error path leave the GPU in an
unusable state and leak system memory?

If r570_fbsr_init() fails, r570_fbsr_suspend() returns the error without 
rolling back prior operations:

    /* Initialise FBSR on RM. */
    ret = r570_fbsr_init(gsp, &gsp->sr.fbsr, size);
    if (ret) {
        nvkm_gsp_sg_free(device, &gsp->sr.fbsr);
        return ret;
    }

Since PM core aborts suspend and doesn't call resume on failure, does this 
leave the GPU unresponsive with channel scheduling suspended and BAR2 disabled?

Additionally, earlier in the same function:

    /* 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;
        }
    }

Would failing here (or later) permanently leak the memory allocated by 
nvkm_instobj_save(), since it's only freed during a successful resume?

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