Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/amdgpu: gpu_recovery_enabled left false in amdgpu_device_init() - [Medium] drm/amdgpu: per-device gpu_recovery_enabled ignored in userq and KFD resets --
commit 86b9554fae60bb7deeb057b1bff4576f147cba44 Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: add gpu_recovery_enabled flag to amdgpu_device This commit adds a per-device gpu_recovery_enabled boolean to struct amdgpu_device and exposes it via the amdgpu_gpu_recover debugfs file to control automatic GPU recovery independently of the global amdgpu.gpu_recovery module parameter. It also updates amdgpu_device_should_recover_gpu() to check this flag. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 92cc0fa7ba33b..3658e46141ada 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -4149,6 +4149,8 @@ int amdgpu_device_init(struct amdgpu_device *adev, > } > } > > + adev->gpu_recovery_enabled = true; > + > fence_driver_init: > /* Fence driver */ > r = amdgpu_fence_driver_sw_init(adev); [Severity: Medium] Does placing the initialization of adev->gpu_recovery_enabled before the fence_driver_init label leave it false when amdgpu_emu_mode == 1? Earlier in amdgpu_device_init(), the emulation mode path jumps directly to fence_driver_init: drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_init() { ... if (amdgpu_emu_mode == 1) { /* post the asic on emulation mode */ emu_soc_asic_init(adev); goto fence_driver_init; } ... } Because struct amdgpu_device is zero-allocated in amdgpu_pci_probe() via devm_drm_dev_alloc(), jumping to fence_driver_init bypasses setting adev->gpu_recovery_enabled = true. As a result, amdgpu_device_should_recover_gpu() will treat GPU recovery as disabled during amdgpu_device_ip_init() -> amdgpu_get_soft_full_reset_mask() and on runtime hangs. Subsequent commits in this series do not adjust this placement. Should adev->gpu_recovery_enabled = true be moved after the fence_driver_init label (or before the goto fence_driver_init)? [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > index bff2d38f3571a..c54ff34572b00 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > @@ -980,9 +980,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) > { > @@ -1006,8 +1010,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_enabled = !!val; > + > + return 0; > +} [Severity: Medium] The comment states that writing 0 to amdgpu_gpu_recover is equivalent to amdgpu.gpu_recovery=0 scoped to this device, and later commits in this series ("drm/amdgpu: honor gpu_recovery_enabled in amdgpu_job_timedout" and "drm/amdgpu: add wedge event implementation") update amdgpu_job_timedout() to check adev->gpu_recovery_enabled. However, several other hang recovery and timeout paths only check the global amdgpu_gpu_recovery variable and do not consult adev->gpu_recovery_enabled or amdgpu_device_should_recover_gpu(adev): 1. Usermode queue hang detection and reset in drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c: amdgpu_userq_hang_detect_work() { ... if (!amdgpu_gpu_recovery) return; ... } amdgpu_userq_mgr_reset_work() { ... if (!amdgpu_gpu_recovery) return; ... amdgpu_device_gpu_recover(adev, NULL, &reset_context); } 2. KFD queue and HWS hang resets in drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c within remove_queue_mes_on_reset_option(), reset_queues_mes(), and reset_queues_on_hws_hang(), which only check if (!amdgpu_gpu_recovery). 3. Timeout enablement checks in drivers/gpu/drm/amd/amdgpu/amdgpu.h: static inline bool amdgpu_device_has_timeouts_enabled(struct amdgpu_device *adev) { return amdgpu_gpu_recovery != 0 && adev->gfx_timeout != MAX_SCHEDULE_TIMEOUT && ... } When a user writes 0 to amdgpu_gpu_recover while amdgpu.gpu_recovery remains at its default non-zero value, should these userq, KFD, and timeout helpers also check adev->gpu_recovery_enabled so that per-device recovery is fully disabled? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
