Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal at destroy phase

[Gustavo Padovan]

2016-01-15 John Harrison <John.C.Harrison@xxxxxxxxx>:

> On 15/01/2016 14:55, Gustavo Padovan wrote:
> >From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> >All changes to timeline value come through the user via
> >fence_timeline_signal() calls. When fence_timeline_destroy() is called no
> >changes on timeline->value happens hence call fence_timeline_signal() with
> >no increment is pointless.
> >
> >Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >---
> >  drivers/dma-buf/fence.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> >diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> >index 7a5fc9b..26f5f0f 100644
> >--- a/drivers/dma-buf/fence.c
> >+++ b/drivers/dma-buf/fence.c
> >@@ -136,7 +136,7 @@ EXPORT_SYMBOL(fence_timeline_put);
> >   * fence_timeline_destroy - destroy a fence_timeline
> >   * @timeline	[in]	the fence_timeline to destroy
> >   *
> >- * This function destroys a timeline. It signals any active fence first.
> >+ * This function destroys a timeline.
> 
> The implementation for this was certainly broken but I would say it should
> be fixed to match the comment rather than just abandoned completely. That
> is, what happens if a timeline owner destroys their timeline while there are
> outstanding fences which other drivers are waiting on? That is presumably a
> bug in the code that called destroy prematurely, but bugs happen.
> 
> The old implementation simply leaked the fences. Doing a debugfs dump would
> show the timeline with all its outstanding fences still floating around
> forever after. Worse, anything waiting on them would never be signalled and
> is therefore potentially deadlocked.
> 
> Note that I haven't had chance to look through the entire patch series yet
> so maybe this has been fixed up elsewhere. If not, then I think it
> definitely needs looking into.
> 

Patches 27 and 28 are attempt to fix that. I assumed that if some code is
calling fence_timeline_destroy() it wants to stop everything so I
worked on a solution that stops any waiter and allows the timeline to be
destroyed.

No one is using fence_timeline_destroy() in mainline now, so it is
definately a behaviour we can discuss.

	Gustavo