lock/unlock mismatch in ttm_bo.c

[Chunming Zhou]

On 2018年01月24日 16:58, Christian König wrote:
> NAK, that doesn't looks correct to me. unlock_resv means that the 
> function is allowed to unlock the reservation.
>
> So the original logic seems to do exactly what it is supposed to do. 
> Please keep in mind that reservation_object_trylock returns boolean, 
> not negative error code.
'unlock_resv = true' means ttm_bo_cleanup_refs() is locked outside, then 
reservation_object_trylock will return false, then original code 'if 
(unlock_resv && !reservation_object_trylock(bo->resv))' will always be 
true, and then return directly.
Is my understanding right? is that expected?

Regards,
David Zhou

>
> Regards,
> Christian.
>
> Am 24.01.2018 um 06:46 schrieb Chunming Zhou:
>> update the fix.
>>
>>
>> On 2018年01月24日 11:09, Chunming Zhou wrote:
>>> Hi Tom,
>>>
>>> Your change looks ok, as Roger suggested, you can send both dri and 
>>> amd mail lists.
>>>
>>> In addition, when I review your patches, I found a bug as the 
>>> attached, you can send it together with yours if you think that's a 
>>> right fix.
>>>
>>> Regards,
>>>
>>> David Zhou
>>>
>>>
>>> On 2018年01月24日 03:25, Tom St Denis wrote:
>>>> On 22/01/18 01:42 AM, Chunming Zhou wrote:
>>>>>
>>>>>
>>>>> On 2018年01月20日 02:23, Tom St Denis wrote:
>>>>>> On 19/01/18 01:14 PM, Tom St Denis wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> In the function ttm_bo_cleanup_refs() it seems possible to get 
>>>>>>> to line 551 without entering the block on 516 which means you'll 
>>>>>>> be unlocking a mutex that wasn't locked.
>>>>>>>
>>>>>>> Now it might be that in the course of the API this pattern 
>>>>>>> cannot be expressed but it's not clear from the function alone 
>>>>>>> that that is the case.
>>>>>>
>>>>>>
>>>>>> Looking further it seems the behaviour depends on locking in 
>>>>>> parent callers.  That's kinda a no-no right? Shouldn't the lock 
>>>>>> be taken/released in the same function ideally?
>>>>> Same feelings
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>
>>>> Attached is a patch that addresses this.
>>>>
>>>> I can't see any obvious race in functions that call 
>>>> ttm_bo_cleanup_refs() between the time they let go of the lock and 
>>>> the time it's taken again in the call.
>>>>
>>>> Running it on my system doesn't produce anything notable though the 
>>>> KASAN with DRI_PRIME=1 issue is still there (this patch neither 
>>>> causes that nor fixes it).
>>>>
>>>> Tom
>>>
>>
>