On Wed, Jun 02, 2021 at 02:35:28PM +0530, Anshuman Gupta wrote: > On 2021-05-26 at 20:07:28 +0530, Imre Deak wrote: > > It's possible that an already dequeued put_async_work() will release the > > reference (*) that was put asynchronously after the dequeue happened. > > This leaves an async-put work pending, without any reference to release. > > A subsequent async-put may trigger the > > > > drm_WARN_ON(!queue_delayed_work(&power_domains->async_put_work)); > > > > warn due to async_put_work() still pending. To avoid the warn, cancel > > the pending async_put_work() when releasing the reference at (*) above. > > Not able to visulize the race here between __intel_display_power_put_async > and intel_display_power_put_async_work() considering both are protected by > power_domains->lock. > > queue_delayed_work_on() documentation says following about return value. > "Return: %false if @work was already on a queue, %true otherwise." > AFAIU from the doc, queue_delayed_work will return false only when > work was in queue after dequeued put_async_work() it should return true ? Yes, and so the WARN is triggered when __intel_display_power_put_async() tries to queue a work when one is already pending in the queue: 1. get(A) 2. put_async(A) -> queues put_async_work() 3. put_async_work() dequeued, blocking on power_domains->lock 4. get(A) -> grab_async_put_ref(), reacquires the ref that was put in 2. 5. put_async(A) -> queues put_async_work() 6. put_async_work() dequeued in 3. unblocks, releases the ref that was put in 5., put_async_work() queued in 5. still pending in the queue, without any reference to release. 7. get(A) 8. put_async(A) -> tries to queue put_async_work(), but it's already pending -> WARN triggers. The patch avoids the WARN in 8 by cancelling the queued work at 6. > Thanks, > Anshuman Gupta. > > > > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/3421 > > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/2289 > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 2f7d1664c4738..a95bbf23e6b7c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -2263,6 +2263,12 @@ intel_display_power_put_async_work(struct work_struct *work) > > fetch_and_zero(&power_domains->async_put_domains[1]); > > queue_async_put_domains_work(power_domains, > > fetch_and_zero(&new_work_wakeref)); > > + } else { > > + /* > > + * Cancel the work that got queued after this one got dequeued, > > + * since here we released the corresponding async-put reference. > > + */ > > + cancel_delayed_work(&power_domains->async_put_work); > > } > > > > out_verify: > > -- > > 2.27.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
