[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