Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> > The dma_fence framework checks at many places whether the signaled flag
> > 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_test_signaled_flag(), 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>
> 
> This is a nice cleanp:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> > ---
> >  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..25117a906846 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)))

I need to read a little better, I think this change isn't quite right.
The original code is test and set, the updated code is test only (i.e.,
you are missing the set step). So maybe just leave this line as is.

Matt

> > +	if (unlikely(dma_fence_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(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..19972f5d176f 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_test_signaled_flag - 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_test_signaled_flag(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_test_signaled_flag(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_test_signaled_flag(fence))
> >  		return true;
> >  
> >  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> > -- 
> > 2.49.0
> >