Re: [PATCH v2 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 1 potential issue(s) to consider:
- [Medium] nouveau: nvif_device_gcx_ready negative error codes handled as bitmask
--

commit 181d7ad17939e59149b939f631f63cac7294478a
Author: Lyude Paul <lyude@redhat.com>

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

This commit adds support for querying the GSP to see if the GPU is ready
to enter Gc6 and/or GcOff before runtime suspending. If the GPU is not
ready, the runtime suspend process is delayed.

> 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
[ ... ]
> @@ -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)) {
> +		NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n");
> +		pm_runtime_mark_last_busy(dev);
> +		return -EBUSY;
> +	}
> +

[Severity: Medium]
Should this check for a negative error code before applying the bitmask?

The nvif_device_gcx_ready() function can return negative error codes
if the underlying RPC call fails. Because this returned integer is
directly bitwise ANDed with NV_DEVICE_GCOFF_READY (which is BIT(1)),
the two's complement representation of a negative error code will yield
an arbitrary true or false result depending on its specific bit pattern.

For example, if an error returns -EINVAL (-22), bit 1 is set. The bitwise
check evaluates to true, which bypasses the delay and unsafely puts the
GPU to sleep. Alternatively, if the error is -ENOMEM (-12), bit 1 is
cleared, which evaluates to false and delays the suspend.

> diff --git a/drivers/gpu/drm/nouveau/nvif/device.c b/drivers/gpu/drm/nouveau/nvif/device.c
> --- a/drivers/gpu/drm/nouveau/nvif/device.c
> +++ b/drivers/gpu/drm/nouveau/nvif/device.c
[ ... ]
> +int
> +nvif_device_gcx_ready(struct nvif_device *device)
> +{
> +	struct nv_device_gcx_ready_v0 args = {};
> +	int ret;
> +
> +	ret = nvif_object_mthd(&device->object, NV_DEVICE_V0_GCX_READY, &args, sizeof(args));
> +	if (ret)
> +		return ret;
> +
> +	return args.ready;
> +}

Here is where nvif_device_gcx_ready() explicitly propagates the negative
error code returned from nvif_object_mthd() on failure.

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