| Message ID | 20230428125233.228353-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() | expand |
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 "Upper layer" here currently being the drm scheduler and "Driver callback" being an xe scheduler callback. While opt-in annotating the drm scheduler would achieve the same result, I think this patch should be considered anyway, as I don't think we will miss any true lockdep violations as a result of it. /Thomas
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@linux.intel.com> 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. 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 >
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@linux.intel.com> > > 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@ffwll.ch> 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
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@linux.intel.com> > > > > 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@ffwll.ch> > > 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 >
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@linux.intel.com> > > > > > > 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@ffwll.ch> > > > > > > > > > > > > > > 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 > > >
Sorry, somehow completely missed that. Feel free to push it to drm-misc-next. Christian. Am 18.09.24 um 14:34 schrieb Thomas Hellström: > 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@linux.intel.com> >>>> 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@ffwll.ch> >> >> >> >> >> >> >> >> >> >>> 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
On Wed, 2024-09-18 at 15:18 +0200, Christian König wrote: > Sorry, somehow completely missed that. Feel free to push it to > drm-misc-next. > > Christian. Pushed, thanks. /Thomas > > Am 18.09.24 um 14:34 schrieb Thomas Hellström: > > 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@linux.intel.com> > > > > > 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@ffwll.ch> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 >
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
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@linux.intel.com> --- drivers/dma-buf/dma-fence.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)