Re: [RFC PATCH 2/4] rust: sync: Add dma_fence abstractions

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Feb 10, 2026 at 01:21:47PM +0100, Boris Brezillon wrote:
> On Tue, 10 Feb 2026 11:45:36 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Tue, Feb 10, 2026 at 12:34:32PM +0100, Boris Brezillon wrote:
> > > On Tue, 10 Feb 2026 10:15:04 +0000
> > > Alice Ryhl <aliceryhl@google.com> wrote:
> > >   
> > > > impl MustBeSignalled<'_> {
> > > >     /// Drivers generally should not use this one.
> > > >     fn i_promise_it_will_be_signalled(self) -> WillBeSignalled { ... }
> > > > 
> > > >     /// One way to ensure the fence has been signalled is to signal it.
> > > >     fn signal_fence(self) -> WillBeSignalled {
> > > >         self.fence.signal();
> > > >         self.i_promise_it_will_be_signalled()
> > > >     }
> > > > 
> > > >     /// Another way to ensure the fence will be signalled is to spawn a
> > > >     /// workqueue item that promises to signal it.
> > > >     fn transfer_to_wq(
> > > >         self,
> > > >         wq: &Workqueue,
> > > >         item: impl DmaFenceWorkItem,
> > > >     ) -> WillBeSignalled {
> > > >         // briefly obtain the lock class of the wq to indicate to
> > > >         // lockdep that the signalling path "blocks" on arbitrary jobs
> > > >         // from this wq completing
> > > >         bindings::lock_acquire(&wq->key);
> > > >         bindings::lock_release(&wq->key);  
> > > 
> > > Sorry, I'm still trying to connect the dots here. I get that the intent
> > > is to ensure the pseudo-lock ordering is always:
> > >   
> > >    -> dma_fence_lockdep_map
> > >       -> wq->lockdep_map  
> > > 
> > > but how can this order be the same in the WorkItem execution path? My
> > > interpretation of process_one_work() makes me think we'll end up with
> > >   
> > >   -> wq->lockdep_map
> > >     -> work->run()
> > >        -> WorkItem::run()
> > >           -> dma_fence_lockdep_map
> > >              -> DmaFenceSignalingWorkItem::run()  
> > >                 ...
> > > 
> > > Am I missing something? Is there a way you can insert the
> > > dma_fence_lockdep_map acquisition before the wq->lockdep_map one in the
> > > execution path?  
> > 
> > Conceptually, the dma_fence_lockdep_map is already taken by the time you
> > get to WorkItem::run() because it was taken all the way back in the
> > ioctl, so WorkItem::run() does not need to reacquire it.
> > 
> > Now, of course that does not translate cleanly to how lockdep does
> > things, so in lockdep we do have to re-acquire it in WorkItem::run().
> > You can do that by setting the trylock bit when calling lock_acquire()
> > on dma_fence_lockdep_map. This has the correct semantics because trylock
> > does not create an edge from wq->lockdep_map to dma_fence_lockdep_map.
> 
> Ah, I never noticed dma_fence_begin_signalling() was recording a
> try_lock not a regular lock. I guess it would do then.

Calling dma_fence_begin_signalling() never blocks so 'trylock' is the
right option.


Actually, that raises one question for me. Right now it's implemented
like this:

	/* explicitly nesting ... */
	if (lock_is_held_type(&dma_fence_lockdep_map, 1))
		return true;

	/* ... and non-recursive successful read_trylock */
	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_);

but why not drop the explicit nest check and pass `2` for read instead?

	lock_acquire(&dma_fence_lockdep_map, 0, 1, 2, 1, NULL, _RET_IP_);

Note that passing 2 means that you're taking a readlock with
same-instance recursion allowed. This way you could get rid of the
cookie entirely because you're just taking the lock multiple times, and
lockdep will count how many times it's taken for you.

Alice