On 24/10/2025 11:59, Philipp Stanner wrote:
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 tried to document it in the very patch which added it. Both in the
commit message and in the large sticky-outy comments added to these helpers:
"""
* dma_fence_driver_name - Access the driver name
* @fence: the fence to query
*
* Returns a driver name backing the dma-fence implementation.
*
* IMPORTANT CONSIDERATION:
* Dma-fence contract stipulates that access to driver provided data
(data not
* directly embedded into the object itself), such as the
&dma_fence.lock and
* memory potentially accessed by the &dma_fence.ops functions, is
forbidden
* after the fence has been signalled. Drivers are allowed to free that
data,
* and some do.
*
* To allow safe access drivers are mandated to guarantee a RCU grace
period
* between signalling the fence and freeing said data.
*
* As such access to the driver name is only valid inside a RCU locked
section.
* The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block
guarded
* by the &rcu_read_lock and &rcu_read_unlock pair.
"""
I started with names and Christian has recently continued with ops.
Reported-by: Danilo Krummrich <[email protected]>
Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the
rules")
Signed-off-by: Philipp Stanner <[email protected]>
---
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…)
Sounds like a good plan to start without the problematic parts whenever
possible. More than that I cannot comment since I have no idea how rust
stuff will work and interact with the existing uapi entry points such as
the sync file.
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.
Driver and timeline usually come together so the signalled info is
already there ie. "detached-driver signaled-timeline". For example in
debugfs via dma_fence_describe().
So changing that to "driver-whose-fence-is-already-signalled
signaled-timeline" still looks too much.
Also, the short name can be reduced from a verbose starting point
similar to yours:
"unknown-driver-is-detached-from-the-signaled-fence"
"driver-detached-from-the-fence"
"driver-detached"
Or keep "detached-driver" as good enough. Mea culpa for typing it up
transposed. :)
Regards,
Tvrtko
P.
Regards,
Tvrtko
}
EXPORT_SYMBOL(dma_fence_driver_name);
