* [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
@ 2021-10-01 2:00 Guchun Chen
2021-10-01 2:21 ` Andrey Grodzovsky
0 siblings, 1 reply; 6+ messages in thread
From: Guchun Chen @ 2021-10-01 2:00 UTC (permalink / raw)
To: amd-gfx, christian.koenig, xinhui.pan, alexander.deucher,
andrey.grodzovsky
Cc: Guchun Chen
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.
Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
Signed-off-by: Guchun Chen <guchun.chen@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index bb5ad2b6ca13..12f822d51de2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5370,6 +5370,7 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
switch (state) {
case pci_channel_io_normal:
+ amdgpu_device_lock_adev(adev, NULL);
return PCI_ERS_RESULT_CAN_RECOVER;
/* Fatal error, prepare for slot reset */
case pci_channel_io_frozen:
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
2021-10-01 2:00 [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal Guchun Chen
@ 2021-10-01 2:21 ` Andrey Grodzovsky
2021-10-01 8:21 ` Chen, Guchun
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Grodzovsky @ 2021-10-01 2:21 UTC (permalink / raw)
To: Guchun Chen, amd-gfx, christian.koenig, xinhui.pan, alexander.deucher
On 2021-09-30 10:00 p.m., Guchun Chen wrote:
> 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.
Good catch but, the issue is even wider in scope, what about
drm_sched_resubmit_jobs
and drm_sched_start called without being stopped before ? Better to put
the entire scope
of code in this function under flag that set only in
pci_channel_io_frozen. As far as i remember
we don't need to do anything in case of pci_channel_io_normal.
Andrey
>
> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index bb5ad2b6ca13..12f822d51de2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5370,6 +5370,7 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>
> switch (state) {
> case pci_channel_io_normal:
> + amdgpu_device_lock_adev(adev, NULL);
> return PCI_ERS_RESULT_CAN_RECOVER;
> /* Fatal error, prepare for slot reset */
> case pci_channel_io_frozen:
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
2021-10-01 2:21 ` Andrey Grodzovsky
@ 2021-10-01 8:21 ` Chen, Guchun
2021-10-01 14:28 ` Andrey Grodzovsky
0 siblings, 1 reply; 6+ messages in thread
From: Chen, Guchun @ 2021-10-01 8:21 UTC (permalink / raw)
To: Grodzovsky, Andrey, amd-gfx, Koenig, Christian, Pan, Xinhui,
Deucher, Alexander
[Public]
Hi Andrey,
Do you mean to move the code of drm_sched_resubmit_jobs and drm_sched_start in amdgpu_pci_resume to amdgpu_pci_error_detected, under the case pci_channel_io_frozen?
Then leave amdgpu_pci_resume as a null function, and in this way, we can drop the acquire/lock write lock for case of pci_channel_io_normal as well?
Regards,
Guchun
-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Friday, October 1, 2021 10:22 AM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
On 2021-09-30 10:00 p.m., Guchun Chen wrote:
> 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.
Good catch but, the issue is even wider in scope, what about drm_sched_resubmit_jobs and drm_sched_start called without being stopped before ? Better to put the entire scope of code in this function under flag that set only in pci_channel_io_frozen. As far as i remember we don't need to do anything in case of pci_channel_io_normal.
Andrey
>
> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index bb5ad2b6ca13..12f822d51de2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5370,6 +5370,7 @@ pci_ers_result_t
> amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>
> switch (state) {
> case pci_channel_io_normal:
> + amdgpu_device_lock_adev(adev, NULL);
> return PCI_ERS_RESULT_CAN_RECOVER;
> /* Fatal error, prepare for slot reset */
> case pci_channel_io_frozen:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
2021-10-01 8:21 ` Chen, Guchun
@ 2021-10-01 14:28 ` Andrey Grodzovsky
2021-10-01 15:21 ` Chen, Guchun
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Grodzovsky @ 2021-10-01 14:28 UTC (permalink / raw)
To: Chen, Guchun, amd-gfx, Koenig, Christian, Pan, Xinhui, Deucher,
Alexander
No, scheduler restart and device unlock must take place
inamdgpu_pci_resume (see struct pci_error_handlers for the various
states of PCI recovery). So just add a flag (probably in amdgpu_device)
so we can remember what pci_channel_state_t we came from (unfortunately
it's not passed to us in amdgpu_pci_resume) and unless it's set don't
do anything in amdgpu_pci_resume.
Andrey
On 2021-10-01 4:21 a.m., Chen, Guchun wrote:
> [Public]
>
> Hi Andrey,
>
> Do you mean to move the code of drm_sched_resubmit_jobs and drm_sched_start in amdgpu_pci_resume to amdgpu_pci_error_detected, under the case pci_channel_io_frozen?
> Then leave amdgpu_pci_resume as a null function, and in this way, we can drop the acquire/lock write lock for case of pci_channel_io_normal as well?
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Sent: Friday, October 1, 2021 10:22 AM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
>
> On 2021-09-30 10:00 p.m., Guchun Chen wrote:
>
>> 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.
>
> Good catch but, the issue is even wider in scope, what about drm_sched_resubmit_jobs and drm_sched_start called without being stopped before ? Better to put the entire scope of code in this function under flag that set only in pci_channel_io_frozen. As far as i remember we don't need to do anything in case of pci_channel_io_normal.
>
> Andrey
>
>
>> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index bb5ad2b6ca13..12f822d51de2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -5370,6 +5370,7 @@ pci_ers_result_t
>> amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>>
>> switch (state) {
>> case pci_channel_io_normal:
>> + amdgpu_device_lock_adev(adev, NULL);
>> return PCI_ERS_RESULT_CAN_RECOVER;
>> /* Fatal error, prepare for slot reset */
>> case pci_channel_io_frozen:
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
2021-10-01 14:28 ` Andrey Grodzovsky
@ 2021-10-01 15:21 ` Chen, Guchun
2021-10-02 15:20 ` Chen, Guchun
0 siblings, 1 reply; 6+ messages in thread
From: Chen, Guchun @ 2021-10-01 15:21 UTC (permalink / raw)
To: Grodzovsky, Andrey, amd-gfx, Koenig, Christian, Pan, Xinhui,
Deucher, Alexander
[Public]
Got your point. Will send a new patch to address this.
Regards,
Guchun
-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Friday, October 1, 2021 10:29 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
No, scheduler restart and device unlock must take place inamdgpu_pci_resume (see struct pci_error_handlers for the various states of PCI recovery). So just add a flag (probably in amdgpu_device) so we can remember what pci_channel_state_t we came from (unfortunately it's not passed to us in amdgpu_pci_resume) and unless it's set don't do anything in amdgpu_pci_resume.
Andrey
On 2021-10-01 4:21 a.m., Chen, Guchun wrote:
> [Public]
>
> Hi Andrey,
>
> Do you mean to move the code of drm_sched_resubmit_jobs and drm_sched_start in amdgpu_pci_resume to amdgpu_pci_error_detected, under the case pci_channel_io_frozen?
> Then leave amdgpu_pci_resume as a null function, and in this way, we can drop the acquire/lock write lock for case of pci_channel_io_normal as well?
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Sent: Friday, October 1, 2021 10:22 AM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org;
> Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui
> <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci
> detected state pci_channel_io_normal
>
> On 2021-09-30 10:00 p.m., Guchun Chen wrote:
>
>> 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.
>
> Good catch but, the issue is even wider in scope, what about drm_sched_resubmit_jobs and drm_sched_start called without being stopped before ? Better to put the entire scope of code in this function under flag that set only in pci_channel_io_frozen. As far as i remember we don't need to do anything in case of pci_channel_io_normal.
>
> Andrey
>
>
>> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index bb5ad2b6ca13..12f822d51de2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -5370,6 +5370,7 @@ pci_ers_result_t
>> amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>>
>> switch (state) {
>> case pci_channel_io_normal:
>> + amdgpu_device_lock_adev(adev, NULL);
>> return PCI_ERS_RESULT_CAN_RECOVER;
>> /* Fatal error, prepare for slot reset */
>> case pci_channel_io_frozen:
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
2021-10-01 15:21 ` Chen, Guchun
@ 2021-10-02 15:20 ` Chen, Guchun
0 siblings, 0 replies; 6+ messages in thread
From: Chen, Guchun @ 2021-10-02 15:20 UTC (permalink / raw)
To: Grodzovsky, Andrey, amd-gfx, Koenig, Christian, Pan, Xinhui,
Deucher, Alexander
[Public]
Hi Andrey,
A new patch with subject "drm/amdgpu: handle the case of pci_channel_io_frozen only in amdgpu_pci_resume" has been sent, pls review it. Thanks.
Regards,
Guchun
-----Original Message-----
From: Chen, Guchun
Sent: Friday, October 1, 2021 11:21 PM
To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: RE: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
[Public]
Got your point. Will send a new patch to address this.
Regards,
Guchun
-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Friday, October 1, 2021 10:29 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal
No, scheduler restart and device unlock must take place inamdgpu_pci_resume (see struct pci_error_handlers for the various states of PCI recovery). So just add a flag (probably in amdgpu_device) so we can remember what pci_channel_state_t we came from (unfortunately it's not passed to us in amdgpu_pci_resume) and unless it's set don't do anything in amdgpu_pci_resume.
Andrey
On 2021-10-01 4:21 a.m., Chen, Guchun wrote:
> [Public]
>
> Hi Andrey,
>
> Do you mean to move the code of drm_sched_resubmit_jobs and drm_sched_start in amdgpu_pci_resume to amdgpu_pci_error_detected, under the case pci_channel_io_frozen?
> Then leave amdgpu_pci_resume as a null function, and in this way, we can drop the acquire/lock write lock for case of pci_channel_io_normal as well?
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Sent: Friday, October 1, 2021 10:22 AM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org;
> Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui
> <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: add missed write lock for pci
> detected state pci_channel_io_normal
>
> On 2021-09-30 10:00 p.m., Guchun Chen wrote:
>
>> 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.
>
> Good catch but, the issue is even wider in scope, what about drm_sched_resubmit_jobs and drm_sched_start called without being stopped before ? Better to put the entire scope of code in this function under flag that set only in pci_channel_io_frozen. As far as i remember we don't need to do anything in case of pci_channel_io_normal.
>
> Andrey
>
>
>> Fixes: c9a6b82f45e2("drm/amdgpu: Implement DPC recovery")
>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index bb5ad2b6ca13..12f822d51de2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -5370,6 +5370,7 @@ pci_ers_result_t
>> amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>>
>> switch (state) {
>> case pci_channel_io_normal:
>> + amdgpu_device_lock_adev(adev, NULL);
>> return PCI_ERS_RESULT_CAN_RECOVER;
>> /* Fatal error, prepare for slot reset */
>> case pci_channel_io_frozen:
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-02 15:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 2:00 [PATCH] drm/amdgpu: add missed write lock for pci detected state pci_channel_io_normal Guchun Chen
2021-10-01 2:21 ` Andrey Grodzovsky
2021-10-01 8:21 ` Chen, Guchun
2021-10-01 14:28 ` Andrey Grodzovsky
2021-10-01 15:21 ` Chen, Guchun
2021-10-02 15:20 ` Chen, Guchun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.