Sparse complains about assigning a string to a __rcu annotated local variable:
drivers/dma-buf/dma-fence.c:1040:38: warning: incorrect type in initializer (different address spaces) drivers/dma-buf/dma-fence.c:1040:38: expected char const [noderef] __rcu *timeline drivers/dma-buf/dma-fence.c:1040:38: got char * drivers/dma-buf/dma-fence.c:1041:36: warning: incorrect type in initializer (different address spaces) drivers/dma-buf/dma-fence.c:1041:36: expected char const [noderef] __rcu *driver drivers/dma-buf/dma-fence.c:1041:36: got char *
It is harmless but lets silence it.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: ac364014fd81 ("dma-buf: cleanup dma_fence_describe v3") Cc: Christian König christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/dma-buf/dma-fence.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 1826ba73094c..a2aa82f4eedd 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -1037,8 +1037,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline); */ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) { - const char __rcu *timeline = ""; - const char __rcu *driver = ""; + const char __rcu *timeline = (const char __rcu *)""; + const char __rcu *driver = (const char __rcu *)""; const char *signaled = "";
rcu_read_lock();
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return;
+ trace_dma_fence_signaled(fence); + /* * When neither a release nor a wait operation is specified set the ops * pointer to NULL to allow the fence structure to become independent @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); - trace_dma_fence_signaled(fence);
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@
struct dma_fence;
+DECLARE_EVENT_CLASS(dma_fence, + + TP_PROTO(struct dma_fence *fence), + + TP_ARGS(fence), + + TP_STRUCT__entry( + __string(driver, dma_fence_driver_name(fence)) + __string(timeline, dma_fence_timeline_name(fence)) + __field(unsigned int, context) + __field(unsigned int, seqno) + ), + + TP_fast_assign( + __assign_str(driver); + __assign_str(timeline); + __entry->context = fence->context; + __entry->seqno = fence->seqno; + ), + + TP_printk("driver=%s timeline=%s context=%u seqno=%u", + __get_str(driver), __get_str(timeline), __entry->context, + __entry->seqno) +); + /* * Safe only for call sites which are guaranteed to not race with fence * signaling,holding the fence->lock and having checked for not signaled, or the * signaling path itself. */ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops,
TP_PROTO(struct dma_fence *fence),
@@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) );
-DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
TP_PROTO(struct dma_fence *fence),
@@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) );
-DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
TP_PROTO(struct dma_fence *fence),
TP_ARGS(fence) );
-DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
TP_PROTO(struct dma_fence *fence),
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return;
- trace_dma_fence_signaled(fence);
- /*
- When neither a release nor a wait operation is specified set the ops
- pointer to NULL to allow the fence structure to become independent
@@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
- trace_dma_fence_signaled(fence);
I think this part here should be a separate patch.
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@ struct dma_fence; +DECLARE_EVENT_CLASS(dma_fence,
- TP_PROTO(struct dma_fence *fence),
- TP_ARGS(fence),
- TP_STRUCT__entry(
__string(driver, dma_fence_driver_name(fence))__string(timeline, dma_fence_timeline_name(fence))__field(unsigned int, context)__field(unsigned int, seqno)- ),
- TP_fast_assign(
__assign_str(driver);__assign_str(timeline);__entry->context = fence->context;__entry->seqno = fence->seqno;- ),
- TP_printk("driver=%s timeline=%s context=%u seqno=%u",
__get_str(driver), __get_str(timeline), __entry->context,__entry->seqno)+);
Mhm, I'm strongly in favor to just use this approach for all trace points.
The minimal extra overhead shouldn't really matter at all.
Regards, Christian.
/*
- Safe only for call sites which are guaranteed to not race with fence
- signaling,holding the fence->lock and having checked for not signaled, or the
- signaling path itself.
*/ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops, TP_PROTO(struct dma_fence *fence), @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init, TP_PROTO(struct dma_fence *fence), @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal, TP_PROTO(struct dma_fence *fence), TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled, TP_PROTO(struct dma_fence *fence),
On 14/04/2026 19:30, Christian König wrote:
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return;
- trace_dma_fence_signaled(fence);
- /*
- When neither a release nor a wait operation is specified set the ops
- pointer to NULL to allow the fence structure to become independent
@@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
- trace_dma_fence_signaled(fence);
I think this part here should be a separate patch.
I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igal... but the discussion fizzled out before an rb.
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@ struct dma_fence; +DECLARE_EVENT_CLASS(dma_fence,
- TP_PROTO(struct dma_fence *fence),
- TP_ARGS(fence),
- TP_STRUCT__entry(
__string(driver, dma_fence_driver_name(fence))__string(timeline, dma_fence_timeline_name(fence))__field(unsigned int, context)__field(unsigned int, seqno)- ),
- TP_fast_assign(
__assign_str(driver);__assign_str(timeline);__entry->context = fence->context;__entry->seqno = fence->seqno;- ),
- TP_printk("driver=%s timeline=%s context=%u seqno=%u",
__get_str(driver), __get_str(timeline), __entry->context,__entry->seqno)+);
Mhm, I'm strongly in favor to just use this approach for all trace points.
The minimal extra overhead shouldn't really matter at all.
Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
Regards,
Tvrtko
Regards, Christian.
/*
- Safe only for call sites which are guaranteed to not race with fence
- signaling,holding the fence->lock and having checked for not signaled, or the
- signaling path itself.
*/ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops, TP_PROTO(struct dma_fence *fence), @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init, TP_PROTO(struct dma_fence *fence), @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal, TP_PROTO(struct dma_fence *fence), TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled, TP_PROTO(struct dma_fence *fence),
On 4/15/26 09:58, Tvrtko Ursulin wrote:
On 14/04/2026 19:30, Christian König wrote:
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return; + trace_dma_fence_signaled(fence);
/* * When neither a release nor a wait operation is specified set the ops * pointer to NULL to allow the fence structure to become independent @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); - trace_dma_fence_signaled(fence);
I think this part here should be a separate patch.
I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igal... but the discussion fizzled out before an rb.
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@ struct dma_fence; +DECLARE_EVENT_CLASS(dma_fence,
+ TP_PROTO(struct dma_fence *fence),
+ TP_ARGS(fence),
+ TP_STRUCT__entry( + __string(driver, dma_fence_driver_name(fence)) + __string(timeline, dma_fence_timeline_name(fence)) + __field(unsigned int, context) + __field(unsigned int, seqno) + ),
+ TP_fast_assign( + __assign_str(driver); + __assign_str(timeline); + __entry->context = fence->context; + __entry->seqno = fence->seqno; + ),
+ TP_printk("driver=%s timeline=%s context=%u seqno=%u", + __get_str(driver), __get_str(timeline), __entry->context, + __entry->seqno) +);
Mhm, I'm strongly in favor to just use this approach for all trace points.
The minimal extra overhead shouldn't really matter at all.
Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init
I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.
I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.
and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
Mhm, that is a bit more problematic.
ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.
Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.
Thanks, Christian.
Regards,
Tvrtko
Regards, Christian.
/* * Safe only for call sites which are guaranteed to not race with fence * signaling,holding the fence->lock and having checked for not signaled, or the * signaling path itself. */ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops, TP_PROTO(struct dma_fence *fence), @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init, TP_PROTO(struct dma_fence *fence), @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal, TP_PROTO(struct dma_fence *fence), TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled, TP_PROTO(struct dma_fence *fence),
On 15/04/2026 09:13, Christian König wrote:
On 4/15/26 09:58, Tvrtko Ursulin wrote:
On 14/04/2026 19:30, Christian König wrote:
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return; + trace_dma_fence_signaled(fence);
/* * When neither a release nor a wait operation is specified set the ops * pointer to NULL to allow the fence structure to become independent @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); - trace_dma_fence_signaled(fence);
I think this part here should be a separate patch.
I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igal... but the discussion fizzled out before an rb.
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@ struct dma_fence; +DECLARE_EVENT_CLASS(dma_fence,
+ TP_PROTO(struct dma_fence *fence),
+ TP_ARGS(fence),
+ TP_STRUCT__entry( + __string(driver, dma_fence_driver_name(fence)) + __string(timeline, dma_fence_timeline_name(fence)) + __field(unsigned int, context) + __field(unsigned int, seqno) + ),
+ TP_fast_assign( + __assign_str(driver); + __assign_str(timeline); + __entry->context = fence->context; + __entry->seqno = fence->seqno; + ),
+ TP_printk("driver=%s timeline=%s context=%u seqno=%u", + __get_str(driver), __get_str(timeline), __entry->context, + __entry->seqno) +);
Mhm, I'm strongly in favor to just use this approach for all trace points.
The minimal extra overhead shouldn't really matter at all.
Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init
I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.
I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.
and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
Mhm, that is a bit more problematic.
ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.
Hm yes, that too.
Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.
Thank you, I've sent an updated version with a comment to this effect placed to the event class definition. I put your r-b so please double check if you are happy with that version.
Regards,
Tvrtko
/* * Safe only for call sites which are guaranteed to not race with fence * signaling,holding the fence->lock and having checked for not signaled, or the * signaling path itself. */ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops, TP_PROTO(struct dma_fence *fence), @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init, TP_PROTO(struct dma_fence *fence), @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal, TP_PROTO(struct dma_fence *fence), TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled, TP_PROTO(struct dma_fence *fence),
On 4/15/26 10:33, Tvrtko Ursulin wrote:
On 15/04/2026 09:13, Christian König wrote:
On 4/15/26 09:58, Tvrtko Ursulin wrote:
On 14/04/2026 19:30, Christian König wrote:
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Trace_dma_fence_signaled, trace_dma_fence_wait_end and trace_dma_fence_destroy can all currently dereference a null fence->ops pointer after it has been reset on fence signalling.
Lets use the safe string getters for most tracepoints to avoid this class of a problem, while for the signal tracepoint we move it to before ops are cleared to avoid losing the driver and timeline name information. Apart from moving it we also need to add a new tracepoint class to bypass the safe name getters since the signaled bit is already set.
For dma_fence_init we also need to use the new tracepoint class since the rcu read lock is not held there, and we can do the same for the enable signaling since there we are certain the fence cannot be signaled while we are holding the lock and have even validated the fence->ops.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Cc: Christian König christian.koenig@amd.com Cc: Philipp Stanner phasta@kernel.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/dma-fence.c | 3 ++- include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index a2aa82f4eedd..b3bfa6943a8e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return; + trace_dma_fence_signaled(fence);
/* * When neither a release nor a wait operation is specified set the ops * pointer to NULL to allow the fence structure to become independent @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, fence->timestamp = timestamp; set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); - trace_dma_fence_signaled(fence);
I think this part here should be a separate patch.
I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igal... but the discussion fizzled out before an rb.
list_for_each_entry_safe(cur, tmp, &cb_list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 3abba45c0601..9e0cb9ce2388 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,12 +9,37 @@ struct dma_fence; +DECLARE_EVENT_CLASS(dma_fence,
+ TP_PROTO(struct dma_fence *fence),
+ TP_ARGS(fence),
+ TP_STRUCT__entry( + __string(driver, dma_fence_driver_name(fence)) + __string(timeline, dma_fence_timeline_name(fence)) + __field(unsigned int, context) + __field(unsigned int, seqno) + ),
+ TP_fast_assign( + __assign_str(driver); + __assign_str(timeline); + __entry->context = fence->context; + __entry->seqno = fence->seqno; + ),
+ TP_printk("driver=%s timeline=%s context=%u seqno=%u", + __get_str(driver), __get_str(timeline), __entry->context, + __entry->seqno) +);
Mhm, I'm strongly in favor to just use this approach for all trace points.
The minimal extra overhead shouldn't really matter at all.
Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init
I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.
I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.
and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
Mhm, that is a bit more problematic.
ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.
Hm yes, that too.
Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.
Thank you, I've sent an updated version with a comment to this effect placed to the event class definition. I put your r-b so please double check if you are happy with that version.
Yeah works for me, feel free to push to drm-misc-next.
Thanks, Christian.
Regards,
Tvrtko
/* * Safe only for call sites which are guaranteed to not race with fence * signaling,holding the fence->lock and having checked for not signaled, or the * signaling path itself. */ -DECLARE_EVENT_CLASS(dma_fence, +DECLARE_EVENT_CLASS(dma_fence_ops, TP_PROTO(struct dma_fence *fence), @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_init, +DEFINE_EVENT(dma_fence_ops, dma_fence_init, TP_PROTO(struct dma_fence *fence), @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal, TP_PROTO(struct dma_fence *fence), TP_ARGS(fence) ); -DEFINE_EVENT(dma_fence, dma_fence_signaled, +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled, TP_PROTO(struct dma_fence *fence),
On 4/14/26 17:49, Tvrtko Ursulin wrote:
Sparse complains about assigning a string to a __rcu annotated local variable:
drivers/dma-buf/dma-fence.c:1040:38: warning: incorrect type in initializer (different address spaces) drivers/dma-buf/dma-fence.c:1040:38: expected char const [noderef] __rcu *timeline drivers/dma-buf/dma-fence.c:1040:38: got char * drivers/dma-buf/dma-fence.c:1041:36: warning: incorrect type in initializer (different address spaces) drivers/dma-buf/dma-fence.c:1041:36: expected char const [noderef] __rcu *driver drivers/dma-buf/dma-fence.c:1041:36: got char *
It is harmless but lets silence it.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Fixes: ac364014fd81 ("dma-buf: cleanup dma_fence_describe v3") Cc: Christian König christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org
Reviewed-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-fence.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 1826ba73094c..a2aa82f4eedd 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -1037,8 +1037,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline); */ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) {
- const char __rcu *timeline = "";
- const char __rcu *driver = "";
- const char __rcu *timeline = (const char __rcu *)"";
- const char __rcu *driver = (const char __rcu *)""; const char *signaled = "";
rcu_read_lock();
linaro-mm-sig@lists.linaro.org
-
Christian König -
Tvrtko Ursulin