[PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device

[PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device

[Pierre-Eric Pelloux-Prayer]

Add a per-device boolean to control whether GPU recovery is attempted
on a hang, independently of the global amdgpu.gpu_recovery module
parameter. It defaults to true and is exposed as a write to the
existing amdgpu_gpu_recover debugfs file, so a single device can have
auto-recovery disabled without affecting every other GPU in the
system.

amdgpu_device_should_recover_gpu() now takes this flag into account.

Assisted-by: Claude:Sonnet 5
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  7 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  5 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 21 +++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 7974f9b7944f..21b33dc34edf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -957,6 +957,13 @@ struct amdgpu_device {
 
 	struct amdgpu_uma_carveout_info uma_info;
 
+	/* Whether this device is allowed to attempt GPU recovery on a hang.
+	 * Defaults to true; can be turned off per-device (e.g. via the
+	 * amdgpu_gpu_recover debugfs file) independently of the global
+	 * amdgpu.gpu_recovery module parameter.
+	 */
+	bool gpu_recovery_allowed;
+
 	/* KFD
 	 * Must be last --ends in a flexible-array member.
 	 */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 964efec0d335..5578d5f64937 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4034,6 +4034,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		}
 	}
 
+	adev->gpu_recovery_allowed = true;
+
 fence_driver_init:
 	/* Fence driver */
 	r = amdgpu_fence_driver_sw_init(adev);
@@ -4834,6 +4836,9 @@ bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev)
 	if (amdgpu_gpu_recovery == 0)
 		goto disabled;
 
+	if (!adev->gpu_recovery_allowed)
+		goto disabled;
+
 	/* Skip soft reset check in fatal error mode */
 	if (!amdgpu_ras_is_poison_mode_supported(adev))
 		return true;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 3043ad041bb4..707e69d8bb11 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -975,9 +975,13 @@ static int amdgpu_debugfs_fence_info_show(struct seq_file *m, void *unused)
 }
 
 /*
- * amdgpu_debugfs_gpu_recover - manually trigger a gpu reset & recover
+ * amdgpu_debugfs_gpu_recover - manually trigger a gpu reset & recover,
+ * and control whether this device is allowed to auto-recover from a hang.
  *
- * Manually trigger a gpu reset at the next fence wait.
+ * Read triggers a gpu reset at the next fence wait.
+ *
+ * Write 0/1 to disable/enable auto GPU recovery for this device
+ * (equivalent to amdgpu.gpu_recovery=0, but scoped to this device only).
  */
 static int gpu_recover_get(void *data, u64 *val)
 {
@@ -1001,8 +1005,17 @@ static int gpu_recover_get(void *data, u64 *val)
 	return 0;
 }
 
+static int gpu_recover_set(void *data, u64 val)
+{
+	struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+	adev->gpu_recovery_allowed = !!val;
+
+	return 0;
+}
+
 DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info);
-DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, NULL,
+DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, gpu_recover_set,
 			 "%lld\n");
 
 static void amdgpu_debugfs_reset_work(struct work_struct *work)
@@ -1037,7 +1050,7 @@ void amdgpu_debugfs_fence_init(struct amdgpu_device *adev)
 	if (!amdgpu_sriov_vf(adev)) {
 
 		INIT_WORK(&adev->reset_work, amdgpu_debugfs_reset_work);
-		debugfs_create_file("amdgpu_gpu_recover", 0444, root, adev,
+		debugfs_create_file("amdgpu_gpu_recover", 0644, root, adev,
 				    &amdgpu_debugfs_gpu_recover_fops);
 	}
 #endif
-- 
2.43.0

[PATCH v2 2/5] drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout

[Pierre-Eric Pelloux-Prayer]

The soft-recovery, per-ring-reset and IP-block-reset attempts in
amdgpu_job_timedout() were only gated on the global amdgpu_gpu_recovery
module parameter. Introduce a single can_reset variable that also
takes the per-device adev->gpu_recovery_allowed flag into account, and
use it for all three attempts instead of reading amdgpu_gpu_recovery
directly.

Assisted-by: Claude:Sonnet 5
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index d97cf1212e0f..43511e0419a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -92,6 +92,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
 	struct drm_wedge_task_info *info = NULL;
 	struct amdgpu_task_info *ti = NULL;
 	struct amdgpu_device *adev = ring->adev;
+	bool can_reset = amdgpu_gpu_recovery && adev->gpu_recovery_allowed;
 	int idx, r;
 
 	if (!drm_dev_enter(adev_to_drm(adev), &idx)) {
@@ -111,7 +112,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
 	if (!amdgpu_sriov_vf(adev))
 		amdgpu_job_core_dump(adev, job);
 
-	if (amdgpu_gpu_recovery &&
+	if (can_reset &&
 	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_SOFT_RECOVERY) &&
 	    amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) {
 		dev_err(adev->dev, "ring %s timeout, but soft recovered\n",
@@ -130,7 +131,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
 	}
 
 	/* attempt a per ring reset */
-	if (amdgpu_gpu_recovery &&
+	if (can_reset &&
 	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_PER_QUEUE) &&
 	    ring->funcs->reset) {
 		dev_err(adev->dev, "Starting %s ring reset\n",
@@ -152,7 +153,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
 	}
 
 	/* Attempt an IP block soft reset, if supported. */
-	if (amdgpu_gpu_recovery &&
+	if (can_reset &&
 	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET)) {
 		r = amdgpu_device_ip_soft_reset(ring, job->hw_fence);
 		if (!r) {
-- 
2.43.0

[PATCH v2 3/5] drm/amdgpu: add wedge event implementation

[Pierre-Eric Pelloux-Prayer]

When a job times out the kernel tries to reset the GPU to get
the system back to a normal state.
Sometimes it's useful to disable this process - for instance to
be able to inspect the hardware state at the time of the hang.
Since leaving fence unsignalled might affect the system's
stability, this commit enables the drm wedge framework for amdgpu.

When a hang condition is detected, the GPU isn't reset but all
the pending fences are signalled and the hung GPU cannot receive
new work. For KFD it's implemented by making
kfd_process_device_data_by_id return NULL if the requested GPU
is wedged.

When the user is done, it's possible to trigger a GPU
reset.

The auto-recovery is still enabled by default, and to use this
feature one has to either:
* boot with amdgpu.gpu_recovery=0: not recommended as it disables
  recovery for all GPUs
* write 0 to /sys/kernel/debug/dri/X/amdgpu_gpu_recover to disable
  auto-recovery for one GPU only

Runtime power management is disabled when the device is wedged
(because we can't submit any work to the GPU).

Assisted-by: Claude:Sonnet 5
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 13 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  5 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 34 ++++++++++++++++++++--
 drivers/gpu/drm/amd/amdkfd/kfd_process.c   |  6 +++-
 6 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 21b33dc34edf..039a92f2eb0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -964,6 +964,13 @@ struct amdgpu_device {
 	 */
 	bool gpu_recovery_allowed;
 
+	/* Set to 1 when the device has been declared wedged following a
+	 * hang: it no longer accepts new work and pending fences have been
+	 * force-signalled. Reset to 0 once the device is reset back to a
+	 * working state.
+	 */
+	atomic_t wedge_status;
+
 	/* KFD
 	 * Must be last --ends in a flexible-array member.
 	 */
@@ -1349,4 +1356,10 @@ void amdgpu_device_set_uid(struct amdgpu_uid *uid_info,
 			   uint64_t uid);
 uint64_t amdgpu_device_get_uid(struct amdgpu_uid *uid_info,
 			       enum amdgpu_uid_type type, uint8_t inst);
+
+static inline bool amdgpu_device_is_wedged(struct amdgpu_device *adev)
+{
+	return atomic_read(&adev->wedge_status);
+}
+
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5578d5f64937..949de13d7997 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4035,6 +4035,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	}
 
 	adev->gpu_recovery_allowed = true;
+	if (atomic_xchg(&adev->wedge_status, 0))
+		pm_runtime_put_autosuspend(adev->dev);
 
 fence_driver_init:
 	/* Fence driver */
@@ -5745,6 +5747,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->wedge_status, 0))
+		pm_runtime_put_autosuspend(adev->dev);
 
 	if (!r) {
 		struct amdgpu_task_info *ti = NULL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 0ab380ca7e64..399e935df7b2 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;
+
 	ret = pm_runtime_get_sync(dev->dev);
 	if (ret < 0)
 		goto out;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 3dc8faa091d7..f43d85ba4b78 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -716,6 +716,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
 	struct amdgpu_job *job;
 	int r;
 
+	if (amdgpu_device_is_wedged(adev))
+		return;
+
 	ring = to_amdgpu_ring(adev->mman.buffer_funcs_scheds[0]);
 
 	if (!hub->sdma_invalidation_workaround || vmid ||
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 43511e0419a1..41d083646da5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -24,6 +24,7 @@
 #include <linux/kthread.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/pm_runtime.h>
 
 #include <drm/drm_drv.h>
 
@@ -185,9 +186,35 @@ 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 {
+			struct drm_gpu_scheduler *sched;
+			struct amdgpu_fence *guilty_fence;
+
+			/* Declare the device as wedged if it's not already. */
+			if (!atomic_xchg(&adev->wedge_status, 1)) {
+				pm_runtime_get_sync(adev->dev);
+
+				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;
+
+			sched = &ring->sched;
+
+			/* 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);
+		}
 	}
 
 exit:
@@ -453,7 +480,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 
 	/* Skip job if VRAM is lost and never resubmit gangs */
 	if (job->generation != amdgpu_vm_generation(adev, job->vm) ||
-	    (job->job_run_counter && job->gang_submit))
+	    (job->job_run_counter && job->gang_submit) ||
+	    amdgpu_device_is_wedged(adev))
 		dma_fence_set_error(finished, -ECANCELED);
 
 	if (finished->error < 0) {
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 0a7c1900da95..544dc960833b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -2433,8 +2433,12 @@ struct kfd_process_device *kfd_process_device_data_by_id(struct kfd_process *p,
 		for (i = 0; i < p->n_pdds; i++) {
 			struct kfd_process_device *pdd = p->pdds[i];
 
-			if (pdd->user_gpu_id == gpu_id)
+			if (pdd->user_gpu_id == gpu_id) {
+				if (amdgpu_device_is_wedged(pdd->dev->adev))
+					return NULL;
+
 				return pdd;
+			}
 		}
 	}
 	return NULL;
-- 
2.43.0

[PATCH v2 4/5] drm/amdgpu: skip amdgpu_gart_unbind if the device is wedged

[Pierre-Eric Pelloux-Prayer]

This operation requires access to the GPU.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
index c4c21dbbbdbf..09ddfccb8174 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
@@ -315,7 +315,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
 	uint64_t flags = 0;
 	int idx;
 
-	if (!adev->gart.ptr)
+	if (!adev->gart.ptr || amdgpu_device_is_wedged(adev))
 		return;
 
 	if (!drm_dev_enter(adev_to_drm(adev), &idx))
-- 
2.43.0

[PATCH v2 5/5] drm/amdgpu: skip amdgpu_gmc_flush_gpu_tlb_pasid if device is wedged

[Pierre-Eric Pelloux-Prayer]

Otherwise it'll fail with: "failed to reg_write_reg_wait" and we're
going to fill the MES buffer which will prevent the reset to succeed.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index f43d85ba4b78..c33320a6413e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -786,6 +786,9 @@ int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid,
 	int r, cnt = 0;
 	uint32_t seq;
 
+	if (amdgpu_device_is_wedged(adev))
+		return -ENODEV;
+
 	/*
 	 * A GPU reset should flush all TLBs anyway, so no need to do
 	 * this while one is ongoing.
-- 
2.43.0

Re: [PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Overloading an action-triggering debugfs file with a configuration state setter creates a severe usability trap.
- [Medium] Missing initialization of `gpu_recovery_allowed` in emulation mode unintentionally disables GPU recovery.

Pre-existing issues:
- [High] Kernel stack memory leak in `gpu_recover_get` during debugfs file read.
--

--- Patch [1]: [PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device ---
commit 387fc7f831c412d1422444dc6a5d877786192a5b
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device
    
Add a per-device boolean to control whether GPU recovery is attempted
on a hang, independently of the global amdgpu.gpu_recovery module
parameter. It defaults to true and is exposed as a write to the
existing amdgpu_gpu_recover debugfs file.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 520fd59036d57..eba75b41b50a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4080,6 +4080,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	adev->gpu_recovery_allowed = true;
> +
>  fence_driver_init:

[Severity: Medium]
Does this skip initializing the flag in emulation mode?

If amdgpu_emu_mode == 1, the code earlier in this function jumps directly to
fence_driver_init:

amdgpu_device_init() {
    if (amdgpu_emu_mode == 1) {
        emu_soc_asic_init(adev);
        goto fence_driver_init;
    }
    ...
}

Since the device structure is zero-allocated, jumping over the assignment
will leave gpu_recovery_allowed as false, which inadvertently disables GPU
recovery for this device in emulation mode. Should the initialization be
moved before the goto or handled at the beginning of the function?

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 3043ad041bb42..707e69d8bb115 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c

[ ... ]

>  static int gpu_recover_get(void *data, u64 *val)
>  {

[Severity: High]
This is a pre-existing issue, but does this function leak kernel stack memory?

In simple_attr_read(), an uninitialized u64 val is allocated on the stack
and passed to the attribute's get callback. gpu_recover_get() returns 0
without writing to val. The uninitialized value is then formatted into a
string and copied to userspace.

Because this patch converts the file into a configuration knob, users and
tooling are much more likely to read it to check its state, making this
leak more exposed.

[ ... ]

> @@ -1001,8 +1005,17 @@ static int gpu_recover_get(void *data, u64 *val)
>  	return 0;
>  }
>  
> +static int gpu_recover_set(void *data, u64 val)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)data;
> +
> +	adev->gpu_recovery_allowed = !!val;
> +
> +	return 0;
> +}
> +
>  DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info);
> -DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, NULL,
> +DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, gpu_recover_set,
>  			 "%lld\n");

[Severity: High]
Does overloading this debugfs file create a usability trap?

The patch adds a set callback to configure gpu_recovery_allowed, changing
amdgpu_gpu_recover into a dual-purpose configuration file. However,
gpu_recover_get unconditionally triggers a full GPU reset when read, rather
than returning the current configuration value:

gpu_recover_get() {
    ...
    if (amdgpu_reset_domain_schedule(adev->reset_domain, &adev->reset_work))
        flush_work(&adev->reset_work);
    return 0;
}

If a user attempts to read amdgpu_gpu_recover to check if recovery is
enabled, they will unintentionally reset the GPU. Would it be safer to
use a separate debugfs file for the configuration flag?

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

Re: [PATCH v2 2/5] drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent GPU ring timeouts cause list corruption due to unprotected modification of `adev->reset_list`.
--

--- Patch [2]: [PATCH v2 2/5] drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout ---
commit 2bc1f829e1f5f1741410cfa00dfab26ce6099f90
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: honor gpu_recovery_allowed in amdgpu_job_timedout

This commit introduces a single can_reset variable in amdgpu_job_timedout
that takes both the global module parameter and the per-device
gpu_recovery_allowed flag into account. It uses this new variable to gate
the soft-recovery, per-ring-reset, and IP-block-reset attempts.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index d97cf1212e0f9..43511e0419a12 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c

[ ... ]

> @@ -111,7 +112,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  	if (!amdgpu_sriov_vf(adev))
>  		amdgpu_job_core_dump(adev, job);

[Severity: High]
This is a pre-existing issue, but can this lead to list corruption if
multiple GPU rings timeout concurrently?

When a timeout occurs, the code calls amdgpu_job_core_dump(). If the
device does not have an XGMI hive (hive is NULL), amdgpu_job_core_dump()
links the shared adev->reset_list into a local stack-allocated list:

drivers/gpu/drm/amd/amdgpu/amdgpu_job.c:amdgpu_job_core_dump() {
        ...
        if (!amdgpu_sriov_vf(adev) && (adev->gmc.xgmi.num_physical_nodes > 1) && hive) {
                ...
        } else {
                list_add_tail(&adev->reset_list, &device_list);
                device_list_handle = &device_list;
        }
        ...
}

Since adev->reset_list is shared, if an untrusted userspace application
submits hanging shaders on two different rings concurrently, wouldn't both
threads race to modify adev->reset_list without any lock protection? 

This looks like it could lead to wild pointer dereferences during list 
iteration or when exiting the stack frame on concurrent executions of
amdgpu_job_timedout() for the same adev.

> -	if (amdgpu_gpu_recovery &&
> +	if (can_reset &&
>  	    amdgpu_ring_is_reset_type_supported(ring, AMDGPU_RESET_TYPE_SOFT_RECOVERY) &&
>  	    amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) {
>  		dev_err(adev->dev, "ring %s timeout, but soft recovered\n",

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

Re: [PATCH v2 3/5] drm/amdgpu: add wedge event implementation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] NULL pointer dereference in `kfd_profiler_pmc` due to TOCTOU race with asynchronous wedge state.
- [High] Concurrent GPU recovery races with wedge state application in `amdgpu_job_timedout`, leading to PM counter underflow and broken bus master state.
- [High] Top-level ioctl wedge check blocks core DRM cleanup ioctls, leading to kernel resource leaks.
--

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

This commit enables the drm wedge framework for amdgpu. When a hang is
detected, the GPU isn't reset but pending fences are signalled and new
work is blocked. Runtime PM is also disabled when the device is wedged.

> 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 this block core resource cleanup ioctls? By returning -ENODEV here,
it bypasses drm_ioctl() entirely. 

If a Wayland compositor or X server tries to gracefully clean up a client's
resources after a GPU hang, won't standard cleanup interfaces like
DRM_IOCTL_GEM_CLOSE or DRM_IOCTL_SYNCOBJ_DESTROY fail and leak memory in
the kernel?

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

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 43511e0419a12..41d083646da59 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
[ ... ]
> @@ -185,9 +186,35 @@ 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 {
> +			struct drm_gpu_scheduler *sched;
> +			struct amdgpu_fence *guilty_fence;
> +
> +			/* Declare the device as wedged if it's not already. */
> +			if (!atomic_xchg(&adev->wedge_status, 1)) {
> +				pm_runtime_get_sync(adev->dev);
> +
> +				pci_clear_master(adev->pdev);
> +

[Severity: High]
Can this race with concurrent GPU recovery since it is not holding the
adev->reset_domain->sem lock?

If a user manually triggers a GPU reset concurrently, the reset thread in
amdgpu_device_gpu_recover() could clear wedge_status to 0 and call
pm_runtime_put_autosuspend(). 

If this happens while amdgpu_job_timedout() is preempted before executing
pm_runtime_get_sync(), will the PM usage count underflow? 

Furthermore, when amdgpu_job_timedout() resumes, won't it execute
pci_clear_master() on the newly-recovered healthy GPU, breaking its state?

> +				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;

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 0a7c1900da959..544dc960833b2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
[ ... ]
> @@ -2433,8 +2433,12 @@ struct kfd_process_device *kfd_process_device_data_by_id(struct kfd_process *p,
>  		for (i = 0; i < p->n_pdds; i++) {
>  			struct kfd_process_device *pdd = p->pdds[i];
>  
> -			if (pdd->user_gpu_id == gpu_id)
> +			if (pdd->user_gpu_id == gpu_id) {
> +				if (amdgpu_device_is_wedged(pdd->dev->adev))
> +					return NULL;
> +

[Severity: High]
Does returning NULL dynamically here introduce a TOCTOU race leading to a
NULL pointer dereference?

Looking at kfd_profiler_pmc() in kfd_chardev.c, it acquires the pdd pointer
and immediately dereferences it without checking for NULL:

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_profiler_pmc() {
    ...
    mutex_lock(&p->mutex);
    pdd = kfd_process_device_data_by_id(p, args->gpu_id);
    dqm = pdd->dev->dqm;
    mutex_unlock(&p->mutex);
    ...
}

Since the device can enter the wedged state asynchronously on a job timeout,
could this second call to kfd_process_device_data_by_id() return NULL and
cause a kernel crash when pdd->dev is accessed?

Additionally, does this abort unmapping loops and leak mappings on healthy
GPUs?

In kfd_ioctl_unmap_memory_from_gpu(), if kfd_process_device_data_by_id()
returns NULL for a wedged GPU, the loop aborts and goes to 
get_mem_obj_from_handle_failed, leaking mappings on any subsequent healthy
GPUs in the array.

>  				return pdd;
> +			}
>  		}
>  	}
>  	return NULL;

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