Re: [PATCH 2/2] drm/amdgpu: rework the amdgpu_device_gpu_recover() lock sequence

On 7/27/26 15:35, Prike Liang wrote:
> Suspend the KMS/DRM clients before locking the reset domain.
> drm_client_dev_suspend() acquires clientlist_mutex, which must be
> taken outside reset_domain->sem. Acquiring it while holding the
> reset domain lock inverts the ordering established elsewhere
> (clientlist_mutex -> ... -> reset_domain->sem) and can deadlock.


That is a good catch, but clearly not the correct resolution.

drm_client_dev_suspend() should *never* be called from the reset path in the first place.

This is only for suspend/resume and can trivially deadlock here through quite a number of other locks.

Regards,
Christian.

> 
> Signed-off-by: Prike Liang <[email protected]>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 472e96ae884e..c670d5cfe3cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5371,8 +5371,6 @@ static void amdgpu_device_halt_activities(struct amdgpu_device *adev,
>  		 */
>  		amdgpu_unregister_gpu_instance(tmp_adev);
>  
> -		drm_client_dev_suspend(adev_to_drm(tmp_adev));
> -
>  		/* disable ras on ALL IPs */
>  		if (!need_emergency_restart && !amdgpu_reset_in_dpc(adev))
>  			amdgpu_ras_suspend(tmp_adev);
> @@ -5535,6 +5533,22 @@ static void amdgpu_device_gpu_resume(struct amdgpu_device *adev,
>  }
>  
>  
> +static void amdgpu_device_recovery_suspend_clients(struct list_head *device_list)
> +{
> +	struct amdgpu_device *tmp_adev = NULL;
> +
> +	list_for_each_entry(tmp_adev, device_list, reset_list)
> +		drm_client_dev_suspend(adev_to_drm(tmp_adev));
> +}
> +
> +static void amdgpu_device_recovery_resume_clients(struct list_head *device_list)
> +{
> +	struct amdgpu_device *tmp_adev = NULL;
> +
> +	list_for_each_entry(tmp_adev, device_list, reset_list)
> +		drm_client_dev_resume(adev_to_drm(tmp_adev));
> +}
> +
>  /**
>   * amdgpu_device_gpu_recover - reset the asic and recover scheduler
>   *
> @@ -5613,6 +5627,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>  	/* Cannot be called after locking reset domain */
>  	amdgpu_ras_pre_reset(adev, &device_list);
>  
> +	amdgpu_device_recovery_suspend_clients(&device_list);
>  	/* We need to lock reset domain only once both for XGMI and single device */
>  	amdgpu_device_recovery_get_reset_lock(adev, &device_list);
>  
> @@ -5649,6 +5664,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>  	amdgpu_device_gpu_resume(adev, &device_list, need_emergency_restart);
>  reset_unlock:
>  	amdgpu_device_recovery_put_reset_lock(adev, &device_list);
> +	amdgpu_device_recovery_resume_clients(&device_list);
>  	amdgpu_ras_post_reset(adev, &device_list);
>  end_reset:
>  	if (hive) {