Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] The error check `if (cbc_size < 0)` is always false because `cbc_size` is an unsigned `u64`. When `r570_fb_get_compbit_store_size()` fails, it returns a negative error code that is implicitly cast to a large positive `u64`, bypassing the error check and causing an integer wrap-around. - [Low] Mismatched GSP object handles used for API allocation and cleanup in `r570_fb_get_compbit_store_size()`. Pre-existing issues: - [High] Missing cleanup of suspended channels on error paths in `r570_fbsr_suspend()`. -- commit caaa903dee4578bf9a609b4561ba8eb17ac6e144 Author: Lyude Paul <[email protected]> drm/nouveau/gsp/r570: Start saving comptag backing stores This commit implements support for saving and restoring comptag backing stores in OpenRM's fbsr process by fetching the required space and allocating it. The goal is to safely enable bEnteringGcOff in 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 7f5aae0c055b8..58f426ba81191 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 > @@ -26,6 +26,24 @@ r570_fbsr_suspend_channels(struct nvkm_gsp *gsp, bool > suspend) > return nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl); > } > > +static u64 > +r570_fb_get_compbit_store_size(struct nvkm_gsp *gsp) > +{ > + NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS *ctrl; > + u64 size; > + > + ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.object, > + NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO, > + sizeof(*ctrl)); > + if (IS_ERR(ctrl)) > + return PTR_ERR(ctrl); [Severity: Critical] Since r570_fb_get_compbit_store_size() returns a u64, does this implicitly cast the negative signed error code (like -ENOMEM) into a huge positive value? > + > + size = ctrl->Size; > + > + nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl); [Severity: Low] Does this violate API symmetry by allocating the control struct using &gsp->internal.device.object but cleaning it up with &gsp->internal.device.subdevice? > + return size; > +} > + > static int > r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable) > { [ ... ] > @@ -151,6 +169,11 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp) > return ret; [Severity: High] This isn't a bug introduced by this patch, but does this error path fail to undo r570_fbsr_suspend_channels(gsp, true) called earlier in the function? If the suspend fails here, it seems the channels are left permanently suspended, making the GPU unusable until a reboot. > } > > + cbc_size = r570_fb_get_compbit_store_size(gsp); > + if (cbc_size < 0) > + return cbc_size; [Severity: Critical] Is this error check dead code? Since cbc_size is an unsigned u64, it can never be less than 0. If r570_fb_get_compbit_store_size() fails, the implicitly cast large positive error code will bypass this check, leading to an integer wrap-around when added to size later in the function. Could this cause an undersized memory allocation that results in memory corruption when the firmware attempts to write the FBSR state out of bounds? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
