Re: [BUG] gpu: drm: radeon: two possible deadlocks involving locking and waiting

[Jia-Ju Bai]

Hi Christian,

Thanks for the reply :)

On 2022/2/1 15:56, Christian König wrote:
> Hi Jia-Ju,
>
> interesting that you have found those issues with an automated tool.
>
> And yes that is a well design flaw within the radeon driver which can 
> happen on hardware faults, e.g. when radeon_ring_backup() needs to be 
> called.

In fact, my tool finds dozens of similar possible deadlocks caused by 
wait_event_timeout() in radeon_fence_wait_seq_timeout().
There are three other examples in Linux 5.16:

#BUG 1
radeon_dpm_change_power_state_locked()
  mutex_lock(&rdev->ring_lock); --> Line 1133 (Lock A)
  radeon_fence_wait_empty()
    radeon_fence_wait_seq_timeout()
      wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X)

radeon_fence_driver_fini()
  mutex_lock(&rdev->ring_lock); --> Line 917 (Lock A)
  wake_up_all(&rdev->fence_queue); --> Line 927 (Wake X)

#BUG 2
radeon_set_pm_profile()
  mutex_lock(&rdev->pm.mutex); --> Line 382 (Lock A)
  radeon_pm_set_clocks()
    radeon_fence_wait_empty()
      radeon_fence_wait_seq_timeout()
        wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X)

radeon_dynpm_idle_work_handler()
  mutex_lock(&rdev->pm.mutex); --> Line 1861 (Lock A)
  radeon_fence_count_emitted()
    radeon_fence_process()
      wake_up_all(&rdev->fence_queue); --> Line 323 (Wake X)

#BUG 3
radeon_pm_fini_old()
  mutex_lock(&rdev->pm.mutex); --> Line 1642 (Lock A)
  radeon_pm_set_clocks()
    radeon_fence_wait_empty()
      radeon_fence_wait_seq_timeout()
        wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X)

radeon_dynpm_idle_work_handler()
  mutex_lock(&rdev->pm.mutex); --> Line 1861 (Lock A)
  radeon_fence_count_emitted()
    radeon_fence_process()
      wake_up_all(&rdev->fence_queue); --> Line 323 (Wake X)

Thus, to fix these possible deadlocks, we could moditify the code 
related to radeon_fence_wait_seq_timeout().
But I am not quite familar with the radeon driver, so I am not sure how 
to moditify the code properly.

> But that happens so rarely and the driver is not developed further 
> that we decided to not address this any more.

Ah, okay.

> Regards,
> Christian.
>
> Am 01.02.22 um 08:40 schrieb Jia-Ju Bai:
> > Hello,
> >
> > My static analysis tool reports a possible deadlock in the radeon 
> > driver in Linux 5.16:
> >
> > #BUG 1
> > radeon_dpm_change_power_state_locked()
> >   mutex_lock(&rdev->ring_lock); --> Line 1133 (Lock A)
> >   radeon_fence_wait_empty()
> >     radeon_fence_wait_seq_timeout()
> >       wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X)
> >
> > radeon_ring_backup()
> >   mutex_lock(&rdev->ring_lock); --> Line 289(Lock A)
> >   radeon_fence_count_emitted()
> >     radeon_fence_process()
> >       wake_up_all(&rdev->fence_queue); --> Line 323 (Wake X)
> >
> > When radeon_dpm_change_power_state_locked() is executed, "Wait X" is 
> > performed by holding "Lock A". If radeon_ring_backup() is executed at 
> > this time, "Wake X" cannot be performed to wake up "Wait X" in 
> > radeon_dpm_change_power_state_locked(), because "Lock A" has been 
> > already hold by radeon_dpm_change_power_state_locked(), causing a 
> > possible deadlock.
> > I find that "Wait X" is performed with a timeout 
> > MAX_SCHEDULE_TIMEOUT, to relieve the possible deadlock; but I think 
> > this timeout can cause inefficient execution.
> >
> > #BUG 2
> > radeon_ring_lock()
> >   mutex_lock(&rdev->ring_lock); --> Line 147 (Lock A)
> >   radeon_ring_alloc()
> >     radeon_fence_wait_next()
> >       radeon_fence_wait_seq_timeout()
> >         wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X)
> >
> > radeon_ring_backup()
> >   mutex_lock(&rdev->ring_lock); --> Line 289(Lock A)
> >   radeon_fence_count_emitted()
> >     radeon_fence_process()
> >       wake_up_all(&rdev->fence_queue); --> Line 323 (Wake X)
> >
> > When radeon_ring_lock() is executed, "Wait X" is performed by holding 
> > "Lock A". If radeon_ring_backup() is executed at this time, "Wake X" 
> > cannot be performed to wake up "Wait X" in radeon_ring_lock(), 
> > because "Lock A" has been already hold by radeon_ring_lock(), causing 
> > a possible deadlock.
> > I find that "Wait X" is performed with a timeout 
> > MAX_SCHEDULE_TIMEOUT, to relieve the possible deadlock; but I think 
> > this timeout can cause inefficient execution.
> >
> > I am not quite sure whether these possible problems are real and how 
> > to fix them if they are real.
> > Any feedback would be appreciated, thanks :)
> >
> >
> > Best wishes,
> > Jia-Ju Bai