[PATCH] dma-buf/fence-array: enable_signaling from wq

[Rob Clark]

Currently with fence-array, we have a potential deadlock situation.  If we
fence_add_callback() on an array-fence, the array-fence's lock is aquired
first, and in it's ->enable_signaling() callback, it will install cb's on
it's array-member fences, so the array-member's lock is acquired second.

But in the signal path, the array-member's lock is acquired first, and the
array-fence's lock acquired second.

To solve that, punt adding the callbacks on the array member fences to a
worker.

lockdep splat:

 ======================================================
[ INFO: possible circular locking dependency detected ]
4.7.0-rc7+ #489 Not tainted
-------------------------------------------------------
surfaceflinger/2034 is trying to acquire lock:
 (&(&array->lock)->rlock){......}, at: [<ffff00000858cddc>] fence_signal+0x5c/0xf8

but task is already holding lock:
 (&(&obj->child_list_lock)->rlock){......}, at: [<ffff0000085902f8>] sw_sync_ioctl+0x228/0x3b0

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&(&obj->child_list_lock)->rlock){......}:
       [<ffff000008108924>] __lock_acquire+0x173c/0x18d8
       [<ffff000008108e0c>] lock_acquire+0x4c/0x68
       [<ffff000008ac6a6c>] _raw_spin_lock_irqsave+0x54/0x70
       [<ffff00000858d05c>] fence_add_callback+0x3c/0x100
       [<ffff00000858f100>] fence_array_enable_signaling+0x80/0x170
       [<ffff00000858d0d8>] fence_add_callback+0xb8/0x100
       [<ffff00000858f504>] sync_file_poll+0xd4/0xf0
       [<ffff0000081fd3a0>] do_sys_poll+0x220/0x438
       [<ffff0000081fd8d0>] SyS_ppoll+0x1b0/0x1d8
       [<ffff000008084f30>] el0_svc_naked+0x24/0x28

-> #0 (&(&array->lock)->rlock){......}:
       [<ffff000008104768>] print_circular_bug+0x80/0x2e0
       [<ffff0000081089ac>] __lock_acquire+0x17c4/0x18d8
       [<ffff000008108e0c>] lock_acquire+0x4c/0x68
       [<ffff000008ac6a6c>] _raw_spin_lock_irqsave+0x54/0x70
       [<ffff00000858cddc>] fence_signal+0x5c/0xf8
       [<ffff00000858f268>] fence_array_cb_func+0x78/0x88
       [<ffff00000858cb28>] fence_signal_locked+0x80/0xe0
       [<ffff0000085903c8>] sw_sync_ioctl+0x2f8/0x3b0
       [<ffff0000081faf6c>] do_vfs_ioctl+0xa4/0x790
       [<ffff0000081fb6e4>] SyS_ioctl+0x8c/0xa0
       [<ffff000008084f30>] el0_svc_naked+0x24/0x28

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&(&obj->child_list_lock)->rlock);
                               lock(&(&array->lock)->rlock);
                               lock(&(&obj->child_list_lock)->rlock);
  lock(&(&array->lock)->rlock);

 *** DEADLOCK ***

1 lock held by surfaceflinger/2034:
 #0:  (&(&obj->child_list_lock)->rlock){......}, at: [<ffff0000085902f8>] sw_sync_ioctl+0x228/0x3b0

Signed-off-by: Rob Clark <robdclark at gmail.com>
---
 drivers/dma-buf/dma-fence-array.c | 18 ++++++++++++++----
 include/linux/dma-fence-array.h   |  4 ++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 67eb7c8..274bbb5 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f,
 	dma_fence_put(&array->base);
 }
 
-static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
+static void enable_signaling_worker(struct work_struct *w)
 {
-	struct dma_fence_array *array = to_dma_fence_array(fence);
+	struct dma_fence_array *array =
+		container_of(w, struct dma_fence_array, enable_signaling_worker);
 	struct dma_fence_array_cb *cb = (void *)(&array[1]);
 	unsigned i;
 
@@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
 		if (dma_fence_add_callback(array->fences[i], &cb[i].cb,
 					   dma_fence_array_cb_func)) {
 			dma_fence_put(&array->base);
-			if (atomic_dec_and_test(&array->num_pending))
-				return false;
+			if (atomic_dec_and_test(&array->num_pending)) {
+				dma_fence_signal(&array->base);
+				return;
+			}
 		}
 	}
+}
 
+static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
+{
+	struct dma_fence_array *array = to_dma_fence_array(fence);
+	queue_work(system_unbound_wq, &array->enable_signaling_worker);
 	return true;
 }
 
@@ -141,6 +149,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences,
 	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
 	array->fences = fences;
 
+	INIT_WORK(&array->enable_signaling_worker, enable_signaling_worker);
+
 	return array;
 }
 EXPORT_SYMBOL(dma_fence_array_create);
diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h
index 5900945..f48e8f4 100644
--- a/include/linux/dma-fence-array.h
+++ b/include/linux/dma-fence-array.h
@@ -35,6 +35,8 @@ struct dma_fence_array_cb {
 /**
  * struct dma_fence_array - fence to represent an array of fences
  * @base: fence base class
+ * @enable_signaling_worker: &work_struct for deferring enable_signaling
+ *    to context not holding fence->lock
  * @lock: spinlock for fence handling
  * @num_fences: number of fences in the array
  * @num_pending: fences in the array still pending
@@ -43,6 +45,8 @@ struct dma_fence_array_cb {
 struct dma_fence_array {
 	struct dma_fence base;
 
+	struct work_struct enable_signaling_worker;
+
 	spinlock_t lock;
 	unsigned num_fences;
 	atomic_t num_pending;
-- 
2.7.4