On 11/30/21 13:19, Thomas Hellström wrote:
The locking order for taking two fence locks is implicitly defined in
at least two ways in the code:
1) Fence containers first and other fences next, which is defined by
the enable_signaling() callbacks of dma_fence_chain and
dma_fence_array.
2) Reverse signal order, which is used by __i915_active_fence_set().
Now 1) implies 2), except for the signal_on_any mode of dma_fence_array
and 2) does not imply 1), and also 1) makes locking order between
different containers confusing.
Establish 2) and fix up the signal_on_any mode by calling
enable_signaling() on such fences unlocked at creation.
Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: Christian König <christian.koenig@xxxxxxx>
Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
---
drivers/dma-buf/dma-fence-array.c | 13 +++--
drivers/dma-buf/dma-fence-chain.c | 3 +-
drivers/dma-buf/dma-fence.c | 79 +++++++++++++++++++++----------
include/linux/dma-fence.h | 3 ++
4 files changed, 69 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 3e07f961e2f3..0322b92909fe 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -84,8 +84,8 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
* insufficient).
*/
dma_fence_get(&array->base);
- if (dma_fence_add_callback(array->fences[i], &cb[i].cb,
- dma_fence_array_cb_func)) {
+ if (dma_fence_add_callback_nested(array->fences[i], &cb[i].cb,
+ dma_fence_array_cb_func)) {
int error = array->fences[i]->error;
dma_fence_array_set_pending_error(array, error);
@@ -158,6 +158,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences,
{
struct dma_fence_array *array;
size_t size = sizeof(*array);
+ struct dma_fence *fence;
/* Allocate the callback structures behind the array. */
size += num_fences * sizeof(struct dma_fence_array_cb);
@@ -165,8 +166,9 @@ struct dma_fence_array *dma_fence_array_create(int num_fences,
if (!array)
return NULL;
+ fence = &array->base;
spin_lock_init(&array->lock);
- dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
+ dma_fence_init(fence, &dma_fence_array_ops, &array->lock,
context, seqno);
init_irq_work(&array->work, irq_dma_fence_array_work);
@@ -174,7 +176,10 @@ 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;
- array->base.error = PENDING_ERROR;
+ fence->error = PENDING_ERROR;
+
+ if (signal_on_any)
+ dma_fence_enable_sw_signaling(fence);
Oh, this looks strange. Was meant to call the
dma_fence_array_enable_signaling() without the lock held here.
/Thomas
