Christian, Ping? On Wed, 2024-08-14 at 10:37 +0200, Thomas Hellström wrote: > 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 > > >
