Re: [PATCH 04/15] dma-buf: detach fence ops on signal

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

On 10/17/25 11:14, Philipp Stanner wrote:
> On Mon, 2025-10-13 at 15:48 +0200, Christian König wrote:
>> When neither a release nor a wait operation is specified it is possible
>> to let the dma_fence live on independent of the module who issued it.
>>
>> This makes it possible to unload drivers and only wait for all their
>> fences to signal.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/dma-buf/dma-fence.c | 16 ++++++++++++----
>>  include/linux/dma-fence.h   |  4 ++--
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 982f2b2a62c0..39f73edf3a33 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -374,6 +374,14 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>  				      &fence->flags)))
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * When neither a release nor a wait operation is specified set the ops
>> +	 * pointer to NULL to allow the fence structure to become independent
>> +	 * who originally issued it.
>> +	 */
>> +	if (!fence->ops->release && !fence->ops->wait)
>> +		RCU_INIT_POINTER(fence->ops, NULL);
> 
> OK, so the basic idea is that still living fences can't access driver
> data or driver code anymore after the driver is unloaded. Good and
> well, nice idea. We need something like that in Rust, too.
> 
> That's based on the rule that the driver, on unload, must signal all
> the fences. Also OK.
> 
> However, how can that possibly fly by relying on the release callback
> not being implemented? How many users don't need it, and could those
> who implement release() be ported to.. sth else?

As far as I can see the only one who really needs the ->release callback for technical reasons is the DRM scheduler fence and the dma_fence_array and dma_fence_chain containers. 

For the DRM scheduler fence it is just the finished fence which needs to drop the reference to the scheduled fence because we can now be sure that nobody can cast the fence any more.

For the dma_fence_array we could actually clean up the state on signaling, but that would need some more cleanup in the framework.

For the dma_fence_chain it is a must have to avoid potential kernel stack overrun.

Apart from that all drivers should be able to cleanup their internal state necessary for signaling when they actually signal.

Regards,
Christian.

> 
> 
> P.
> 
>> +
>>  	/* Stash the cb_list before replacing it with the timestamp */
>>  	list_replace(&fence->cb_list, &cb_list);
>>  
>> @@ -513,7 +521,7 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>>  	rcu_read_lock();
>>  	ops = rcu_dereference(fence->ops);
>>  	trace_dma_fence_wait_start(fence);
>> -	if (ops->wait) {
>> +	if (ops && ops->wait) {
>>  		/*
>>  		 * Implementing the wait ops is deprecated and not supported for
>>  		 * issuer independent fences, so it is ok to use the ops outside
>> @@ -578,7 +586,7 @@ void dma_fence_release(struct kref *kref)
>>  	}
>>  
>>  	ops = rcu_dereference(fence->ops);
>> -	if (ops->release)
>> +	if (ops && ops->release)
>>  		ops->release(fence);
>>  	else
>>  		dma_fence_free(fence);
>> @@ -614,7 +622,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>>  
>>  	rcu_read_lock();
>>  	ops = rcu_dereference(fence->ops);
>> -	if (!was_set && ops->enable_signaling) {
>> +	if (!was_set && ops && ops->enable_signaling) {
>>  		trace_dma_fence_enable_signal(fence);
>>  
>>  		if (!ops->enable_signaling(fence)) {
>> @@ -1000,7 +1008,7 @@ void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>>  
>>  	rcu_read_lock();
>>  	ops = rcu_dereference(fence->ops);
>> -	if (ops->set_deadline && !dma_fence_is_signaled(fence))
>> +	if (ops && ops->set_deadline && !dma_fence_is_signaled(fence))
>>  		ops->set_deadline(fence, deadline);
>>  	rcu_read_unlock();
>>  }
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 38421a0c7c5b..e1ba1d53de88 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -425,7 +425,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>>  
>>  	rcu_read_lock();
>>  	ops = rcu_dereference(fence->ops);
>> -	if (ops->signaled && ops->signaled(fence)) {
>> +	if (ops && ops->signaled && ops->signaled(fence)) {
>>  		rcu_read_unlock();
>>  		dma_fence_signal_locked(fence);
>>  		return true;
>> @@ -461,7 +461,7 @@ dma_fence_is_signaled(struct dma_fence *fence)
>>  
>>  	rcu_read_lock();
>>  	ops = rcu_dereference(fence->ops);
>> -	if (ops->signaled && ops->signaled(fence)) {
>> +	if (ops && ops->signaled && ops->signaled(fence)) {
>>  		rcu_read_unlock();
>>  		dma_fence_signal(fence);
>>  		return true;
>