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