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>
---
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)))
+ 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.0Hi Philipp, > 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))) > + if (unlikely(dma_fence_test_signaled_flag(fence))) > return -EINVAL; Please, drop this change. Andi
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)))
> + 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
>
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
> >
On 11/26/25 17:55, Matthew Brost wrote:
> 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.
Oh, good point! I've totally missed that as well.
But that means that this patch set hasn't even been smoke tested.
Regards,
Christian.
>
> 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
>>>
On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote: > On 11/26/25 17:55, Matthew Brost wrote: > > 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. > > Oh, good point! I've totally missed that as well. Oh dear; I also just saw it when opening the mail client ._. > > But that means that this patch set hasn't even been smoke tested. I've built it and did some basic testing with my Nouveau system. Any suggestions? Do you have a CI that one can trigger? Thx P.
On 11/27/25 10:16, Philipp Stanner wrote: > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote: >> On 11/26/25 17:55, Matthew Brost wrote: >>> 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. >> >> Oh, good point! I've totally missed that as well. > > Oh dear; I also just saw it when opening the mail client ._. > >> >> But that means that this patch set hasn't even been smoke tested. > > I've built it and did some basic testing with my Nouveau system. Any > suggestions? Do you have a CI that one can trigger? DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that. But even running Nouveau should have found this since basically no fence at would signal any more. Regards, Christian. > > Thx > P.
On Thu, 2025-11-27 at 11:01 +0100, Christian König wrote: > On 11/27/25 10:16, Philipp Stanner wrote: > > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote: > > > On 11/26/25 17:55, Matthew Brost wrote: > > > > 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. > > > > > > Oh, good point! I've totally missed that as well. > > > > Oh dear; I also just saw it when opening the mail client ._. > > > > > > > > But that means that this patch set hasn't even been smoke tested. > > > > I've built it and did some basic testing with my Nouveau system. Any > > suggestions? Do you have a CI that one can trigger? > > DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that. > > But even running Nouveau should have found this since basically no fence at would signal any more. They will signal and execute their callbacks, but all is_signaled() checks are then broken. Thx for the feedback, I'll be more careful with changes like those and test more extensively. P.
On Thu, Nov 27, 2025 at 11:16:26AM +0100, Philipp Stanner wrote: > On Thu, 2025-11-27 at 11:01 +0100, Christian König wrote: > > On 11/27/25 10:16, Philipp Stanner wrote: > > > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote: > > > > On 11/26/25 17:55, Matthew Brost wrote: > > > > > 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. > > > > > > > > Oh, good point! I've totally missed that as well. > > > > > > Oh dear; I also just saw it when opening the mail client ._. > > > > > > > > > > > But that means that this patch set hasn't even been smoke tested. > > > > > > I've built it and did some basic testing with my Nouveau system. Any > > > suggestions? Do you have a CI that one can trigger? Intel's Xe list will trigger CI - you are authorized to trigger that flow. Navigate to Intel's list patchworks [1], find your series and see the results. It is noisy, so also certainly to say failure but I let you know if anything is actually a problem. Matt [1] https://patchwork.freedesktop.org/project/intel-xe/series/?ordering=-last_updated > > > > DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that. > > > > But even running Nouveau should have found this since basically no fence at would signal any more. > > They will signal and execute their callbacks, but all is_signaled() > checks are then broken. > > Thx for the feedback, I'll be more careful with changes like those and > test more extensively. > > P.
© 2016 - 2025 Red Hat, Inc.