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 <[email protected]>
---
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
