sw_sync callback deadlock

sw_sync callback deadlock

[Chris Wilson]

Take 2. dma_fence_parent() relied on fence->lock pointing into the
sync_timeline which is no more, so we need a sync_pt->timeline
backpointer instead.
-Chris


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.

[Chris Wilson]

From: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>

Calltree:
  timeline_fence_release
  drm_sched_entity_wakeup
  dma_fence_signal_locked
  sync_timeline_signal
  sw_sync_ioctl

Releasing the reference to the fence in the fence signal callback
seems reasonable to me, so this patch avoids the locking issue in
sw_sync.

d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists")
fixed the recursive locking issue but caused an use-after-free. Later
d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
fixed the use-after-free but reintroduced the recursive locking issue.

In this attempt we avoid the use-after-free still because the release
function still always locks, and outside of the locking region in the
signal function we have properly refcounted references.

We furthermore also avoid the recurive lock by making sure that either:

1) We have a properly refcounted reference, preventing the signal from
   triggering the release function inside the locked region.
2) The refcount was already zero, and hence nobody will be able to trigger
   the release function from the signal function.

v2: Move dma_fence_signal() into second loop in preparation to moving
the callback out of the timeline obj->lock.

Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/sw_sync.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 348b3a9170fa..807c82148062 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -192,6 +192,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
 static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 {
 	struct sync_pt *pt, *next;
+	LIST_HEAD(signal);
 
 	trace_sync_timeline(obj);
 
@@ -203,21 +204,32 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 		if (!timeline_fence_signaled(&pt->base))
 			break;
 
-		list_del_init(&pt->link);
-		rb_erase(&pt->node, &obj->pt_tree);
-
 		/*
-		 * A signal callback may release the last reference to this
-		 * fence, causing it to be freed. That operation has to be
-		 * last to avoid a use after free inside this loop, and must
-		 * be after we remove the fence from the timeline in order to
-		 * prevent deadlocking on timeline->lock inside
-		 * timeline_fence_release().
+		 * We need to take a reference to avoid a release during
+		 * signalling (which can cause a recursive lock of obj->lock).
+		 * If refcount was already zero, another thread is already
+		 * taking care of destroying the fence.
 		 */
-		dma_fence_signal_locked(&pt->base);
+		if (!dma_fence_get_rcu(&pt->base))
+			continue;
+
+		list_move_tail(&pt->link, &signal);
+		rb_erase(&pt->node, &obj->pt_tree);
 	}
 
 	spin_unlock_irq(&obj->lock);
+
+	list_for_each_entry_safe(pt, next, &signal, link) {
+		/*
+		 * This needs to be cleared before release, otherwise the
+		 * timeline_fence_release function gets confused about also
+		 * removing the fence from the pt_tree.
+		 */
+		list_del_init(&pt->link);
+
+		dma_fence_signal(&pt->base);
+		dma_fence_put(&pt->base);
+	}
 }
 
 /**
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] dma-buf/sw_sync: Separate signal/timeline locks

[Chris Wilson]

Since we decouple the sync_pt from the timeline tree upon release, in
order to allow releasing the sync_pt from a signal callback we need to
separate the sync_pt signaling lock from the timeline tree lock.

v2: Mark up the unlocked read of the current timeline value.
v3: Store a timeline pointer in the sync_pt as we cannot use the common
fence->lock trick to find our parent anymore.

Suggested-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
---
 drivers/dma-buf/sw_sync.c    | 40 +++++++++++++++++++++---------------
 drivers/dma-buf/sync_debug.c |  2 +-
 drivers/dma-buf/sync_debug.h | 13 +++++++-----
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 807c82148062..17a5c1a3b7ce 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -123,33 +123,39 @@ static const char *timeline_fence_get_driver_name(struct dma_fence *fence)
 
 static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
 {
-	struct sync_timeline *parent = dma_fence_parent(fence);
-
-	return parent->name;
+	return sync_timeline(fence)->name;
 }
 
 static void timeline_fence_release(struct dma_fence *fence)
 {
 	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
-	struct sync_timeline *parent = dma_fence_parent(fence);
-	unsigned long flags;
+	struct sync_timeline *parent = pt->timeline;
 
-	spin_lock_irqsave(fence->lock, flags);
 	if (!list_empty(&pt->link)) {
-		list_del(&pt->link);
-		rb_erase(&pt->node, &parent->pt_tree);
+		unsigned long flags;
+
+		spin_lock_irqsave(&parent->lock, flags);
+		if (!list_empty(&pt->link)) {
+			list_del(&pt->link);
+			rb_erase(&pt->node, &parent->pt_tree);
+		}
+		spin_unlock_irqrestore(&parent->lock, flags);
 	}
-	spin_unlock_irqrestore(fence->lock, flags);
 
 	sync_timeline_put(parent);
 	dma_fence_free(fence);
 }
 
-static bool timeline_fence_signaled(struct dma_fence *fence)
+static int timeline_value(struct dma_fence *fence)
 {
-	struct sync_timeline *parent = dma_fence_parent(fence);
+	return READ_ONCE(sync_timeline(fence)->value);
+}
 
-	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
+static bool timeline_fence_signaled(struct dma_fence *fence)
+{
+	return !__dma_fence_is_later(fence->seqno,
+				     timeline_value(fence),
+				     fence->ops);
 }
 
 static bool timeline_fence_enable_signaling(struct dma_fence *fence)
@@ -166,9 +172,7 @@ static void timeline_fence_value_str(struct dma_fence *fence,
 static void timeline_fence_timeline_value_str(struct dma_fence *fence,
 					     char *str, int size)
 {
-	struct sync_timeline *parent = dma_fence_parent(fence);
-
-	snprintf(str, size, "%d", parent->value);
+	snprintf(str, size, "%d", timeline_value(fence));
 }
 
 static const struct dma_fence_ops timeline_fence_ops = {
@@ -252,12 +256,14 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 		return NULL;
 
 	sync_timeline_get(obj);
-	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
+	spin_lock_init(&pt->lock);
+	dma_fence_init(&pt->base, &timeline_fence_ops, &pt->lock,
 		       obj->context, value);
 	INIT_LIST_HEAD(&pt->link);
+	pt->timeline = obj;
 
 	spin_lock_irq(&obj->lock);
-	if (!dma_fence_is_signaled_locked(&pt->base)) {
+	if (!dma_fence_is_signaled(&pt->base)) {
 		struct rb_node **p = &obj->pt_tree.rb_node;
 		struct rb_node *parent = NULL;
 
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 101394f16930..2188ee17e889 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -65,7 +65,7 @@ static const char *sync_status_str(int status)
 static void sync_print_fence(struct seq_file *s,
 			     struct dma_fence *fence, bool show)
 {
-	struct sync_timeline *parent = dma_fence_parent(fence);
+	struct sync_timeline *parent = sync_timeline(fence);
 	int status;
 
 	status = dma_fence_get_status_locked(fence);
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 6176e52ba2d7..56589dae2159 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -45,23 +45,26 @@ struct sync_timeline {
 	struct list_head	sync_timeline_list;
 };
 
-static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
-{
-	return container_of(fence->lock, struct sync_timeline, lock);
-}
-
 /**
  * struct sync_pt - sync_pt object
  * @base: base fence object
  * @link: link on the sync timeline's list
  * @node: node in the sync timeline's tree
+ * @lock: fence signaling lock
  */
 struct sync_pt {
 	struct dma_fence base;
+	struct sync_timeline *timeline;
 	struct list_head link;
 	struct rb_node node;
+	spinlock_t lock;
 };
 
+static inline struct sync_timeline *sync_timeline(struct dma_fence *fence)
+{
+	return container_of(fence, struct sync_pt, base)->timeline;
+}
+
 extern const struct file_operations sw_sync_debugfs_fops;
 
 void sync_timeline_debug_add(struct sync_timeline *obj);
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] dma-buf/selftests: Add locking selftests for sw_sync

[Chris Wilson]

While sw_sync is purely a debug facility for userspace to create fences
and timelines it can control, nevertheless it has some tricky locking
semantics of its own. In particular, Bas Nieuwenhuizen reported that we
had reintroduced a deadlock if a signal callback attempted to destroy
the fence. So let's add a few trivial selftests to make sure that once
fixed again, it stays fixed.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
---
 drivers/dma-buf/Makefile     |   3 +-
 drivers/dma-buf/selftests.h  |   1 +
 drivers/dma-buf/st-sw_sync.c | 279 +++++++++++++++++++++++++++++++++++
 drivers/dma-buf/sw_sync.c    |  39 +++++
 drivers/dma-buf/sync_debug.h |   8 +
 5 files changed, 329 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma-buf/st-sw_sync.c

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 995e05f609ff..9be4d4611609 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF)		+= udmabuf.o
 dmabuf_selftests-y := \
 	selftest.o \
 	st-dma-fence.o \
-	st-dma-fence-chain.o
+	st-dma-fence-chain.o \
+	st-sw_sync.o
 
 obj-$(CONFIG_DMABUF_SELFTESTS)	+= dmabuf_selftests.o
diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h
index bc8cea67bf1e..232499a24872 100644
--- a/drivers/dma-buf/selftests.h
+++ b/drivers/dma-buf/selftests.h
@@ -12,3 +12,4 @@
 selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */
 selftest(dma_fence, dma_fence)
 selftest(dma_fence_chain, dma_fence_chain)
+selftest(sw_sync, sw_sync)
diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c
new file mode 100644
index 000000000000..145fd330f1c6
--- /dev/null
+++ b/drivers/dma-buf/st-sw_sync.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include <linux/delay.h>
+#include <linux/dma-fence.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/sched/signal.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "sync_debug.h"
+#include "selftest.h"
+
+static int sanitycheck(void *arg)
+{
+	struct sync_timeline *tl;
+	struct dma_fence *f;
+	int err = -ENOMEM;
+
+	/* Quick check we can create the timeline and syncpt */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	f = st_sync_pt_create(tl, 1);
+	if (!f)
+		goto out;
+
+	dma_fence_signal(f);
+	dma_fence_put(f);
+
+	err = 0;
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+static int signal(void *arg)
+{
+	struct sync_timeline *tl;
+	struct dma_fence *f;
+	int err = -EINVAL;
+
+	/* Check that the syncpt fence is signaled when the timeline advances */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	f = st_sync_pt_create(tl, 1);
+	if (!f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (dma_fence_is_signaled(f)) {
+		pr_err("syncpt:%lld signaled too early\n", f->seqno);
+		goto out_fence;
+	}
+
+	st_sync_timeline_signal(tl, 1);
+
+	if (!dma_fence_is_signaled(f)) {
+		pr_err("syncpt:%lld not signaled after increment\n", f->seqno);
+		goto out_fence;
+	}
+
+	err = 0;
+out_fence:
+	dma_fence_signal(f);
+	dma_fence_put(f);
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+struct cb_destroy {
+	struct dma_fence_cb cb;
+	struct dma_fence *f;
+};
+
+static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb)
+{
+	struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb);
+
+	pr_info("syncpt:%llx destroying syncpt:%llx\n",
+		fence->seqno, cb->f->seqno);
+	dma_fence_put(cb->f);
+	cb->f = NULL;
+}
+
+static int cb_autodestroy(void *arg)
+{
+	struct sync_timeline *tl;
+	struct cb_destroy cb;
+	int err = -EINVAL;
+
+	/* Check that we can drop the final syncpt reference from a callback */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	cb.f = st_sync_pt_create(tl, 1);
+	if (!cb.f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (dma_fence_add_callback(cb.f, &cb.cb, cb_destroy)) {
+		pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno);
+		goto out;
+	}
+
+	st_sync_timeline_signal(tl, 1);
+	if (cb.f) {
+		pr_err("syncpt:%lld callback not run\n", cb.f->seqno);
+		dma_fence_put(cb.f);
+		goto out;
+	}
+
+	err = 0;
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+static int cb_destroy_12(void *arg)
+{
+	struct sync_timeline *tl;
+	struct cb_destroy cb;
+	struct dma_fence *f;
+	int err = -EINVAL;
+
+	/* Check that we can drop some other syncpt reference from a callback */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	f = st_sync_pt_create(tl, 1);
+	if (!f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	cb.f = st_sync_pt_create(tl, 2);
+	if (!cb.f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
+		pr_err("syncpt:%lld signaled before increment\n", f->seqno);
+		goto out;
+	}
+
+	st_sync_timeline_signal(tl, 1);
+	if (cb.f) {
+		pr_err("syncpt:%lld callback not run\n", f->seqno);
+		dma_fence_put(cb.f);
+		goto out_fence;
+	}
+
+	err = 0;
+out_fence:
+	dma_fence_put(f);
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+static int cb_destroy_21(void *arg)
+{
+	struct sync_timeline *tl;
+	struct cb_destroy cb;
+	struct dma_fence *f;
+	int err = -EINVAL;
+
+	/* Check that we can drop an earlier syncpt reference from a callback */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	cb.f = st_sync_pt_create(tl, 1);
+	if (!cb.f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	f = st_sync_pt_create(tl, 2);
+	if (!f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
+		pr_err("syncpt:%lld signaled before increment\n", f->seqno);
+		goto out;
+	}
+
+	st_sync_timeline_signal(tl, 2);
+	if (cb.f) {
+		pr_err("syncpt:%lld callback not run\n", f->seqno);
+		dma_fence_put(cb.f);
+		goto out_fence;
+	}
+
+	err = 0;
+out_fence:
+	dma_fence_put(f);
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+static int cb_destroy_22(void *arg)
+{
+	struct sync_timeline *tl;
+	struct cb_destroy cb;
+	struct dma_fence *f;
+	int err = -EINVAL;
+
+	/* Check that we can drop the later syncpt reference from a callback */
+
+	tl = st_sync_timeline_create("mock");
+	if (!tl)
+		return -ENOMEM;
+
+	f = st_sync_pt_create(tl, 1);
+	if (!f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	cb.f = st_sync_pt_create(tl, 2);
+	if (!cb.f) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
+		pr_err("syncpt:%lld signaled before increment\n", f->seqno);
+		goto out;
+	}
+
+	st_sync_timeline_signal(tl, 2);
+	if (cb.f) {
+		pr_err("syncpt:%lld callback not run\n", f->seqno);
+		dma_fence_put(cb.f);
+		goto out_fence;
+	}
+
+	err = 0;
+out_fence:
+	dma_fence_put(f);
+out:
+	st_sync_timeline_put(tl);
+	return err;
+}
+
+int sw_sync(void)
+{
+	static const struct subtest tests[] = {
+		SUBTEST(sanitycheck),
+		SUBTEST(signal),
+		SUBTEST(cb_autodestroy),
+		SUBTEST(cb_destroy_12),
+		SUBTEST(cb_destroy_21),
+		SUBTEST(cb_destroy_22),
+	};
+
+	return subtests(tests, NULL);
+}
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 17a5c1a3b7ce..f16b6c476eef 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -428,3 +428,42 @@ const struct file_operations sw_sync_debugfs_fops = {
 	.unlocked_ioctl = sw_sync_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 };
+
+#if IS_ENABLED(CONFIG_DMABUF_SELFTESTS)
+struct sync_timeline *st_sync_timeline_create(const char *name)
+{
+	return sync_timeline_create(name);
+}
+EXPORT_SYMBOL_GPL(st_sync_timeline_create);
+
+void st_sync_timeline_get(struct sync_timeline *tl)
+{
+	sync_timeline_get(tl);
+}
+EXPORT_SYMBOL_GPL(st_sync_timeline_get);
+
+void st_sync_timeline_put(struct sync_timeline *tl)
+{
+	sync_timeline_put(tl);
+}
+EXPORT_SYMBOL_GPL(st_sync_timeline_put);
+
+void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc)
+{
+	sync_timeline_signal(tl, inc);
+}
+EXPORT_SYMBOL_GPL(st_sync_timeline_signal);
+
+struct dma_fence *
+st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno)
+{
+	struct sync_pt *pt;
+
+	pt = sync_pt_create(tl, seqno);
+	if (!pt)
+		return NULL;
+
+	return &pt->base;
+}
+EXPORT_SYMBOL_GPL(st_sync_pt_create);
+#endif
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 56589dae2159..546a11e564a7 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -72,4 +72,12 @@ void sync_timeline_debug_remove(struct sync_timeline *obj);
 void sync_file_debug_add(struct sync_file *fence);
 void sync_file_debug_remove(struct sync_file *fence);
 
+struct sync_timeline *st_sync_timeline_create(const char *name);
+void st_sync_timeline_get(struct sync_timeline *tl);
+void st_sync_timeline_put(struct sync_timeline *tl);
+void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc);
+
+struct dma_fence *
+st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno);
+
 #endif /* _LINUX_SYNC_H */
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] dma-buf/sw_sync: Separate signal/timeline locks

[Bas Nieuwenhuizen]

Still Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>

On Tue, Jul 14, 2020 at 11:24 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Since we decouple the sync_pt from the timeline tree upon release, in
> order to allow releasing the sync_pt from a signal callback we need to
> separate the sync_pt signaling lock from the timeline tree lock.
>
> v2: Mark up the unlocked read of the current timeline value.
> v3: Store a timeline pointer in the sync_pt as we cannot use the common
> fence->lock trick to find our parent anymore.
>
> Suggested-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> ---
>  drivers/dma-buf/sw_sync.c    | 40 +++++++++++++++++++++---------------
>  drivers/dma-buf/sync_debug.c |  2 +-
>  drivers/dma-buf/sync_debug.h | 13 +++++++-----
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 807c82148062..17a5c1a3b7ce 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -123,33 +123,39 @@ static const char *timeline_fence_get_driver_name(struct dma_fence *fence)
>
>  static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
>  {
> -       struct sync_timeline *parent = dma_fence_parent(fence);
> -
> -       return parent->name;
> +       return sync_timeline(fence)->name;
>  }
>
>  static void timeline_fence_release(struct dma_fence *fence)
>  {
>         struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> -       struct sync_timeline *parent = dma_fence_parent(fence);
> -       unsigned long flags;
> +       struct sync_timeline *parent = pt->timeline;
>
> -       spin_lock_irqsave(fence->lock, flags);
>         if (!list_empty(&pt->link)) {
> -               list_del(&pt->link);
> -               rb_erase(&pt->node, &parent->pt_tree);
> +               unsigned long flags;
> +
> +               spin_lock_irqsave(&parent->lock, flags);
> +               if (!list_empty(&pt->link)) {
> +                       list_del(&pt->link);
> +                       rb_erase(&pt->node, &parent->pt_tree);
> +               }
> +               spin_unlock_irqrestore(&parent->lock, flags);
>         }
> -       spin_unlock_irqrestore(fence->lock, flags);
>
>         sync_timeline_put(parent);
>         dma_fence_free(fence);
>  }
>
> -static bool timeline_fence_signaled(struct dma_fence *fence)
> +static int timeline_value(struct dma_fence *fence)
>  {
> -       struct sync_timeline *parent = dma_fence_parent(fence);
> +       return READ_ONCE(sync_timeline(fence)->value);
> +}
>
> -       return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
> +static bool timeline_fence_signaled(struct dma_fence *fence)
> +{
> +       return !__dma_fence_is_later(fence->seqno,
> +                                    timeline_value(fence),
> +                                    fence->ops);
>  }
>
>  static bool timeline_fence_enable_signaling(struct dma_fence *fence)
> @@ -166,9 +172,7 @@ static void timeline_fence_value_str(struct dma_fence *fence,
>  static void timeline_fence_timeline_value_str(struct dma_fence *fence,
>                                              char *str, int size)
>  {
> -       struct sync_timeline *parent = dma_fence_parent(fence);
> -
> -       snprintf(str, size, "%d", parent->value);
> +       snprintf(str, size, "%d", timeline_value(fence));
>  }
>
>  static const struct dma_fence_ops timeline_fence_ops = {
> @@ -252,12 +256,14 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
>                 return NULL;
>
>         sync_timeline_get(obj);
> -       dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
> +       spin_lock_init(&pt->lock);
> +       dma_fence_init(&pt->base, &timeline_fence_ops, &pt->lock,
>                        obj->context, value);
>         INIT_LIST_HEAD(&pt->link);
> +       pt->timeline = obj;
>
>         spin_lock_irq(&obj->lock);
> -       if (!dma_fence_is_signaled_locked(&pt->base)) {
> +       if (!dma_fence_is_signaled(&pt->base)) {
>                 struct rb_node **p = &obj->pt_tree.rb_node;
>                 struct rb_node *parent = NULL;
>
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 101394f16930..2188ee17e889 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -65,7 +65,7 @@ static const char *sync_status_str(int status)
>  static void sync_print_fence(struct seq_file *s,
>                              struct dma_fence *fence, bool show)
>  {
> -       struct sync_timeline *parent = dma_fence_parent(fence);
> +       struct sync_timeline *parent = sync_timeline(fence);
>         int status;
>
>         status = dma_fence_get_status_locked(fence);
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 6176e52ba2d7..56589dae2159 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -45,23 +45,26 @@ struct sync_timeline {
>         struct list_head        sync_timeline_list;
>  };
>
> -static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
> -{
> -       return container_of(fence->lock, struct sync_timeline, lock);
> -}
> -
>  /**
>   * struct sync_pt - sync_pt object
>   * @base: base fence object
>   * @link: link on the sync timeline's list
>   * @node: node in the sync timeline's tree
> + * @lock: fence signaling lock
>   */
>  struct sync_pt {
>         struct dma_fence base;
> +       struct sync_timeline *timeline;
>         struct list_head link;
>         struct rb_node node;
> +       spinlock_t lock;
>  };
>
> +static inline struct sync_timeline *sync_timeline(struct dma_fence *fence)
> +{
> +       return container_of(fence, struct sync_pt, base)->timeline;
> +}
> +
>  extern const struct file_operations sw_sync_debugfs_fops;
>
>  void sync_timeline_debug_add(struct sync_timeline *obj);
> --
> 2.20.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel