Am 24.01.2018 um 10:10 schrieb 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...". No wait a moment. Your initial assumption seems to be incorrect: 'unlock_resv = true' means ttm_bo_cleanup_refs() is locked outside When unlock_resv = false then that means ttm_bo_cleanup_refs() is locked outside. And when we assume this, the original code is indeed right. Christian. > > 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 >>>>> >>>> >>> >> >
