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