Re: [PATCH v1 1/3] drm/amdgpu: add wedge event implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Concurrent execution of wedge recovery across multiple rings causes race conditions in DRM scheduler and fence driver state.
- [High] Unconditionally returning `-ENODEV` in `amdgpu_drm_ioctl` blocks all core DRM ioctls, breaking userspace tools, KMS, and resource cleanup.
- [Medium] The per-device debugfs node `amdgpu_wedge` incorrectly modifies a global module parameter, affecting all GPUs.
- [Medium] The PM runtime usage counter is leaked if the device is removed or unbound while in the wedged state.
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race exists between `amdgpu_job_timedout` setting the wedge flag and `amdgpu_device_gpu_recover` clearing it.
--

--- Patch [1]: [PATCH v1 1/3] drm/amdgpu: add wedge event implementation ---
commit da2e81546012e13c20ccab9ed3ffded97a0f3ece
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Subject: drm/amdgpu: add wedge event implementation

This commit adds a wedge framework for amdgpu to disable gpu recovery and
signal pending fences upon a hang, allowing userspace to inspect hardware
state. The wedge status is tracked via an atomic flag, and users can
manually trigger recovery later.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 964efec0d3352..c636a94d869c7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
[ ... ]
> @@ -5740,6 +5744,8 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>  		dev_info(adev->dev, "GPU reset end with ret = %d\n", r);
>  
>  	atomic_set(&adev->reset_domain->reset_res, r);
> +	if (atomic_xchg(&adev->wedged.flag, 0))
> +		pm_runtime_put_autosuspend(adev->dev);
>  

[Severity: Medium]
Is there a race condition between setting the wedge flag in
amdgpu_job_timedout() and clearing it here?

If a TDR thread in amdgpu_job_timedout() sets the flag but gets preempted
before calling pci_clear_master(), a concurrent call to
amdgpu_device_gpu_recover() could clear the flag.

If the TDR thread then resumes execution, could it execute
pci_clear_master() on a successfully recovered and active GPU?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 0ab380ca7e645..399e935df7b29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -3006,10 +3006,15 @@ long amdgpu_drm_ioctl(struct file *filp,
>  		      unsigned int cmd, unsigned long arg)
>  {
>  	struct drm_file *file_priv = filp->private_data;
> +	struct amdgpu_device *adev;
>  	struct drm_device *dev;
>  	long ret;
>  
>  	dev = file_priv->minor->dev;
> +	adev = drm_to_adev(dev);
> +	if (amdgpu_device_is_wedged(adev))
> +		return -ENODEV;
> +

[Severity: High]
Does returning -ENODEV unconditionally here block all core DRM ioctls?

This seems like it would lock out standard KMS and modeset queries, as
well as diagnostic tools attempting to read hardware state.

Could this prevent the exact hardware inspection the commit message
mentions?

>  	ret = pm_runtime_get_sync(dev->dev);
>  	if (ret < 0)
>  		goto out;

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 3043ad041bb42..b241062985769 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
[ ... ]
> @@ -974,6 +974,39 @@ static int amdgpu_debugfs_fence_info_show(struct seq_file *m, void *unused)
> +static int amdgpu_device_wedged_set(void *data, u64 val)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)data;
> +
> +	if (amdgpu_device_is_wedged(adev))
> +		return -EINVAL;
> +
> +	if (val)
> +		amdgpu_gpu_recovery = 0;
> +	else
> +		amdgpu_gpu_recovery = adev->wedged.boot_gpu_recovery;
> +
> +	return 0;
> +}
> +

[Severity: Medium]
Does modifying amdgpu_gpu_recovery here affect all GPUs in a multi-GPU
system?

The commit message mentions this is to enable/disable at runtime for one
GPU, but amdgpu_gpu_recovery is a global module parameter.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index d97cf1212e0f9..187eac9aab1f0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
[ ... ]
> @@ -184,9 +185,44 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  		if (r)
>  			dev_err(adev->dev, "GPU Recovery Failed: %d\n", r);
>  	} else {
> -		drm_sched_suspend_timeout(&ring->sched);
> -		if (amdgpu_sriov_vf(adev))
> +		if (amdgpu_sriov_vf(adev)) {
> +			drm_sched_suspend_timeout(&ring->sched);
>  			adev->virt.tdr_debug = true;
> +		} else {
> +			/* Declare the device as wedged if it's not already. */
> +			struct drm_gpu_scheduler *sched;
> +			struct amdgpu_ring *ring;
> +			struct amdgpu_fence *guilty_fence;
> +			int i;
> +
> +			if (!atomic_xchg(&adev->wedged.flag, 1)) {
> +				pm_runtime_get_sync(adev->dev);
> +

[Severity: Medium]
If the device is unbound or hot-unplugged while in the wedged state, will
this pm_runtime_get_sync() reference be leaked?

It appears there is no corresponding put if the device is torn down before
manual recovery is triggered.

> +				pci_clear_master(adev->pdev);
> +
> +				drm_dev_wedged_event(&adev->ddev, DRM_WEDGE_RECOVERY_REBIND |
> +						DRM_WEDGE_RECOVERY_BUS_RESET, NULL);
> +			}
> +
> +			guilty_fence = to_amdgpu_job(s_job)->hw_fence;
> +
> +			for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +				ring = adev->rings[i];
> +
> +				if (!amdgpu_ring_sched_ready(ring))
> +					continue;
> +
> +				sched = &ring->sched;
> +
> +				/* Stop the scheduler to prevent anybody else from touching the ring buffer. */
> +				drm_sched_wqueue_stop(sched);
> +
> +				amdgpu_fence_driver_force_completion(ring, &guilty_fence->base);
> +
> +				/* Start the scheduler again */
> +				drm_sched_wqueue_start(sched);
> +			}

[Severity: High]
Since this loop iterating over AMDGPU_MAX_RINGS is outside the
atomic_xchg() check, can multiple concurrent TDR threads race on stopping
and starting schedulers?

If multiple rings hang concurrently, all their respective TDR threads will
attempt to stop/start the schedulers and force fence completions across
all rings simultaneously, which could corrupt the scheduler and fence
driver state.

> +		}
>  	}
>  
>  exit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810165322.24682-1-pierre-eric.pelloux-prayer@amd.com?part=1