[PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

On 02/10/2017 11:37 AM, Christian König wrote:
> Am 10.02.2017 um 11:22 schrieb Samuel Pitoiset:
>>
>>
>> On 02/10/2017 11:19 AM, Christian König wrote:
>>> Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>>>>
>>>>
>>>> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>>>>
>>>>>
>>>>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>>>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>>>>> at it, add a missing break in the default case.
>>>>>>
>>>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index 1dc59aafec71..ae4658a10e2c 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>           info.alignment = robj->tbo.mem.page_alignment <<
>>>>>> PAGE_SHIFT;
>>>>>>           info.domains = robj->prefered_domains;
>>>>>>           info.domain_flags = robj->flags;
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>>>>               r = -EFAULT;
>>>>> NAK, your this change will break our previous deadlock fix for
>>>>> ww_mutex
>>>>> and mm->mmap_sem if I remember correctly.
>>>>
>>>> Mmh, really? Can you pinpoint the commit? I don't see anything obvious
>>>> in the history about that.
>>>
>>> David is right here. Not sure when we fixed that, but in general calling
>>> copy_to/from_user while a BO is reserved is illegal.
>>>
>>> Otherwise somebody could send the kernel a memory mapped BO as address
>>> and the copy_to/from_user would just deadlock because it tries to
>>> reserve a BO while another (or the same) BO is already reserved in the
>>> call path.
>>
>> Okay, makes more sense.
>>
>> Thanks for the review. My goal is not to introduce new regressions but
>> I didn't know that.
>>
>> Maybe this should be explained in the code? Just a thought.
>
> We do have a comment in the TTM mapping code I think.
>
> But copy_to/from_user and BO reservation is just so common that we would
> need to add the same comment on a whole bunch of different places and
> that doesn't make to much sense.
>
> But in this particular case a comment might make sense because I think
> somebody proposed the same patch before. Ah! Enlightenment, that also
> explains why you can't find it in the history. We just rejected the same
> patch multiple times now :)

Yeah, I bet someone else will submit the same patch in few weeks/months 
without a comment explaining why we shouldn't change it. :)

>
> Regards,
> Christian.
>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>>           break;
>>>>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>>>>               r = -EPERM;
>>>>>> -            amdgpu_bo_unreserve(robj);
>>>>>>               break;
>>>>>>           }
>>>>>>           robj->prefered_domains = args->value &
>>>>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>           robj->allowed_domains = robj->prefered_domains;
>>>>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>> -
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           break;
>>>>>>       default:
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           r = -EINVAL;
>>>>>> +        break;
>>>>>>       }
>>>>>>   +    amdgpu_bo_unreserve(robj);
>>>>>>   out:
>>>>>>       drm_gem_object_unreference_unlocked(gobj);
>>>>>>       return r;
>>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>