On 13.08.25 10:16, Philipp Stanner wrote:
> On Tue, 2025-08-12 at 16:34 +0200, Christian König wrote:
>> From: Christian König <ckoe...@able.fritz.box>
>
> Is this the correct mail addr? :)
^^ No it isn't and how the heck did that happen?
Looks like my gitconfig is somehow changed, but I have no idea why.
>>
>> We have the re-occurring problem that people try to invent a
>> DMA-fences implementation which signals fences based on an userspace
>> IOCTL.
>>
>> This is well known as source of hard to track down crashes and is
>> documented to be an invalid approach. The problem is that it seems
>> to work during initial testing and only long term tests points out
>> why this can never work correctly.
>>
>> So give at least a warning when people try to signal a fence from
>> task context and not from interrupts or a work item. This check is
>> certainly not perfect but better than nothing.
>>
>> Signed-off-by: Christian König <ckoe...@able.fritz.box>
>> ---
>> drivers/dma-buf/dma-fence.c | 59 +++++++++++++++++++++++++++----------
>> include/linux/dma-fence.h | 9 ++++--
>> 2 files changed, 51 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 3f78c56b58dc..2bce620eacac 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -345,33 +345,23 @@ void __dma_fence_might_wait(void)
>> }
>> #endif
>>
>> -
>> /**
>> - * dma_fence_signal_timestamp_locked - signal completion of a fence
>> + * dma_fence_signal_internal - internal signal completion of a fence
>> * @fence: the fence to signal
>> * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time
>> domain
>> *
>> - * Signal completion for software callbacks on a fence, this will unblock
>> - * dma_fence_wait() calls and run all the callbacks added with
>> - * dma_fence_add_callback(). Can be called multiple times, but since a fence
>> - * can only go from the unsignaled to the signaled state and not back, it
>> will
>> - * only be effective the first time. Set the timestamp provided as the fence
>> - * signal timestamp.
>> - *
>> - * Unlike dma_fence_signal_timestamp(), this function must be called with
>> - * &dma_fence.lock held.
>> + * Internal signal the dma_fence without error checking. Should *NEVER* be
>> used
>> + * by drivers or external code directly.
>
> s/Internal/Internally
>
>> *
>> * Returns 0 on success and a negative error value when @fence has been
>> * signalled already.
>> */
>> -int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>> - ktime_t timestamp)
>> +int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp)
>> {
>> struct dma_fence_cb *cur, *tmp;
>> struct list_head cb_list;
>>
>> lockdep_assert_held(fence->lock);
>> -
>> if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>> &fence->flags)))
>> return -EINVAL;
>> @@ -390,7 +380,46 @@ int dma_fence_signal_timestamp_locked(struct dma_fence
>> *fence,
>>
>> return 0;
>> }
>> -EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>> +EXPORT_SYMBOL(dma_fence_signal_internal);
>
> If it must only be used internally, can it be kept private, without
> exporting the symbol?
I have gone back and force about that as well.
We would then need to uninline dma_fence_is_signaled().
>> +
>> +/**
>> + * dma_fence_signal_timestamp_locked - signal completion of a fence
>> + * @fence: the fence to signal
>> + * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time
>> domain
>> + *
>> + * Signal completion for software callbacks on a fence, this will unblock
>> + * dma_fence_wait() calls and run all the callbacks added with
>> + * dma_fence_add_callback(). Can be called multiple times, but since a fence
>> + * can only go from the unsignaled to the signaled state and not back, it
>> will
>> + * only be effective the first time. Set the timestamp provided as the fence
>> + * signal timestamp.
>> + *
>> + * Unlike dma_fence_signal_timestamp(), this function must be called with
>> + * &dma_fence.lock held.
>> + *
>> + * Returns 0 on success and a negative error value when @fence has been
>> + * signalled already.
>> + */
>> +int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>> + ktime_t timestamp)
>> +{
>> + /*
>> + * We have the re-occurring problem that people try to invent a
>> + * DMA-fences implementation which signals fences based on an userspace
>> + * IOCTL.
>> + *
>> + * This is well known as source of hard to track down crashes and is
>> + * documented to be an invalid approach. The problem is that it seems
>> + * to work during initial testing and only long term tests points out
>> + * why this can never work correctly.
>> + *
>> + * So give at least a warning when people try to signal a fence from
>> + * task context and not from interrupts or a work item. This check is
>> + * certainly not perfect but better than nothing.
>> + */
>> + WARN_ON_ONCE(!in_interrupt() && !current_work());
>> + return dma_fence_signal_internal(fence, timestamp);
>> +}
>
> So this now is the point to decide what we want: do you want to *allow*
> drivers to do it, or want to *prohibit* it?
>
> If you want to prohibit it, then (additionally) returning an error code
> here would make sense.
I'm actually trying to remove the return value for quite a while now.
IIRC nobody is actually using it any more since it doesn't really signal an
error condition.
Thanks,
Christian.
>
>
> P.
>
>>
>> /**
>> * dma_fence_signal_timestamp - signal completion of a fence
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 64639e104110..8dbcd66989b8 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -369,6 +369,7 @@ 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,
>> ktime_t timestamp);
>> +int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp);
>> signed long dma_fence_default_wait(struct dma_fence *fence,
>> bool intr, signed long timeout);
>> int dma_fence_add_callback(struct dma_fence *fence,
>> @@ -422,7 +423,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>> return true;
>>
>> if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> - dma_fence_signal_locked(fence);
>> + dma_fence_signal_internal(fence, ktime_get());
>> return true;
>> }
>>
>> @@ -448,11 +449,15 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>> static inline bool
>> dma_fence_is_signaled(struct dma_fence *fence)
>> {
>> + unsigned long flags;
>> +
>> if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> return true;
>>
>> if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> - dma_fence_signal(fence);
>> + spin_lock_irqsave(fence->lock, flags);
>> + dma_fence_signal_internal(fence, ktime_get());
>> + spin_unlock_irqrestore(fence->lock, flags);
>> return true;
>> }
>>
>
