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 <[email protected]> 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/[email protected]?part=5
