Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Negative error codes from `nvif_device_gcx_ready()` are treated as bitmasks, leading to unpredictable runtime suspend behavior. - [High] Hardcoding `r535_gsp_gcx_ready(gsp)` bypasses the GSP function vtable and will cause a NULL pointer dereference on non-RM GSPs. - [Medium] `nvkm_udevice_gcx_ready()` bypasses the ioctl unpack protocol and returns a positive bitmask directly as a return code. - [Low] `GSP_GCX_ENTRY_GC6_READY` and `GSP_GCX_ENTRY_GCOFF_READY` are defined but never used. --
commit 9f05a64653e46f7b990f534168f7024b8a4d408a Author: Lyude Paul <[email protected]> drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE OpenRM consults GSP to check if the GPU should be allowed to enter Gc6 and/or GcOff before runtime suspending. If not ready, runtime suspend is delayed. This commit implements the NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE query and adds readiness checks in nouveau_pmops_runtime_suspend(). > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > @@ -10,6 +10,9 @@ > #define GSP_PAGE_SHIFT 12 > #define GSP_PAGE_SIZE BIT(GSP_PAGE_SHIFT) > > +#define GSP_GCX_ENTRY_GC6_READY BIT(0) > +#define GSP_GCX_ENTRY_GCOFF_READY BIT(1) [Severity: Low] This isn't a bug, but these macros appear to be unused in this patch? It looks like NV_DEVICE_GC6_READY and NV_DEVICE_GCOFF_READY are used instead throughout the rest of the implementation. [ ... ] > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c > b/drivers/gpu/drm/nouveau/nouveau_drm.c > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c > @@ -1157,6 +1157,7 @@ nouveau_pmops_runtime_suspend(struct device *dev) > { > struct pci_dev *pdev = to_pci_dev(dev); > struct nouveau_drm *drm = pci_get_drvdata(pdev); > + struct nvif_device *nvif = &drm->client.device; > int ret; > > if (!nouveau_pmops_runtime()) { > @@ -1164,6 +1165,14 @@ nouveau_pmops_runtime_suspend(struct device *dev) > return -EBUSY; > } > > + // Check if the GPU itself is ready for runtime suspend, otherwise mark > as busy and check > + // again in a bit. > + if (!(nvif_device_gcx_ready(nvif) & NV_DEVICE_GCOFF_READY)) { [Severity: High] Can this bitwise check misinterpret negative error codes? Looking at nvif_device_gcx_ready(), it can return negative error codes like -ENOSYS or other transport errors. Since negative numbers usually have higher bits set in two's complement (e.g., -ENOSYS often has bit 1 set), the bitwise AND with NV_DEVICE_GCOFF_READY (which is BIT(1)) might evaluate to non-zero. This could cause the driver to mistakenly assume the GPU is ready and proceed with runtime suspend when the query actually failed, leading to unpredictable power transition behavior. > + NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n"); > + pm_runtime_mark_last_busy(dev); > + return -EBUSY; > + } [ ... ] > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > @@ -189,6 +191,38 @@ nvkm_udevice_time(struct nvkm_udevice *udev, void *data, > u32 size) > return ret; > } > > +static int > +nvkm_udevice_gcx_ready(struct nvkm_udevice *udev, void *data, u32 size) > +{ > + struct nvkm_object *object = &udev->object; > + struct nvkm_device *device = udev->device; > + struct nvkm_gsp *gsp = device->gsp; > + union { > + struct nv_device_gcx_ready_v0 v0; > + } *args = data; > + int ret = -ENOSYS; > + > + /* XXX: We don't know any way of performing this check outside of GSP, > so just assume > + * things are ready for non-GSP clients. > + */ > + if (!device->gsp) > + return NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY; [Severity: Medium] Does this early return break the NVIF mthd API contract? The convention for mthd handlers is generally to unpack the arguments, set the output fields in the args struct, and return 0 on success. Returning a positive bitmask directly here bypasses the nvif_unpack sequence. This positive value then bubbles up to the caller in nvif_device_gcx_ready(), where it acts as a pseudo-error code and leaves the actual args.ready field uninitialized. > + > + nvif_ioctl(object, "device gcx ready size %d\n", size); > + if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false))) { > + nvif_ioctl(object, "device gcx ready vers %d\n", > args->v0.version); > + > + ret = r535_gsp_gcx_ready(gsp); [Severity: High] Could this lead to a NULL pointer dereference on architectures running without full Resource Manager (RM) functionality? When falling back to firmware like gv100_gsp_nofw (e.g., on Turing or Ampere), device->gsp is non-NULL, so the early return above is bypassed. However, the RM structures are not initialized. When r535_gsp_gcx_ready() is called in this state, it eventually invokes nvkm_gsp_rm_ctrl_rd() -> nvkm_gsp_rm_ctrl_get(), which attempts to dereference object->client->gsp->rm->api. If gsp->rm is NULL, this will crash. Would checking for !nvkm_gsp_rm(device->gsp) rather than !device->gsp safeguard against this? > + if (ret < 0) > + return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
