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@xxxxxxxxxxxxxxx>
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Cc: Christian König <christian.koenig@xxxxxxx>
>> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
>> Cc: linux-media@xxxxxxxxxxxxxxx
>> Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
>> Cc: Huang Rui <ray.huang@xxxxxxx>
>> Cc: Eric Anholt <eric@xxxxxxxxxx>
>> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
>> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
>> Cc: Rob Herring <robh@xxxxxxxxxx>
>> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
>> Cc: Russell King <linux+etnaviv@xxxxxxxxxxxxxxx>
>> Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
>> Cc: Rob Clark <robdclark@xxxxxxxxx>
>> Cc: Sean Paul <sean@xxxxxxxxxx>
>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> Acked-by: Christian König <christian.koenig@xxxxxxx>
>
>> ---
>> 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@xxxxxxxxxxxxxxx>
typo in patch 3 btw :)
