| Message ID | 1502491174-10913-8-git-send-email-jason.ekstrand@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, 2017-08-11 Jason Ekstrand <jason@jlekstrand.net>: > From: Chris Wilson <chris@chris-wilson.co.uk> > > This is an illegal scenario, to free the fence whilst there are pending > callbacks. Currently, we emit a WARN and then cast aside the callbacks > leaving them dangling. Alternatively, we could set an error on the fence > and then signal fence so that any dependency chains from the fence can > be tidied up, and if they care they can check for the error. > > The question is whether or not the cure is worse than the disease > (premature fence signaling is never pretty). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/dma-buf/dma-fence.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 0cac367..4062708 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -172,7 +172,19 @@ void dma_fence_release(struct kref *kref) > > trace_dma_fence_destroy(fence); > > - WARN_ON(!list_empty(&fence->cb_list)); > + if (WARN_ON(!list_empty(&fence->cb_list))) { > + unsigned long flags; > + > + /* > + * This should never happen, but if it does make sure that we > + * don't leave chains dangling. We set the error flag first > + * so that the callbacks know this signal is due to an error. > + */ > + spin_lock_irqsave(fence->lock, flags); > + fence->error = -EDEADLK; > + dma_fence_signal_locked(fence); > + spin_unlock_irqrestore(fence->lock, flags); > + } Thanks for the patch! Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> > > if (fence->ops->release) > fence->ops->release(fence); > -- > 2.5.0.400.gff86faf > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Gustavo Padovan (2018-01-31 12:32:21) > Hi, > > 2017-08-11 Jason Ekstrand <jason@jlekstrand.net>: > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > This is an illegal scenario, to free the fence whilst there are pending > > callbacks. Currently, we emit a WARN and then cast aside the callbacks > > leaving them dangling. Alternatively, we could set an error on the fence > > and then signal fence so that any dependency chains from the fence can > > be tidied up, and if they care they can check for the error. > > > > The question is whether or not the cure is worse than the disease > > (premature fence signaling is never pretty). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/dma-buf/dma-fence.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 0cac367..4062708 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -172,7 +172,19 @@ void dma_fence_release(struct kref *kref) > > > > trace_dma_fence_destroy(fence); > > > > - WARN_ON(!list_empty(&fence->cb_list)); > > + if (WARN_ON(!list_empty(&fence->cb_list))) { > > + unsigned long flags; > > + > > + /* > > + * This should never happen, but if it does make sure that we > > + * don't leave chains dangling. We set the error flag first > > + * so that the callbacks know this signal is due to an error. > > + */ > > + spin_lock_irqsave(fence->lock, flags); > > + fence->error = -EDEADLK; > > + dma_fence_signal_locked(fence); > > + spin_unlock_irqrestore(fence->lock, flags); > > + } > > Thanks for the patch! > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> If you are picking up this version, please also include a Reviewed-by: Christian König <christian.koenig@amd.com> given to an earlier posting, which included a discussion on the pitiful lack of EHUMAN. -Chris
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 0cac367..4062708 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -172,7 +172,19 @@ void dma_fence_release(struct kref *kref) trace_dma_fence_destroy(fence); - WARN_ON(!list_empty(&fence->cb_list)); + if (WARN_ON(!list_empty(&fence->cb_list))) { + unsigned long flags; + + /* + * This should never happen, but if it does make sure that we + * don't leave chains dangling. We set the error flag first + * so that the callbacks know this signal is due to an error. + */ + spin_lock_irqsave(fence->lock, flags); + fence->error = -EDEADLK; + dma_fence_signal_locked(fence); + spin_unlock_irqrestore(fence->lock, flags); + } if (fence->ops->release) fence->ops->release(fence);