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