Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 20-11-2019 om 12:30 schreef Christian König:
> Am 19.11.19 um 22:08 schrieb Daniel Vetter:
>> Semnatically it really doesn't matter where we grab the ticket. But
>> since the ticket is a fake lockdep lock, it matters for lockdep
>> validation purposes.
>>
>> This means stuff like grabbing a ticket and then doing
>> copy_from/to_user isn't allowed anymore. This is a changed compared to
>> the current ttm fault handler, which doesn't bother with having a full
>> reservation. Since I'm looking into fixing the TODO entry in
>> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
>> later anyway, better get started. A bit more context on why I'm
>> looking into this: For backwards compat with existing i915 gem code I
>> think we'll have to do full slowpath locking in the i915 equivalent of
>> the eviction code. And with dynamic dma-buf that will leak across
>> drivers, so another thing we need to standardize and make sure it's
>> done the same way everyway.
>>
>> Unfortunately this means another full audit of all drivers:
>>
>> - gem helpers: acquire_init is done right before taking locks, so no
>>    problem. Same for acquire_fini and unlocking, which means nothing
>>    that's not already covered by the dma_resv_lock rules will be caught
>>    with this extension here to the acquire_ctx.
>>
>> - etnaviv: An absolute massive amount of code is run between the
>>    acquire_init and the first lock acquisition in submit_lock_objects.
>>    But nothing that would touch user memory and could cause a fault.
>>    Furthermore nothing that uses the ticket, so even if I missed
>>    something, it would be easy to fix by pushing the acquire_init right
>>    before the first use. Similar on the unlock/acquire_fini side.
>>
>> - i915: Right now (and this will likely change a lot rsn) the acquire
>>    ctx and actual locks are right next to each another. No problem.
>>
>> - msm has a problem: submit_create calls acquire_init, but then
>>    submit_lookup_objects() has a bunch of copy_from_user to do the
>>    object lookups. That's the only thing before submit_lock_objects
>>    call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
>>    does not have this issue since it copies all the userspace structs
>>    earlier. submit_cleanup does not have any such issues.
>>
>>    With the prep patch to pull out the acquire_ctx and reorder it msm
>>    is going to be safe too.
>>
>> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
>>    Similar on the acquire_fini/ttm_bo_unreserve side.
>>
>> - ttm execbuf utils: acquire context and locking are even in the same
>>    functions here (one function to reserve everything, the other to
>>    unreserve), so all good.
>>
>> - vc4: Another case where acquire context and locking are handled in
>>    the same functions (one function to lock everything, the other to
>>    unlock).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Cc: Huang Rui <ray.huang@amd.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Acked-by: Christian König <christian.koenig@amd.com>
>
>> ---
>>   drivers/dma-buf/dma-resv.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index d3c760e19991..079e38fde33a 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
>>   static void __init dma_resv_lockdep(void)
>>   {
>>       struct mm_struct *mm = mm_alloc();
>> +    struct ww_acquire_ctx ctx;
>>       struct dma_resv obj;
>> +    int ret;
>>         if (!mm)
>>           return;
>> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
>>       dma_resv_init(&obj);
>>         down_read(&mm->mmap_sem);
>> -    ww_mutex_lock(&obj.lock, NULL);
>> +    ww_acquire_init(&ctx, &reservation_ww_class);
>> +    ret = dma_resv_lock(&obj, &ctx);
>> +    if (ret == -EDEADLK)
>> +        dma_resv_lock_slow(&obj, &ctx);
>>       fs_reclaim_acquire(GFP_KERNEL);
>>       fs_reclaim_release(GFP_KERNEL);
>>       ww_mutex_unlock(&obj.lock);
>> +    ww_acquire_fini(&ctx);
>>       up_read(&mm->mmap_sem);
>>      
>>       mmput(mm);
>

For whole series:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

typo in patch 3 btw :)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel