lock/unlock mismatch in ttm_bo.c

[Tom St Denis]

On 24/01/18 03:58 AM, 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.

If the cleanup_refs() function assumes the lock is held it shouldn't 
release it then.  My only goal here was to cleanup the lock usage so 
that it's only manipulated in a single context.

The lock is unlocked a bit before the function ends do the other ttm 
functions cleanup_refs() call require the ability to take the lock?

Tom


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