Re: [PATCH v4 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 3 potential issue(s) to consider:
- [Critical] nouveau/gsp: NULL pointer dereference when RM is disabled
- [Medium] pm: autosuspend stall on transient GCX readiness query error
- [Low] nouveau/gsp: API mismatch in gcx_ready using different object for alloc/free
--

commit 71e096484486fcd07150f6da48fcb331a8eae639
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

OpenRM consults GSP to ask whether the GPU should be allowed to enter Gc6
and/or GcOff before runtime suspending. This patch implements the
NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE command query to check for
this readiness and delay runtime suspend if necessary.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index b0f9fb10a74dd..c2e2f430deded 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c

[ ... ]

> @@ -1164,6 +1165,18 @@ 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.
> +	ret = nvif_device_gcx_ready(nvif);
> +	if (ret < 0) {
> +		NV_ERROR(drm, "Failed to query GCX readiness (returned %d)\n", ret);
> +		return -EBUSY;

[Severity: Medium]
When nvif_device_gcx_ready() fails, this error path returns -EBUSY without
calling pm_runtime_mark_last_busy(dev).

Will this stall the autosuspend timer? Without advancing last_busy, the PM
core will not automatically reschedule the timer, leaving the device in
the RPM_ACTIVE state.

> +	} else if (!(ret & NV_DEVICE_GCOFF_READY)) {
> +		NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n");
> +		pm_runtime_mark_last_busy(dev);
> +		return -EBUSY;
> +	}
> +
>  	nouveau_switcheroo_optimus_dsm();
>  	ret = nouveau_do_suspend(drm, true);

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
> index 9ba1316831e7e..e475d0e8fa7bf 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c

[ ... ]

> @@ -47,6 +48,15 @@ nvkm_gsp_intr_stall(struct nvkm_gsp *gsp, enum nvkm_subdev_type type, int inst)
>  	return -ENOENT;
>  }
>  
> +int
> +nvkm_gsp_gcx_ready(struct nvkm_gsp *gsp)
> +{
> +	if (!gsp->rm->api->gsp->gcx_ready)

[Severity: Critical]
Does this code cause a NULL pointer dereference when RM is disabled?

On devices that use GSP but do not load GSP-RM (such as GV100 or Turing
without experimental RM enabled, or when disabled via the module parameter),
gsp->rm is never allocated and remains NULL.

Since nouveau_pmops_runtime_suspend() invokes this path for any GPU with a
GSP, won't this result in a kernel panic upon entering runtime suspend?

> +		return NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY;
> +
> +	return gsp->rm->api->gsp->gcx_ready(gsp);
> +}
> +

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> index b45781cd0dfdc..dfda6b0b910a2 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c

[ ... ]

> @@ -215,6 +216,32 @@ r570_gsp_set_rmargs(struct nvkm_gsp *gsp, bool resume)
>  	args->bDmemStack = 1;
>  }
>  
> +int
> +r570_gsp_gcx_ready(struct nvkm_gsp *gsp)
> +{
> +	NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS *ctrl;
> +	int ret = 0;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.subdevice,
> +				   NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE,
> +				   sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	if (ctrl->bIsGC6Satisfied)
> +		ret |= NV_DEVICE_GC6_READY;
> +	if (ctrl->bIsGCOFFSatisfied)
> +		ret |= NV_DEVICE_GCOFF_READY;
> +
> +	nvkm_debug(&gsp->subdev,
> +		   "GCX ready status: GC6=%s GCOFF=%s\n",
> +		   str_yes_no(ctrl->bIsGC6Satisfied), str_yes_no(ctrl->bIsGCOFFSatisfied));
> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.client.object, ctrl);

[Severity: Low]
This isn't a bug, but is there an object mismatch here?

The memory for ctrl is allocated earlier using the device.subdevice object:

	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.subdevice, ...);

but it is released here using the mismatched client.object.

While this avoids a crash because both objects' client fields resolve to
the same GSP context internally, does this violate the API contract?

> +	return ret;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916223358.507351-1-lyude@redhat.com?part=1