dma-fence: add comment for WARN_ON in dma_fence_release()
Subsystems (2)
PATCH 1/1 dma-fence: add comment for WARN_ON in dma_fence_release()
In dma_fence_release() there is a WARN_ON which could be triggered by several cases of wrong dma-fence usage. This patch adds a comment to explain two use-cases to help driver developers that use dma-fence and trigger that WARN_ON to better understand the reasons for it.
--- drivers/dma-buf/dma-fence.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
drivers/dma-buf/dma-fence.c +33 -0
@@ -171,6 +171,39 @@ void dma_fence_release(struct kref *kref)trace_dma_fence_destroy (fence); +/* +* If the WARN_ON below is triggered it could be because the dma fence +* was not signaled and therefore, the cb list is still not empty +* because the cb functions were not called. +* +* A more subtle case is where the fence got signaled by a thread that +* didn't hold a ref to the fence. The following describes the scenario: +* +* Thread A Thread B +*-------------------------- -------------------------- +* calls dma_fence_signal() { +* set signal bit +* +* scheduled out +* ---------------------------> calls dma_fence_wait_timeout() and +* returns immediately +* +* calls dma_fence_put() +* | +* |thread A doesn't hold ref +* |to fence so ref goes to 0 +* |and release is called +* | +* -> dma_fence_release() +* | +* -> WARN_ON triggered +* +* go over CB list, +* call each CB and remove it +* } +* +* +*/ WARN_ON (!list_empty (&fence->cb_list));if (fence->ops->release)
Signature · 6 lines
2.14.3 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 6 lines · Oded Gabbay
In
dma_fence_release()there is aWARN_ONwhich could be triggered by several cases of wrong dma-fence usage. This patch adds a comment to explain two use-cases to help driver developers that use dma-fence and trigger thatWARN_ONto better understand the reasons for it.Signed-off-by Oded Gabbay <[email protected]>
Not sure how useful this is, trying to do anything with a dma_fence while not holding a reference is just plain buggy. If you do that, then all kinds of use-after-free hilarity can happen. Trying to enumerate all the ways you can get refcounting wrong seems futile.
What we maybe could do is a simple one-line like
/* Failed to signal before release, could be a refcounting issue. */
I think this is generally a true statement and more useful hint for the next convoluted scenario (which in all likelihood will be different from yours).
-Daniel
Quoted 51 lines
--- drivers/dma-buf/dma-fence.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)drivers/dma-buf/dma-fence.c +33
-0@@ -171,6 +171,39 @@ void dma_fence_release(struct kref *kref)trace_dma_fence_destroy (fence); +/* +* If the WARN_ON below is triggered it could be because the dma fence +* was not signaled and therefore, the cb list is still not empty +* because the cb functions were not called. +* +* A more subtle case is where the fence got signaled by a thread that +* didn't hold a ref to the fence. The following describes the scenario: +* +* Thread A Thread B +*-------------------------- -------------------------- +* calls dma_fence_signal() { +* set signal bit +* +* scheduled out +* ---------------------------> calls dma_fence_wait_timeout() and +* returns immediately +* +* calls dma_fence_put() +* | +* |thread A doesn't hold ref +* |to fence so ref goes to 0 +* |and release is called +* | +* -> dma_fence_release() +* | +* -> WARN_ON triggered +* +* go over CB list, +* call each CB and remove it +* } +* +* +*/ WARN_ON (!list_empty (&fence->cb_list));if (fence->ops->release)Signature · 1 line
2.14.3
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 69 lines · Daniel Vetter
Quoted 6 lines · Oded Gabbay
In
dma_fence_release()there is aWARN_ONwhich could be triggered by several cases of wrong dma-fence usage. This patch adds a comment to explain two use-cases to help driver developers that use dma-fence and trigger thatWARN_ONto better understand the reasons for it.Signed-off-by Oded Gabbay <[email protected]>Not sure how useful this is, trying to do anything with a
dma_fencewhile not holding a reference is just plain buggy. If you do that, then all kinds of use-after-free hilarity can happen. Trying to enumerate all the ways you can get refcounting wrong seems futile.What we maybe could do is a simple one-line like
/* Failed to signal before release, could be a refcounting issue. */I think this is generally a true statement and more useful hint for the next convoluted scenario (which in all likelihood will be different from yours).
-DanielQuoted 47 lines
--- drivers/dma-buf/dma-fence.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)drivers/dma-buf/dma-fence.c +33
-0@@ -171,6 +171,39 @@ void dma_fence_release(struct kref *kref)trace_dma_fence_destroy (fence); +/* +* If the WARN_ON below is triggered it could be because the dma fence +* was not signaled and therefore, the cb list is still not empty +* because the cb functions were not called. +* +* A more subtle case is where the fence got signaled by a thread that +* didn't hold a ref to the fence. The following describes the scenario: +* +* Thread A Thread B +*-------------------------- -------------------------- +* calls dma_fence_signal() { +* set signal bit +* +* scheduled out +* ---------------------------> calls dma_fence_wait_timeout() and +* returns immediately +* +* calls dma_fence_put() +* | +* |thread A doesn't hold ref +* |to fence so ref goes to 0 +* |and release is called +* | +* -> dma_fence_release() +* | +* -> WARN_ON triggered +* +* go over CB list, +* call each CB and remove it +* } +* +* +*/ WARN_ON (!list_empty (&fence->cb_list));
Quoted 9 lines
Quoted 4 lines
drivers/dma-buf/dma-fence.c +0
-1if (fence->ops->release) --2.14.3
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel,
Yeah, I get what you are saying.
Can I add your RB to the one-liner version ?
Thanks,
Oded
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter
Reviewed-byQuoted 80 lines · Oded Gabbay
Quoted 69 lines · Daniel Vetter
Quoted 6 lines · Oded Gabbay
In
dma_fence_release()there is aWARN_ONwhich could be triggered by several cases of wrong dma-fence usage. This patch adds a comment to explain two use-cases to help driver developers that use dma-fence and trigger thatWARN_ONto better understand the reasons for it.Signed-off-by Oded Gabbay <[email protected]>Not sure how useful this is, trying to do anything with a
dma_fencewhile not holding a reference is just plain buggy. If you do that, then all kinds of use-after-free hilarity can happen. Trying to enumerate all the ways you can get refcounting wrong seems futile.What we maybe could do is a simple one-line like
/* Failed to signal before release, could be a refcounting issue. */I think this is generally a true statement and more useful hint for the next convoluted scenario (which in all likelihood will be different from yours).
-DanielQuoted 47 lines
--- drivers/dma-buf/dma-fence.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)drivers/dma-buf/dma-fence.c +33
-0@@ -171,6 +171,39 @@ void dma_fence_release(struct kref *kref)trace_dma_fence_destroy (fence); +/* +* If the WARN_ON below is triggered it could be because the dma fence +* was not signaled and therefore, the cb list is still not empty +* because the cb functions were not called. +* +* A more subtle case is where the fence got signaled by a thread that +* didn't hold a ref to the fence. The following describes the scenario: +* +* Thread A Thread B +*-------------------------- -------------------------- +* calls dma_fence_signal() { +* set signal bit +* +* scheduled out +* ---------------------------> calls dma_fence_wait_timeout() and +* returns immediately +* +* calls dma_fence_put() +* | +* |thread A doesn't hold ref +* |to fence so ref goes to 0 +* |and release is called +* | +* -> dma_fence_release() +* | +* -> WARN_ON triggered +* +* go over CB list, +* call each CB and remove it +* } +* +* +*/ WARN_ON (!list_empty (&fence->cb_list));Quoted 9 lines
Quoted 4 lines
drivers/dma-buf/dma-fence.c +0
-1if (fence->ops->release) --2.14.3
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Quoted 3 lines
Hi Daniel,
Yeah, I get what you are saying.
Can I add your RB to the one-liner version ?
Sure.
Cheers, Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel