Re: [PATCH 5/5] drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] An always-false `cbc_size < 0` check fails to catch errors in `r570_fbsr_suspend()`, causing arithmetic wrap-around and an undersized scatter-gather table allocation.
- [High] Error paths in `r570_fbsr_suspend()` fail to restore hardware state, leaving the GPU suspended.
--

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

drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

Now that we're properly saving the compbit backing stores on fbsr init, we
can start setting bEnteringGcOff = 1 again without things breaking, which
brings us closer to following the exact same code-paths OpenRM does for
fbsr.

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

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential issue in
r570_fbsr_suspend() while reviewing this file:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c:r570_fbsr_suspend() {
    ...
	u64 size, cbc_size;
    ...
	cbc_size = r570_fb_get_compbit_store_size(gsp);
	if (cbc_size < 0)
		return cbc_size;
    ...
}

Because cbc_size is typed as u64, is it possible for the cbc_size < 0
check to ever be true?

If r570_fb_get_compbit_store_size() returns a negative error code
like PTR_ERR(ctrl), it will be cast to a massive unsigned value. This
huge value would then be added to size later in the function, which
could wrap around and lead to allocating an undersized SG table. Can
this overrun the allocated scatter-gather table during suspend?


[Severity: High]
This isn't a bug introduced by this patch, but I found another issue with
the error paths in r570_fbsr_suspend():

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c:r570_fbsr_suspend() {
	/* Stop channel scheduling. */
	r570_fbsr_suspend_channels(gsp, true);

	/* Temporarily disable raw mode ... */
	if (r570_need_raw_comp_war(gsp, device)) {
		ret = r570_memsys_enable_raw_comp_mode(gsp, false);
		if (ret)
			return ret;
	}
    ...
	/* 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;
		}
	}
    ...
}

If operations like r570_memsys_enable_raw_comp_mode() or nvkm_instobj_save()
fail, the function returns immediately. Does this leave the GPU with channels
suspended and raw compression mode disabled without rolling them back?

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