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