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