Re: [PATCH] dma-fence: Correct return of dma_fence_driver_name()

[Philipp Stanner <phasta@mailbox.org>]

On Fri, 2025-10-24 at 09:31 +0100, Tvrtko Ursulin wrote:
> 
> On 24/10/2025 08:50, Philipp Stanner wrote:
> > To decouple the dma_fence_ops lifetime from dma_fences lifetime RCU
> > support was added to said function, coupled with using the signaled bit
> > to detect whether the fence_ops might be gone already.
> > 
> > When implementing that a wrong string was set as a default return
> > parameter, indicating that every driver whose fence is already signalled
> > must be detached, which is frankly wrong.
> 
> Depends on how you look at it. After being signaled fence has to be 
> detached from the driver. Ie. nothing belonging to this driver must be 
> accessed via the fence.

Is that even documented btw? Many of the mysterious "dma fence rules"
are often only obtainable by asking Christian & Co

> 
> I started with names and Christian has recently continued with ops.
> 
> > Reported-by: Danilo Krummrich <dakr@kernel.org>
> > Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> > When this was merged, it sadly slipped by me. I think this entire RCU
> > mechanism was / is an overengineered idea.
> > 
> > If we look at who actually uses dma_fence_driver_name() and
> > dma_fence_timeline_name() – functions from which the largest share of
> > the fence_ops vs. fence lifetime issue stems from – we discover that
> > there is a single user:
> > 
> > i915.
> 

[…]

> 
> 
> That would be nice, I also do not see much value in exporting names to 
> userspace. But first more conversation around breaking the sync file ABI 
> needs to happen. I think we had a little bit of it when changing the 
> names of signalled fences and thinking was existing tools which look at 
> the names will mostly survive it. Not sure if they would if unsignalled 
> names would change.

I mean, what you and Christian are addressing in recent weeks are real
problems, and I was / am about to write similar solutions for our Rust
dma_fence.

In the case of those names, however, I'll likely just not support that
in Rust, saving me from adding those RCU guards and delivering output
of questionable use to users.
(could ofc be added later by someone who really needs it…)

> 
> > 
> > P.
> > ---
> >   drivers/dma-buf/dma-fence.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 3f78c56b58dc..1875a0abebd3 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -1111,7 +1111,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
> >   	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >   		return fence->ops->get_driver_name(fence);
> >   	else
> > -		return "detached-driver";
> > +		return "driver-whose-fence-is-already-signalled";
> 
> IMHO unnecessarily verbose and whether or not changing it to anything
> different warrants a Fixes: tag is debatable.

IMO the output is just wrong and confusing. It's easy to imagine that
some user starts wondering and searching why his driver has been
unloaded, opening support tickets and so on.

Could be less verbose, though. Dunno. I let the maintainer decide.

P.

> 
> Regards,
> 
> Tvrtko
> 
> >   }
> >   EXPORT_SYMBOL(dma_fence_driver_name);
> >   
>