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