[PATCH] drm/amdgpu: handle the case of pci_channel_io_frozen only in amdgpu_pci_resume

[Chen, Guchun]

[Public]

Thanks Andrey, I will update the name to be " pci_channel_state " when submitting.

Regards,
Guchun

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com> 
Sent: Tuesday, October 5, 2021 12:04 AM
To: Chen, Guchun <Guchun.Chen at amd.com>; amd-gfx at lists.freedesktop.org; Koenig, Christian <Christian.Koenig at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Liu, Monk <Monk.Liu at amd.com>
Subject: Re: [PATCH] drm/amdgpu: handle the case of pci_channel_io_frozen only in amdgpu_pci_resume


On 2021-10-02 11:18 a.m., Guchun Chen wrote:
> In current code, when a PCI error state pci_channel_io_normal is 
> detectd, it will report PCI_ERS_RESULT_CAN_RECOVER status to PCI 
> driver, and PCI driver will continue the execution of PCI resume 
> callback report_resume by pci_walk_bridge, and the callback will go 
> into amdgpu_pci_resume finally, where write lock is releasd 
> unconditionally without acquiring such lock first. In this case, a 
> deadlock will happen when other threads start to acquire the read lock.
>
> To fix this, add a member in amdgpu_device strucutre to cache 
> pci_channel_state, and only continue the execution in 
> amdgpu_pci_resume when it's pci_channel_io_frozen.
>
> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
> Suggested-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
> Signed-off-by: Guchun Chen <guchun.chen at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
>   2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f4bceb2624fb..720d0ccecfe0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1094,6 +1094,7 @@ struct amdgpu_device {
>   
>   	bool                            no_hw_access;
>   	struct pci_saved_state          *pci_state;
> +	pci_channel_state_t		cached_state;


I would give a more descriptive name to this (e.g. pci_channel_state) Other then that Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>

Andrey


>   
>   	struct amdgpu_reset_control     *reset_cntl;
>   	uint32_t                        ip_versions[HW_ID_MAX][HWIP_MAX_INSTANCE];
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index bb5ad2b6ca13..1aaeb4b30edc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5368,6 +5368,8 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>   		return PCI_ERS_RESULT_DISCONNECT;
>   	}
>   
> +	adev->cached_state = state;
> +
>   	switch (state) {
>   	case pci_channel_io_normal:
>   		return PCI_ERS_RESULT_CAN_RECOVER; @@ -5510,6 +5512,10 @@ void 
> amdgpu_pci_resume(struct pci_dev *pdev)
>   
>   	DRM_INFO("PCI error: resume callback!!\n");
>   
> +	/* Only continue execution for the case of pci_channel_io_frozen */
> +	if (adev->cached_state != pci_channel_io_frozen)
> +		return;
> +
>   	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   		struct amdgpu_ring *ring = adev->rings[i];
>