lock/unlock mismatch in ttm_bo.c

[Christian König]

Am 24.01.2018 um 10:06 schrieb 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?

Ah, crap you are right. That must be "!unlock_resv && !reserv...".

But why do you want to set unlock_resv to true?

Regards,
Christian.

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