On Mon, 2025-12-01 at 14:23 +0100, Christian König wrote:
> On 12/1/25 11:50, Philipp Stanner wrote:
> > The overwhelming majority of users of dma_fence signaling functions
> > don't care about whether the fence had already been signaled by someone
> > else. Therefore, the return code shall be removed from those functions.
> >
> > For the few users who rely on the check, a new, specialized function
> > shall be provided.
> >
> > Add dma_fence_check_and_signal(), which signals a fence if it had not
> > yet been signaled, and informs the user about that.
> >
> > Add a counter part, dma_fence_check_and_signal_locked(), which doesn't
> > take the spinlock.
> >
> > Signed-off-by: Philipp Stanner <[email protected]>
> > ---
> > drivers/dma-buf/dma-fence.c | 44 +++++++++++++++++++++++++++++++++++++
> > include/linux/dma-fence.h | 2 ++
> > 2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 96d72ffc0750..146de62887cf 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -445,6 +445,50 @@ int dma_fence_signal_locked(struct dma_fence *fence)
> > }
> > EXPORT_SYMBOL(dma_fence_signal_locked);
> >
> > +/**
> > + * dma_fence_check_and_signal_locked - signal the fence if it's not yet
> > signaled
> > + * @fence: the fence to check and signal
> > + *
> > + * Checks whether a fence was signaled and signals it if it was not yet
> > signaled.
> > + *
> > + * Unlike dma_fence_check_and_signal(), this function must be called with
> > + * &struct dma_fence.lock being held.
> > + *
> > + * Return: true if fence has been signaled already, false otherwise.
> > + */
> > +bool dma_fence_check_and_signal_locked(struct dma_fence *fence)
>
> I'm seriously considering to nuke all the unlocked variants of dma_fence
> functions and just make it mandatory for callers to grab the lock manually.
>
You mean "nuke the *locked* variants.
Why, though? Aren't they enough for most users?
I suppose you have all those subtle races in mind..
> > +{
> > + bool ret;
> > +
> > + ret = dma_fence_test_signaled_flag(fence);
> > + dma_fence_signal_locked(fence);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(dma_fence_check_and_signal_locked);
> > +
> > +/**
> > + * dma_fence_check_and_signal - signal the fence if it's not yet signaled
> > + * @fence: the fence to check and signal
> > + *
> > + * Checks whether a fence was signaled and signals it if it was not yet
> > signaled.
> > + * All this is done in a race-free manner.
> > + *
> > + * Return: true if fence has been signaled already, false otherwise.
> > + */
> > +bool dma_fence_check_and_signal(struct dma_fence *fence)
>
> So I think we should name this one here dma_fence_check_and_signal_unlocked()
> and drop the postfix from the locked variant.
postfix?
Well, now, IDK. Can't we, for this series, keep the _locked() variant
so that it's congruent with all the other dma_fence code?
And then later if you want to force manual locking you can add that
kernel-wide in a separate series, since it'll be a discussion-worthy,
bigger chunk of work.
That's cleaner, and my series here won't prevent that once merged.
>
> > +{
> > + unsigned long flags;
> > + bool ret;
> > +
> > + spin_lock_irqsave(fence->lock, flags);
> > + ret = dma_fence_check_and_signal_locked(fence);
> > + spin_unlock_irqrestore(fence->lock, flags);
>
> Could this use guard(fence->lock, flags) ?
guard? You mean a lockdep guard? Do you have a pointer to someplace in
dma_fence who does what you mean / want?
P.
>
> Regards,
> Christian.
>
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(dma_fence_check_and_signal);
> > +
> > /**
> > * dma_fence_signal - signal completion of a fence
> > * @fence: the fence to signal
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 19972f5d176f..0504afe52c2a 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -365,6 +365,8 @@ static inline void __dma_fence_might_wait(void) {}
> > #endif
> >
> > int dma_fence_signal(struct dma_fence *fence);
> > +bool dma_fence_check_and_signal(struct dma_fence *fence);
> > +bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
> > int dma_fence_signal_locked(struct dma_fence *fence);
> > int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
> > int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>
