From b6c5beaaf0b154b9ae9a9dec5daa91b40ee86941 Mon Sep 17 00:00:00 2001 From: Puranam V G Tejaswi Date: Tue, 27 Apr 2021 13:50:51 +0530 Subject: [PATCH] msm: kgsl: Fix spinlock recursion issues Currently there is a possibility of spinlock recursion in kgsl_timeline_signal and kgsl_ioctl_timeline_destroy. The issue can happen if two fences are created on a timeline and KGSL_TIMELINE_WAIT_ANY flag is used. First fence is signalled and once the wait is complete, second fence is signalled. This results in a spinlock recursion as the release of first fence is called when the timeline->lock is held. Fix this by making sure that the lock to protect the list and lock used for dma_fence signalling are used correctly. Also make fence_lock per timeline to reduce lock contention among different timelines. Change-Id: I547d62f29d4407fceca9bd766d8e9f93a88b5c6f Signed-off-by: Puranam V G Tejaswi --- drivers/gpu/msm/kgsl_timeline.c | 63 +++++++++++---------------------- drivers/gpu/msm/kgsl_timeline.h | 4 ++- 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/drivers/gpu/msm/kgsl_timeline.c b/drivers/gpu/msm/kgsl_timeline.c index 0c9be32d9b90..afa0d66fdb3d 100644 --- a/drivers/gpu/msm/kgsl_timeline.c +++ b/drivers/gpu/msm/kgsl_timeline.c @@ -14,8 +14,6 @@ #include "kgsl_timeline.h" #include "kgsl_trace.h" -static DEFINE_SPINLOCK(fence_lock); - struct kgsl_timeline_fence { struct dma_fence base; struct kgsl_timeline *timeline; @@ -152,6 +150,7 @@ static struct kgsl_timeline *kgsl_timeline_alloc(struct kgsl_device_private *dev trace_kgsl_timeline_alloc(id, initial); spin_lock_init(&timeline->lock); + spin_lock_init(&timeline->fence_lock); kref_init(&timeline->ref); @@ -170,8 +169,7 @@ static void timeline_fence_release(struct dma_fence *fence) struct kgsl_timeline_fence *cur, *temp; unsigned long flags; - spin_lock_irqsave(&timeline->lock, flags); - spin_lock(&fence_lock); + spin_lock_irqsave(&timeline->fence_lock, flags); /* If the fence is still on the active list, remove it */ list_for_each_entry_safe(cur, temp, &timeline->fences, node) { @@ -181,8 +179,7 @@ static void timeline_fence_release(struct dma_fence *fence) list_del_init(&f->node); break; } - spin_unlock(&fence_lock); - spin_unlock_irqrestore(&timeline->lock, flags); + spin_unlock_irqrestore(&timeline->fence_lock, flags); trace_kgsl_timeline_fence_release(f->timeline->id, fence->seqno); kgsl_timeline_put(f->timeline); @@ -243,17 +240,17 @@ static void kgsl_timeline_add_fence(struct kgsl_timeline *timeline, struct kgsl_timeline_fence *entry; unsigned long flags; - spin_lock_irqsave(&fence_lock, flags); + spin_lock_irqsave(&timeline->fence_lock, flags); list_for_each_entry(entry, &timeline->fences, node) { if (fence->base.seqno < entry->base.seqno) { list_add_tail(&fence->node, &entry->node); - spin_unlock_irqrestore(&fence_lock, flags); + spin_unlock_irqrestore(&timeline->fence_lock, flags); return; } } list_add_tail(&fence->node, &timeline->fences); - spin_unlock_irqrestore(&fence_lock, flags); + spin_unlock_irqrestore(&timeline->fence_lock, flags); } void kgsl_timeline_signal(struct kgsl_timeline *timeline, u64 seqno) @@ -272,22 +269,18 @@ void kgsl_timeline_signal(struct kgsl_timeline *timeline, u64 seqno) timeline->value = seqno; - /* Copy the list out so we can walk it without holding the lock */ - spin_lock(&fence_lock); - list_replace_init(&timeline->fences, &temp); - spin_unlock(&fence_lock); - - list_for_each_entry_safe(fence, tmp, &temp, node) { + spin_lock(&timeline->fence_lock); + list_for_each_entry_safe(fence, tmp, &timeline->fences, node) { if (timeline_fence_signaled(&fence->base)) { - list_del_init(&fence->node); - dma_fence_signal_locked(&fence->base); + dma_fence_get(&fence->base); + list_move(&fence->node, &temp); } } + spin_unlock(&timeline->fence_lock); - /* Put the active fences back in the timeline list */ list_for_each_entry_safe(fence, tmp, &temp, node) { - list_del_init(&fence->node); - kgsl_timeline_add_fence(timeline, fence); + dma_fence_signal_locked(&fence->base); + dma_fence_put(&fence->base); } unlock: @@ -553,33 +546,19 @@ long kgsl_ioctl_timeline_destroy(struct kgsl_device_private *dev_priv, INIT_LIST_HEAD(&temp); - spin_lock_irq(&timeline->lock); - - /* Copy any still pending fences to a temporary list */ - spin_lock(&fence_lock); - list_replace_init(&timeline->fences, &temp); - spin_unlock(&fence_lock); - - /* - * Set an error on each still pending fence and signal - * them to release any callbacks. Hold the refcount - * to avoid fence getting destroyed during signaling. - */ - list_for_each_entry_safe(fence, tmp, &temp, node) { + spin_lock(&timeline->fence_lock); + list_for_each_entry_safe(fence, tmp, &timeline->fences, node) dma_fence_get(&fence->base); + list_replace_init(&timeline->fences, &temp); + spin_unlock(&timeline->fence_lock); + + spin_lock_irq(&timeline->lock); + list_for_each_entry_safe(fence, tmp, &temp, node) { dma_fence_set_error(&fence->base, -ENOENT); dma_fence_signal_locked(&fence->base); - } - spin_unlock_irq(&timeline->lock); - - /* - * Put the fence refcount taken above outside lock - * to avoid spinlock recursion during fence release. - */ - list_for_each_entry_safe(fence, tmp, &temp, node) { - list_del_init(&fence->node); dma_fence_put(&fence->base); } + spin_unlock_irq(&timeline->lock); kgsl_timeline_put(timeline); diff --git a/drivers/gpu/msm/kgsl_timeline.h b/drivers/gpu/msm/kgsl_timeline.h index 4392833f2a04..987b85fcc03a 100644 --- a/drivers/gpu/msm/kgsl_timeline.h +++ b/drivers/gpu/msm/kgsl_timeline.h @@ -16,7 +16,9 @@ struct kgsl_timeline { int id; /** @value: Current value of the timeline */ u64 value; - /** @lock: Lock to protect @fences */ + /** @fence_lock: Lock to protect @fences */ + spinlock_t fence_lock; + /** @lock: Lock to use for locking each fence in @fences */ spinlock_t lock; /** @ref: Reference count for the struct */ struct kref ref;