[PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Philipp Stanner]

The dma_fence framework checks at many places whether the signaled bit
of a fence is already set. The code can be simplified and made more
readable by providing a helper function for that.

Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
signaled. Use it internally.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 19 +++++++++----------
 include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 39e6f93dc310..3a48896ded62 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	lockdep_assert_held(fence->lock);
 
-	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-				      &fence->flags)))
+	if (unlikely(__dma_fence_is_signaled(fence)))
 		return -EINVAL;
 
 	/* Stash the cb_list before replacing it with the timestamp */
@@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
 	trace_dma_fence_destroy(fence);
 
 	if (!list_empty(&fence->cb_list) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	    !__dma_fence_is_signaled(fence)) {
 		const char __rcu *timeline;
 		const char __rcu *driver;
 		unsigned long flags;
@@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
 	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 				   &fence->flags);
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		return false;
 
 	if (!was_set && fence->ops->enable_signaling) {
@@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 	if (WARN_ON(!fence || !func))
 		return -EINVAL;
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	if (__dma_fence_is_signaled(fence)) {
 		INIT_LIST_HEAD(&cb->node);
 		return -ENOENT;
 	}
@@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 
 	spin_lock_irqsave(fence->lock, flags);
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		goto out;
 
 	if (intr && signal_pending(current)) {
@@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	cb.task = current;
 	list_add(&cb.base.node, &fence->cb_list);
 
-	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
+	while (!__dma_fence_is_signaled(fence) && ret > 0) {
 		if (intr)
 			__set_current_state(TASK_INTERRUPTIBLE);
 		else
@@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
 
 	for (i = 0; i < count; ++i) {
 		struct dma_fence *fence = fences[i];
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		if (__dma_fence_is_signaled(fence)) {
 			if (idx)
 				*idx = i;
 			return true;
@@ -1108,7 +1107,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
 			 "RCU protection is required for safe access to returned string");
 
-	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (!__dma_fence_is_signaled(fence))
 		return fence->ops->get_driver_name(fence);
 	else
 		return "detached-driver";
@@ -1140,7 +1139,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
 			 "RCU protection is required for safe access to returned string");
 
-	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (!__dma_fence_is_signaled(fence))
 		return fence->ops->get_timeline_name(fence);
 	else
 		return "signaled-timeline";
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 64639e104110..d32bf1b5b07d 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
 const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 
+/*
+ * __dma_fence_is_signaled - Only check whether a fence is signaled yet.
+ * @fence: the fence to check
+ *
+ * This function just checks whether @fence is signaled, without interacting
+ * with the fence in any way. The user must, therefore, ensure through other
+ * means that fences get signaled eventually.
+ *
+ * This function uses test_bit(), which is thread-safe. Naturally, this function
+ * should be used opportunistically; a fence could get signaled at any moment
+ * after the check is done.
+ *
+ * Return: true if signaled, false otherwise.
+ */
+static inline bool
+__dma_fence_is_signaled(struct dma_fence *fence)
+{
+	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
+}
+
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
  *                                is signaled yet.
@@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 static inline bool
 dma_fence_is_signaled_locked(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
@@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
-- 
2.49.0

[PATCH 2/2] dma-buf/dma-fence: Unify return codes for signalled fences

[Philipp Stanner]

Almost all users of dma_fence_signal() ignore the return code which
would indicate that the fence was already signaled. The same return code
by dma_fence_add_callback() cannot be ignored, however, because it's
needed to detect races.

For an already signaled fence, dma_fence_signal() returns -EINVAL,
whereas dma_fence_add_callback() returns -ENOENT.

Unify the error codes by having dma_fence_signal() return -ENOENT, too.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 3a48896ded62..09d97624e647 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -373,7 +373,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 	lockdep_assert_held(fence->lock);
 
 	if (unlikely(__dma_fence_is_signaled(fence)))
-		return -EINVAL;
+		return -ENOENT;
 
 	/* Stash the cb_list before replacing it with the timestamp */
 	list_replace(&fence->cb_list, &cb_list);
-- 
2.49.0

Re: [PATCH 2/2] dma-buf/dma-fence: Unify return codes for signalled fences

[Tvrtko Ursulin]

On 25/11/2025 10:44, Philipp Stanner wrote:
> Almost all users of dma_fence_signal() ignore the return code which
> would indicate that the fence was already signaled. The same return code
> by dma_fence_add_callback() cannot be ignored, however, because it's
> needed to detect races.
> 
> For an already signaled fence, dma_fence_signal() returns -EINVAL,
> whereas dma_fence_add_callback() returns -ENOENT.
> 
> Unify the error codes by having dma_fence_signal() return -ENOENT, too.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   drivers/dma-buf/dma-fence.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 3a48896ded62..09d97624e647 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -373,7 +373,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>   	lockdep_assert_held(fence->lock);
>   
>   	if (unlikely(__dma_fence_is_signaled(fence)))
> -		return -EINVAL;
> +		return -ENOENT;
>   
>   	/* Stash the cb_list before replacing it with the timestamp */
>   	list_replace(&fence->cb_list, &cb_list);

Story checks out AFAICT - only two callers fetch the error, xe and kfd, 
and neither does anything with it. So I'd say it makes sense to unify 
the errno.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

P.S. Just not sure of the 1/2 on which this one ends depending on. I 
*think* I suggested the helper in the context of some discussion long 
long time ago but what it was? And what about all the drivers which look 
at the signaled bit directly?

Re: [PATCH 2/2] dma-buf/dma-fence: Unify return codes for signalled fences

[Philipp Stanner]

On Tue, 2025-11-25 at 13:20 +0000, Tvrtko Ursulin wrote:
> 
> On 25/11/2025 10:44, Philipp Stanner wrote:
> > Almost all users of dma_fence_signal() ignore the return code which
> > would indicate that the fence was already signaled. The same return code
> > by dma_fence_add_callback() cannot be ignored, however, because it's
> > needed to detect races.
> > 
> > For an already signaled fence, dma_fence_signal() returns -EINVAL,
> > whereas dma_fence_add_callback() returns -ENOENT.
> > 
> > Unify the error codes by having dma_fence_signal() return -ENOENT, too.
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >   drivers/dma-buf/dma-fence.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 3a48896ded62..09d97624e647 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -373,7 +373,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> >   	lockdep_assert_held(fence->lock);
> >   
> >   	if (unlikely(__dma_fence_is_signaled(fence)))
> > -		return -EINVAL;
> > +		return -ENOENT;
> >   
> >   	/* Stash the cb_list before replacing it with the timestamp */
> >   	list_replace(&fence->cb_list, &cb_list);
> 
> Story checks out AFAICT - only two callers fetch the error, xe and kfd, 
> and neither does anything with it. So I'd say it makes sense to unify
> the errno.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> Regards,
> 
> Tvrtko
> 
> P.S. Just not sure of the 1/2 on which this one ends depending on. I 
> *think* I suggested the helper in the context of some discussion long
> long time ago but what it was? And what about all the drivers which look 
> at the signaled bit directly?

That was in a discussion where we could have needed such a function to
solve a bug in Nouveau where dma_fence_is_signaled() unexpetedely
signaled a fence. AFAIR that caused locking issues in that context.

The other test_bit() users could be ported, of course.


P.

Re: [PATCH 2/2] dma-buf/dma-fence: Unify return codes for signalled fences

[Christian König]

On 11/25/25 14:20, Tvrtko Ursulin wrote:
> On 25/11/2025 10:44, Philipp Stanner wrote:
>> Almost all users of dma_fence_signal() ignore the return code which
>> would indicate that the fence was already signaled. The same return code
>> by dma_fence_add_callback() cannot be ignored, however, because it's
>> needed to detect races.
>>
>> For an already signaled fence, dma_fence_signal() returns -EINVAL,
>> whereas dma_fence_add_callback() returns -ENOENT.
>>
>> Unify the error codes by having dma_fence_signal() return -ENOENT, too.
>>
>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>> ---
>>   drivers/dma-buf/dma-fence.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 3a48896ded62..09d97624e647 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -373,7 +373,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>       lockdep_assert_held(fence->lock);
>>         if (unlikely(__dma_fence_is_signaled(fence)))
>> -        return -EINVAL;
>> +        return -ENOENT;
>>         /* Stash the cb_list before replacing it with the timestamp */
>>       list_replace(&fence->cb_list, &cb_list);
> 
> Story checks out AFAICT - only two callers fetch the error, xe and kfd, and neither does anything with it. So I'd say it makes sense to unify the errno.

To be honest it rather sounds like removing the error code is the right thing to do.

IIRC the policy was initially that drivers should signal a fence only once and when they do it multiple times then that is an error.

But we quickly found that with opportunistically checking that this policy is actually not helpful and dropped it.

The error return code here is basically just a leftover from this policy and not useful as far as I can see.

Regards,
Christian. 

> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> Regards,
> 
> Tvrtko
> 
> P.S. Just not sure of the 1/2 on which this one ends depending on. I *think* I suggested the helper in the context of some discussion long long time ago but what it was? And what about all the drivers which look at the signaled bit directly?

Re: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Christian König]

On 11/25/25 11:44, Philipp Stanner wrote:
> The dma_fence framework checks at many places whether the signaled bit
> of a fence is already set. The code can be simplified and made more
> readable by providing a helper function for that.
> 
> Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
> signaled. Use it internally.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
>  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 39e6f93dc310..3a48896ded62 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	lockdep_assert_held(fence->lock);
>  
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	if (unlikely(__dma_fence_is_signaled(fence)))
>  		return -EINVAL;
>  
>  	/* Stash the cb_list before replacing it with the timestamp */
> @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
>  	trace_dma_fence_destroy(fence);
>  
>  	if (!list_empty(&fence->cb_list) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !__dma_fence_is_signaled(fence)) {
>  		const char __rcu *timeline;
>  		const char __rcu *driver;
>  		unsigned long flags;
> @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>  	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  				   &fence->flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return false;
>  
>  	if (!was_set && fence->ops->enable_signaling) {
> @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  	if (WARN_ON(!fence || !func))
>  		return -EINVAL;
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	if (__dma_fence_is_signaled(fence)) {
>  		INIT_LIST_HEAD(&cb->node);
>  		return -ENOENT;
>  	}
> @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  
>  	spin_lock_irqsave(fence->lock, flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		goto out;
>  
>  	if (intr && signal_pending(current)) {
> @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	cb.task = current;
>  	list_add(&cb.base.node, &fence->cb_list);
>  
> -	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
> +	while (!__dma_fence_is_signaled(fence) && ret > 0) {
>  		if (intr)
>  			__set_current_state(TASK_INTERRUPTIBLE);
>  		else
> @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>  
>  	for (i = 0; i < count; ++i) {
>  		struct dma_fence *fence = fences[i];
> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +		if (__dma_fence_is_signaled(fence)) {
>  			if (idx)
>  				*idx = i;
>  			return true;
> @@ -1108,7 +1107,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>  	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>  			 "RCU protection is required for safe access to returned string");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!__dma_fence_is_signaled(fence))
>  		return fence->ops->get_driver_name(fence);
>  	else
>  		return "detached-driver";
> @@ -1140,7 +1139,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
>  	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>  			 "RCU protection is required for safe access to returned string");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!__dma_fence_is_signaled(fence))
>  		return fence->ops->get_timeline_name(fence);
>  	else
>  		return "signaled-timeline";
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 64639e104110..d32bf1b5b07d 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>  const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>  const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  
> +/*
> + * __dma_fence_is_signaled - Only check whether a fence is signaled yet.
> + * @fence: the fence to check
> + *
> + * This function just checks whether @fence is signaled, without interacting
> + * with the fence in any way. The user must, therefore, ensure through other
> + * means that fences get signaled eventually.
> + *
> + * This function uses test_bit(), which is thread-safe. Naturally, this function
> + * should be used opportunistically; a fence could get signaled at any moment
> + * after the check is done.
> + *
> + * Return: true if signaled, false otherwise.
> + */
> +static inline bool
> +__dma_fence_is_signaled(struct dma_fence *fence)

Maybe name that dma_fence_test_signaled_flag() to clearly distinct it from the is_signaled() path.

Apart from that looks ok to me.

Regards,
Christian.

> +{
> +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> +}
> +
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
>   *                                is signaled yet.
> @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  static inline bool
>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>  static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {

Re: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Steven Price]

On 25/11/2025 10:44, Philipp Stanner wrote:
> The dma_fence framework checks at many places whether the signaled bit
> of a fence is already set. The code can be simplified and made more
> readable by providing a helper function for that.
> 
> Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
> signaled. Use it internally.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
>  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 39e6f93dc310..3a48896ded62 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	lockdep_assert_held(fence->lock);
>  
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	if (unlikely(__dma_fence_is_signaled(fence)))

Here you are replacing test_and_set_bit(), but the helper just does
test_bit() - so this is changing the behaviour (and I suspect is wrong).

Thanks,
Steve

>  		return -EINVAL;
>  
>  	/* Stash the cb_list before replacing it with the timestamp */
> @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
>  	trace_dma_fence_destroy(fence);
>  
>  	if (!list_empty(&fence->cb_list) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !__dma_fence_is_signaled(fence)) {
>  		const char __rcu *timeline;
>  		const char __rcu *driver;
>  		unsigned long flags;
> @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>  	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  				   &fence->flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return false;
>  
>  	if (!was_set && fence->ops->enable_signaling) {
> @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  	if (WARN_ON(!fence || !func))
>  		return -EINVAL;
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	if (__dma_fence_is_signaled(fence)) {
>  		INIT_LIST_HEAD(&cb->node);
>  		return -ENOENT;
>  	}
> @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  
>  	spin_lock_irqsave(fence->lock, flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		goto out;
>  
>  	if (intr && signal_pending(current)) {
> @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	cb.task = current;
>  	list_add(&cb.base.node, &fence->cb_list);
>  
> -	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
> +	while (!__dma_fence_is_signaled(fence) && ret > 0) {
>  		if (intr)
>  			__set_current_state(TASK_INTERRUPTIBLE);
>  		else
> @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>  
>  	for (i = 0; i < count; ++i) {
>  		struct dma_fence *fence = fences[i];
> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +		if (__dma_fence_is_signaled(fence)) {
>  			if (idx)
>  				*idx = i;
>  			return true;
> @@ -1108,7 +1107,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>  	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>  			 "RCU protection is required for safe access to returned string");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!__dma_fence_is_signaled(fence))
>  		return fence->ops->get_driver_name(fence);
>  	else
>  		return "detached-driver";
> @@ -1140,7 +1139,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
>  	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>  			 "RCU protection is required for safe access to returned string");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!__dma_fence_is_signaled(fence))
>  		return fence->ops->get_timeline_name(fence);
>  	else
>  		return "signaled-timeline";
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 64639e104110..d32bf1b5b07d 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>  const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>  const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  
> +/*
> + * __dma_fence_is_signaled - Only check whether a fence is signaled yet.
> + * @fence: the fence to check
> + *
> + * This function just checks whether @fence is signaled, without interacting
> + * with the fence in any way. The user must, therefore, ensure through other
> + * means that fences get signaled eventually.
> + *
> + * This function uses test_bit(), which is thread-safe. Naturally, this function
> + * should be used opportunistically; a fence could get signaled at any moment
> + * after the check is done.
> + *
> + * Return: true if signaled, false otherwise.
> + */
> +static inline bool
> +__dma_fence_is_signaled(struct dma_fence *fence)
> +{
> +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> +}
> +
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
>   *                                is signaled yet.
> @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  static inline bool
>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>  static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {

Re: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Philipp Stanner]

On Mon, 2025-12-01 at 15:46 +0000, Steven Price wrote:
> On 25/11/2025 10:44, Philipp Stanner wrote:
> > The dma_fence framework checks at many places whether the signaled bit
> > of a fence is already set. The code can be simplified and made more
> > readable by providing a helper function for that.
> > 
> > Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
> > signaled. Use it internally.
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
> >  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
> >  2 files changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 39e6f93dc310..3a48896ded62 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> >  
> >  	lockdep_assert_held(fence->lock);
> >  
> > -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > -				      &fence->flags)))
> > +	if (unlikely(__dma_fence_is_signaled(fence)))
> 
> Here you are replacing test_and_set_bit(), but the helper just does
> test_bit() - so this is changing the behaviour (and I suspect is wrong).
> 
> Thanks,
> Steve
> 
> 

Yes, that is a bug and has already been caught by various parties – but
it's good that you're paying attention :)

We're currently in this revision, where the bug is fixed:
https://lore.kernel.org/dri-devel/20251201105011.19386-2-phasta@kernel.org/


P.

Re: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[kernel test robot]

Hello,

kernel test robot noticed "Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN_PTI" on:

commit: 142f20971642be40507d3a0bc94e905e251db81a ("[PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()")
url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/dma-buf-dma-fence-Unify-return-codes-for-signalled-fences/20251125-184735
base: git://linuxtv.org/sailus/media_tree.git master
patch link: https://lore.kernel.org/all/20251125104443.82974-2-phasta@kernel.org/
patch subject: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

in testcase: lkvs
version: lkvs-x86_64-6505b18-1_20251124
with following parameters:

	test: rapl-client



config: x86_64-rhel-9.4-func
compiler: gcc-14
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202512092317.8cf778a-lkp@intel.com


[   46.662293][  T179] i915 0000:00:02.0: vgaarb: VGA decodes changed: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[   46.680656][   T40] sd 1:0:0:0: [sda] Preferred minimum I/O size 4096 bytes
[   46.682302][   T59] i915 0000:00:02.0: Direct firmware load for i915/skl_dmc_ver1_27.bin failed with error -2
[   46.698362][   T59] i915 0000:00:02.0: [drm] Failed to load DMC firmware i915/skl_dmc_ver1_27.bin (-ENOENT). Disabling runtime power management.
[   46.711256][   T59] i915 0000:00:02.0: [drm] DMC firmware homepage: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
[   46.725941][   T12] Oops: general protection fault, probably for non-canonical address 0xdffffc01544551ba: 0000 [#1] SMP KASAN PTI
[   46.725953][  T179] ------------[ cut here ]------------
[   46.737600][   T12] KASAN: probably user-memory-access in range [0x0000000aa22a8dd0-0x0000000aa22a8dd7]
[   46.737612][   T12] CPU: 3 UID: 0 PID: 12 Comm: kworker/u16:0 Tainted: G S        I         6.18.0-rc5-00236-g142f20971642 #1 PREEMPT(voluntary)
[   46.742889][  T179] Fence 0000:00:02.0:[i915]:9:2 released with pending signals!
[   46.752212][   T12] Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
[   46.752217][   T12] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[   46.752220][   T12] Workqueue: i915 __i915_gem_free_work [i915]
[   46.765216][  T179] WARNING: CPU: 0 PID: 179 at drivers/dma-buf/dma-fence.c:555 dma_fence_release (kbuild/src/consumer/drivers/dma-buf/dma-fence.c:555 (discriminator 1))
[   46.772517][   T12]
[   46.772521][   T12] RIP: 0010:__list_add_valid_or_report (kbuild/src/consumer/lib/list_debug.c:29)
[   46.772528][   T12] Code: 8b 98 fe 48 89 d3 48 85 d2 0f 84 af 8b 98 fe 4c 8d 62 08 48 89 fd 49 89 f5 48 b8 00 00 00 00 00 fc ff df 4c 89 e2 48 c1 ea 03 <80> 3c 02 00 0f 85 b7 00 00 00 4c 39 6b 08 75 43 48 b8 00 00 00 00
All code
========
   0:	8b 98 fe 48 89 d3    	mov    -0x2c76b702(%rax),%ebx
   6:	48 85 d2             	test   %rdx,%rdx
   9:	0f 84 af 8b 98 fe    	je     0xfffffffffe988bbe
   f:	4c 8d 62 08          	lea    0x8(%rdx),%r12
  13:	48 89 fd             	mov    %rdi,%rbp
  16:	49 89 f5             	mov    %rsi,%r13
  19:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  20:	fc ff df 
  23:	4c 89 e2             	mov    %r12,%rdx
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)		<-- trapping instruction
  2e:	0f 85 b7 00 00 00    	jne    0xeb
  34:	4c 39 6b 08          	cmp    %r13,0x8(%rbx)
  38:	75 43                	jne    0x7d
  3a:	48                   	rex.W
  3b:	b8 00 00 00 00       	mov    $0x0,%eax

Code starting with the faulting instruction
===========================================
   0:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   4:	0f 85 b7 00 00 00    	jne    0xc1
   a:	4c 39 6b 08          	cmp    %r13,0x8(%rbx)
   e:	75 43                	jne    0x53
  10:	48                   	rex.W
  11:	b8 00 00 00 00       	mov    $0x0,%eax
[   46.779267][  T179] Modules linked in:
[   46.787293][   T12] RSP: 0018:ffffc9000010fac8 EFLAGS: 00010003
[   46.787300][   T12] RAX: dffffc0000000000 RBX: 0000000aa22a8dcf RCX: ffffffff82f5859d
[   46.793179][  T179]  platform_profile
[   46.803018][   T12] RDX: 00000001544551ba RSI: ffff88884fde5c90 RDI: ffffc9000010fb50
[   46.803023][   T12] RBP: ffffc9000010fb50 R08: 0000000000000000 R09: ffffed1109fbcb96
[   46.803025][   T12] R10: ffff88884fde5cb7 R11: fefefefefefefeff R12: 0000000aa22a8dd7
[   46.805219][  T179]  sd_mod
[   46.811413][   T12] R13: ffff88884fde5c90 R14: ffff88884fde5c90 R15: 0000000aa22a8dcf
[   46.811417][   T12] FS:  0000000000000000(0000) GS:ffff888848e3e000(0000) knlGS:0000000000000000
[   46.811419][   T12] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   46.830755][  T179]  intel_rapl_common
[   46.834463][   T12] CR2: 00007f3bc83ae000 CR3: 0000000868388002 CR4: 00000000003726f0
[   46.834467][   T12] Call Trace:
[   46.834469][   T12]  <TASK>
[   46.840348][  T179]  snd_hda_core
[   46.848115][   T12]  dma_fence_default_wait (kbuild/src/consumer/include/linux/list.h:158 (discriminator 1) kbuild/src/consumer/include/linux/list.h:177 (discriminator 1) kbuild/src/consumer/drivers/dma-buf/dma-fence.c:800 (discriminator 1))
[   46.851758][  T179]  x86_pkg_temp_thermal
[   46.859527][   T12]  ? __pfx_dma_fence_default_wait (kbuild/src/consumer/drivers/dma-buf/dma-fence.c:778)


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20251209/202512092317.8cf778a-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Philipp Stanner]

Some parties want to check whether a function is already signaled
without actually signaling the fence, which is what
dma_fence_is_signaled() might due if the fence ops 'signaled' callback
is implemented.

Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
signaled. Use it internally.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 include/linux/dma-fence.h | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 48b5202c531d..ac951a54a007 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -381,6 +381,26 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
 			       struct dma_fence_cb *cb);
 void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 
+/**
+ * __dma_fence_is_signaled - Only check whether a fence is signaled yet.
+ * @fence: the fence to check
+ *
+ * This function just checks whether @fence is signaled, without interacting
+ * with the fence in any way. The user must, therefore, ensure through other
+ * means that fences get signaled eventually.
+ *
+ * This function does not require locking.
+ *
+ * See also dma_fence_is_signaled().
+ *
+ * Return: true if signaled, false otherwise.
+ */
+static inline bool
+__dma_fence_is_signaled(struct dma_fence *fence)
+{
+	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
+}
+
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
  *                                is signaled yet.
@@ -398,7 +418,7 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 static inline bool
 dma_fence_is_signaled_locked(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
@@ -428,7 +448,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (__dma_fence_is_signaled(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
-- 
2.49.0

Re: [PATCH 1/2] dma-buf: Add __dma_fence_is_signaled()

[Tvrtko Ursulin]

On 22/05/2025 12:25, Philipp Stanner wrote:
> Some parties want to check whether a function is already signaled
> without actually signaling the fence, which is what
> dma_fence_is_signaled() might due if the fence ops 'signaled' callback

s/due/do/

> is implemented.
> 
> Add __dma_fence_is_signaled(), which _only_ checks whether a fence is
> signaled. Use it internally.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   include/linux/dma-fence.h | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 48b5202c531d..ac951a54a007 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -381,6 +381,26 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
>   			       struct dma_fence_cb *cb);
>   void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>   
> +/**
> + * __dma_fence_is_signaled - Only check whether a fence is signaled yet.
> + * @fence: the fence to check
> + *
> + * This function just checks whether @fence is signaled, without interacting
> + * with the fence in any way. The user must, therefore, ensure through other

s/user/caller/ ?

Otherwise looks good to me. For if/when Christian approves you can add my:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

> + * means that fences get signaled eventually.
> + *
> + * This function does not require locking.
> + *
> + * See also dma_fence_is_signaled().
> + *
> + * Return: true if signaled, false otherwise.
> + */
> +static inline bool
> +__dma_fence_is_signaled(struct dma_fence *fence)
> +{
> +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> +}
> +
>   /**
>    * dma_fence_is_signaled_locked - Return an indication if the fence
>    *                                is signaled yet.
> @@ -398,7 +418,7 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>   static inline bool
>   dma_fence_is_signaled_locked(struct dma_fence *fence)
>   {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>   		return true;
>   
>   	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> @@ -428,7 +448,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>   static inline bool
>   dma_fence_is_signaled(struct dma_fence *fence)
>   {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (__dma_fence_is_signaled(fence))
>   		return true;
>   
>   	if (fence->ops->signaled && fence->ops->signaled(fence)) {