Christian, Ack to merge this through drm-misc-next, or do you want to pick it up for dma-buf? Thanks, Thomas On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote: > On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote: > > Daniel, > > > > On 4/28/23 14:52, Thomas Hellström wrote: > > > Condsider the following call sequence: > > > > > > /* Upper layer */ > > > dma_fence_begin_signalling(); > > > lock(tainted_shared_lock); > > > /* Driver callback */ > > > dma_fence_begin_signalling(); > > > ... > > > > > > The driver might here use a utility that is annotated as intended > > > for the > > > dma-fence signalling critical path. Now if the upper layer isn't > > > correctly > > > annotated yet for whatever reason, resulting in > > > > > > /* Upper layer */ > > > lock(tainted_shared_lock); > > > /* Driver callback */ > > > dma_fence_begin_signalling(); > > > > > > We will receive a false lockdep locking order violation > > > notification from > > > dma_fence_begin_signalling(). However entering a dma-fence > > > signalling > > > critical section itself doesn't block and could not cause a > > > deadlock. > > > > > > So use a successful read_trylock() annotation instead for > > > dma_fence_begin_signalling(). That will make sure that the > > > locking order > > > is correctly registered in the first case, and doesn't register > > > any > > > locking order in the second case. > > > > > > The alternative is of course to make sure that the "Upper layer" > > > is always > > > correctly annotated. But experience shows that's not easily > > > achievable > > > in all cases. > > > > > > Signed-off-by: Thomas Hellström > > > <thomas.hellstrom@xxxxxxxxxxxxxxx> > > > > Resurrecting the discussion on this one. I can't see a situation > > where we > > would miss *relevant* locking > > order violation warnings with this patch. Ofc if we have a > > scheduler > > annotation patch that would work fine as well, but the lack of > > annotation in > > the scheduler callbacks is really starting to hurt us. > > Yeah this is just a bit too brain-melting to review, but I concur > now. > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > I think what would help is some lockdep selftests to check that we > both > catch the stuff we want to, and don't incur false positives. Maybe > with a > plea that lockdep should have some native form of cross-release > annotations ... > > But definitely seperate patch set, since it might take a few rounds > of > review by lockdep folks. > -Sima > > > > > Thanks, > > > > Thomas > > > > > > > > > --- > > > drivers/dma-buf/dma-fence.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > fence.c > > > index f177c56269bb..17f632768ef9 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void) > > > if (in_atomic()) > > > return true; > > > - /* ... and non-recursive readlock */ > > > - lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, > > > _RET_IP_); > > > + /* ... and non-recursive successful read_trylock */ > > > + lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, > > > _RET_IP_); > > > return false; > > > } > > > @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void) > > > lock_map_acquire(&dma_fence_lockdep_map); > > > lock_map_release(&dma_fence_lockdep_map); > > > if (tmp) > > > - lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, > > > NULL, _THIS_IP_); > > > + lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, > > > NULL, _THIS_IP_); > > > } > > > #endif >
