RESEND Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()

[Christian König]

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 at 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 at 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