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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
