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

[Chris Wilson]

On Thu, Nov 03, 2016 at 04:31:14PM -0400, Rob Clark wrote:
> 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.

Could mention that this is only an issue if the same fence->lock
appeared more than once in the array. Which is possible.

> 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);

I think you need a dma_fence_get(fence) here with a corresponding put in
the worker. Sadly I still can't poke a hole in the lockdep warning.

What I might suggest is that we try

static bool is_signaled(struct dma_fence *fence)
{
	if (test_bit(SIGNALED_BIT, &fence->flags))
		return true;

	return fence->ops->signaled && fence->ops->signaled(fence);
}

static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
{
	struct dma_fence_array *array = to_dma_fence_array(fence);
	int num_pending = atomic_read(&array->num_pending);
	int i;

	for (i = 0; i < array->num_fences; i++)
		if (is_signaled(array->fences[i]) && !--num_pending) {
			atomic_set(&array->num_pending, 0);
			return false;
		}
	
	dma_fence_get(&array->base);
	queue_work(system_unbound_wq, &array->enable_signaling_worker);
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre