Re: [PATCH 1/5] drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260911222428.752641-1-lyude@redhat.com?part=1