Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] drm/amdgpu: goto in amdgpu_gem_va_ioctl() skips cleanup guard init - [High] drm/amdgpu: wedge_status and rpm ref leaked or cleared early on reset - [High] drm/amdgpu: pci_clear_master() in amdgpu_job_timedout() breaks reset - [High] drm/amdgpu: amdgpu_job_timedout() leaves other rings fences unsignaled - [High] drm/amdkfd: NULL dereference in kfd_profiler_pmc() when GPU wedges - [High] drm/amdkfd: kfd_process_device_data_by_id() breaks unmap/free on wedge --
commit 567ed14b8d61a592e7054bc4f4ee30c73a041c70 Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: add wedge event implementation This commit adds DRM wedge event support to amdgpu when GPU auto-recovery is disabled. On a job timeout without recovery, pending fences on the ring are force-signalled, runtime PM is held, and new work submissions and KFD lookups are rejected until the GPU is reset. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 3658e46141ada..c48a3c44e2545 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c [ ... ] > @@ -4405,6 +4406,9 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev) > amdgpu_sync_free(&adev->isolation[i].prev); > } > > + if (atomic_xchg(&adev->wedge_status, 0)) > + pm_runtime_put_autosuspend(adev->dev); > + > amdgpu_reset_fini(adev); > > /* free i2c buses */ > @@ -5212,6 +5216,10 @@ int amdgpu_device_reinit_after_reset(struct > amdgpu_reset_context *reset_context) > r = 0; > list_for_each_entry(tmp_adev, device_list_handle, reset_list) { > amdgpu_set_init_level(tmp_adev, init_level); > + > + if (atomic_xchg(&tmp_adev->wedge_status, 0)) > + pm_runtime_put_autosuspend(tmp_adev->dev); > + > if (full_reset) { > /* post card */ > amdgpu_reset_set_dpc_status(tmp_adev, false); [Severity: High] Can this leave adev->wedge_status set and leak the runtime PM reference on ASICs that use reset_cntl handlers, or clear wedge_status too early during reset? In drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, amdgpu_do_asic_reset() calls amdgpu_reset_perform_reset() first and returns before reaching amdgpu_device_reinit_after_reset() whenever a reset handler is implemented: drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_do_asic_reset() { ... reset_context->reset_device_list = device_list_handle; r = amdgpu_reset_perform_reset(tmp_adev, reset_context); /* If reset handler not implemented, continue; otherwise return */ if (r == -EOPNOTSUPP) r = 0; else return r; ... } For all ASICs that register adev->reset_cntl in amdgpu_reset_init() (aldebaran, sienna_cichlid, smu_v13_0_10, and soc_v1_0), their restore_hwcontext callbacks never call amdgpu_device_reinit_after_reset(). As a result, triggering a reset after a wedge leaves adev->wedge_status set to 1 (causing amdgpu_gmc_flush_gpu_tlb() during GART enable to skip TLB invalidation, amdgpu_job_run() to cancel all jobs with -ECANCELED, and ioctls and KFD lookups to fail) and leaks the runtime PM reference until driver unload. Even on ASICs that do reach amdgpu_device_reinit_after_reset(), clearing tmp_adev->wedge_status and calling pm_runtime_put_autosuspend() at the start of the loop happens before amdgpu_device_asic_init(), IP resume phases, and amdgpu_ib_ring_tests() succeed. Concurrent ioctls that do not acquire reset_domain->sem can observe wedge_status == 0 while the hardware is mid-reset, and if reset fails, wedge_status remains 0 with the runtime PM reference already dropped. Additionally, on driver unbind (for example via DRM_WEDGE_RECOVERY_REBIND), amdgpu_device_fini_sw() only runs from drm_driver.release when the last open DRM file descriptor is closed, rather than synchronously in amdgpu_pci_remove() -> amdgpu_device_fini_hw(). If a userspace process keeps an fd open across unbind/rebind, won't the runtime PM reference on adev->dev stay held across the unbind and be dropped later on the newly bound device? > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 2145211b25e7e..85406b0b0125c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c [ ... ] > @@ -948,6 +951,11 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void > *data, > abo = NULL; > } > > + if (amdgpu_device_is_wedged(adev)) { > + r = -ENODEV; > + goto error_put_gobj; > + } > + > PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); > > r = PM_RUNTIME_ACQUIRE_ERR(&lock); [Severity: Critical] Does this goto in amdgpu_gem_va_ioctl() jump over the initialization of the cleanup-managed lock variable? In drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c, PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock) declares lock with __cleanup(class_pm_runtime_active_auto_try_enabled_destructor). Jumping to error_put_gobj from before that macro enters the scope of lock without initializing it: drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:amdgpu_gem_va_ioctl() { ... if (amdgpu_device_is_wedged(adev)) { r = -ENODEV; goto error_put_gobj; } PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); ... error_put_gobj: drm_gem_object_put(gobj); return r; } Aside from breaking the build under Clang (-Werror=jump-misses-init), when the GPU is wedged and userspace calls DRM_IOCTL_AMDGPU_GEM_VA, returning from error_put_gobj invokes class_pm_runtime_active_auto_try_enabled_destructor() on the uninitialized stack variable lock, passing uninitialized stack contents to pm_runtime_put_autosuspend(). > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > index 70a9a7d4650fb..e6172a9684cad 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)) { > + /* This ref will be dropped in > amdgpu_device_reinit_after_reset. */ > + pm_runtime_get_noresume(adev->dev); > + > + pci_clear_master(adev->pdev); [Severity: High] Does calling pci_clear_master(adev->pdev) here in amdgpu_job_timedout() break subsequent GPU resets when the user later triggers recovery? When a Mode 1 reset runs later via amdgpu_device_mode1_reset() in drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, it caches the current PCI configuration space before clearing bus master itself: drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_mode1_reset() { ... /* Cache the state before bus master disable. The saved config space * values are used in other cases like restore after mode-2 reset. */ amdgpu_device_cache_pci_state(adev->pdev); /* disable BM */ pci_clear_master(adev->pdev); ... amdgpu_device_load_pci_state(adev->pdev); ... } Because amdgpu_job_timedout() already cleared PCI_COMMAND_MASTER, amdgpu_device_cache_pci_state() overwrites adev->pci_state with bus mastering disabled, and amdgpu_device_load_pci_state() restores it without bus mastering enabled during ASIC init, phase 1 resume, and PSP firmware loading. In addition, amdgpu_device_pre_asic_reset() -> amdgpu_device_ip_suspend() runs with bus mastering disabled (affecting DMA during suspend, such as psp_suspend()), and Mode 2 reset paths that only resume GFX, MES, and SDMA without resuming IH: drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c:smu_v13_0_10_mode2_restore_ip() { ... for (i = 0; i < adev->num_ip_blocks; i++) { if (!(adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX || adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_MES || adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA)) continue; r = amdgpu_ip_block_resume(&adev->ip_blocks[i]); ... } never re-enable PCI bus mastering at all after reset. > + > + drm_dev_wedged_event(&adev->ddev, > DRM_WEDGE_RECOVERY_REBIND, info); > + } > + > + 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); > + } > } [Severity: High] What happens to in-flight fences on other rings when amdgpu_job_timedout() clears bus mastering for the whole device, and is drm_dev_wedged_event() sent before fences are signalled? In drivers/gpu/drm/amd/amdgpu/amdgpu_job.c, amdgpu_job_timedout() calls pci_clear_master(adev->pdev) and emits drm_dev_wedged_event() before stopping the scheduler or calling amdgpu_fence_driver_force_completion(), and only force-completes fences on the single timed-out ring. Once pci_clear_master() disables DMA writebacks and MSI interrupts across the entire PCI function: 1. In-flight fences on rings without a finite scheduler timeout (such as compute rings when lockup_timeout is 0, or KIQ/MES polling rings) cannot signal via hardware writeback and are never force-completed, leaving fence waiters blocked indefinitely. 2. In-flight jobs on other rings with finite timeouts are forced into secondary TDR timeouts, which unconditionally call amdgpu_job_core_dump() at the top of amdgpu_job_timedout() and overwrite the original hang's IP state dump. 3. Calling drm_dev_wedged_event() beforeamdgpu_fence_driver_force_completion() notifies userspace before pending fences on the wedged device have been signalled. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index ac18c2be6ecb6..b95dddeaccc51 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -2424,8 +2424,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; [Severity: High] Can returning NULL from kfd_process_device_data_by_id() when the device wedges cause a NULL pointer dereference in kfd_profiler_pmc()? In drivers/gpu/drm/amd/amdkfd/kfd_chardev.c, kfd_profiler_pmc() first calls profile_lock_device(), which looks up pdd under p->mutex and then drops p->mutex. Afterward, kfd_profiler_pmc() re-acquires p->mutex and calls kfd_process_device_data_by_id() a second time without checking for NULL: drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_profiler_pmc() { ... retval = profile_lock_device(p, args->gpu_id, args->lock); if (retval < 0) return retval; /* Enable/disable perfcount if requested */ mutex_lock(&p->mutex);\n pdd = kfd_process_device_data_by_id(p, args->gpu_id); dqm = pdd->dev->dqm; mutex_unlock(&p->mutex); ... } Because adev->wedge_status is set asynchronously in amdgpu_job_timedout(), if the GPU becomes wedged between the lookup in profile_lock_device() and the lookup in kfd_profiler_pmc(), pdd is NULL and pdd->dev->dqm dereferences NULL. Furthermore, if a process already locked the device for profiling before the wedge, subsequent calls to profile_lock_device() with op == 0 or kfd_profiler_ptl_control() will fail with -EINVAL and be unable to release kfd->profiler_process or the PTL disable reference. [Severity: High] Does returning NULL from kfd_process_device_data_by_id() also break KFD memory unmap, free, and SVM eviction paths when a GPU becomes wedged? Several KFD paths rely on kfd_process_device_data_by_id() to look up an existing pdd during cleanup or after waiting on fences: 1. In drivers/gpu/drm/amd/amdkfd/kfd_chardev.c, kfd_ioctl_unmap_memory_from_gpu() and kfd_ioctl_map_memory_to_gpu() look up peer_pdd in a first loop, wait in amdgpu_amdkfd_gpuvm_sync_memory(), and then look up peer_pdd again in a second loop: drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_ioctl_unmap_memory_from_gpu() { ... /* Flush TLBs after waiting for the page table updates to complete */ for (i = 0; i < args->n_devices; i++) { peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]); if (WARN_ON_ONCE(!peer_pdd)) continue; if (flush_tlb) kfd_flush_tlb(peer_pdd); /* Remove dma mapping after tlb flush to avoid IO_PAGE_FAULT */ err = amdgpu_amdkfd_gpuvm_dmaunmap_mem(mem, peer_pdd->drm_priv); ... } If the GPU wedges while waiting in amdgpu_amdkfd_gpuvm_sync_memory() (which returns 0 once the fences are force-completed), the second lookup returns NULL, triggering WARN_ON_ONCE(!peer_pdd) and skipping amdgpu_amdkfd_gpuvm_dmaunmap_mem(). 2. Once wedged, kfd_ioctl_free_memory_of_gpu() and kfd_ioctl_unmap_memory_from_gpu() fail with -EINVAL because kfd_process_device_data_by_id() returns NULL, preventing userspace from unmapping or freeing BOs on the wedged GPU (or freeing BOs on healthy GPUs that were peer-mapped to the wedged GPU). 3. In drivers/gpu/drm/amd/amdkfd/kfd_svm.c, svm_range_get_node_by_id() calls kfd_process_device_data_by_id(). When a GPU is wedged, svm_range_best_prefetch_location() triggers WARN_ONCE(1, "failed to get valid kfd node at id%x\n", best_loc), and TTM eviction in svm_range_evict_svm_bo() -> svm_migrate_vram_to_ram() fails with -ENODEV after prange has already been unlinked from svm_bo->range_list. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
