lkml.org 
[lkml]   [2009]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Regression X Hangs at bootup -- PATCH
On Mon, 06 Apr 2009 19:03:55 -0700
Eric Anholt <eric@anholt.net> wrote:

> Nice catch! Thanks. I did some cleanup that brings it more in line
> with style elsewhere in the code and cuts some of the gratuitous
> looking changes. Would you be OK with these changes rolled into your
> original diff?

i take it, you appended the endresult?

i'm ok with it, it's less invasive. but i think your
i915_gem_put_relocs part is wrong. (see below)


>
> drivers/gpu/drm/i915/i915_gem.c | 34
> ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
> int length)
> {
> char __iomem *vaddr;
> - int ret;
> + int unwritten;
>
> vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> KM_USER0); if (vaddr == NULL)
> return -ENOMEM;
> - ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> length);
> + unwritten = __copy_to_user_inatomic(data, vaddr +
> page_offset, length); kunmap_atomic(vaddr, KM_USER0);
>
> - return ret;
> + if (unwritten)
> + return -EFAULT;
> +
> + return 0;
> }

yep thats ok.

>
> static inline int
> @@ -3000,13 +3003,13 @@ i915_gem_get_relocs_from_user(struct
> drm_i915_gem_exec_object *exec_list, drm_free(*relocs, reloc_count *
> sizeof(**relocs), DRM_MEM_DRIVER);
> *relocs = NULL;
> - return ret;
> + return -EFAULT;
> }
>
> reloc_index += exec_list[i].relocation_count;
> }
>
> - return ret;
> + return 0;
> }
>

right.

> static int
> @@ -3015,23 +3018,28 @@ i915_gem_put_relocs_to_user(struct
> drm_i915_gem_exec_object *exec_list, struct
> drm_i915_gem_relocation_entry *relocs) {
> uint32_t reloc_count = 0, i;
> - int ret;
> + int ret = 0;
>
> for (i = 0; i < buffer_count; i++) {
> struct drm_i915_gem_relocation_entry __user
> *user_relocs;
> + int unwritten;
>
> user_relocs = (void __user
> *)(uintptr_t)exec_list[i].relocs_ptr;
> - if (ret == 0) {
> - ret = copy_to_user(user_relocs,
> - &relocs[reloc_count],
> -
> exec_list[i].relocation_count *
> - sizeof(*relocs));
> + unwritten = copy_to_user(user_relocs,
> + &relocs[reloc_count],
> +
> exec_list[i].relocation_count *
> + sizeof(*relocs));
> +
> + if (unwritten) {
> + ret = -EFAULT;
> + goto err;
> }
>
> reloc_count += exec_list[i].relocation_count;
> }
>

i wondered too at first about the if (ret == 0) part, but you need the
whole reloc_count to free everything in the next part:
> +err:
> drm_free(relocs, reloc_count * sizeof(*relocs),
> DRM_MEM_DRIVER);
> return ret;


so i think, this would be a memleak in the error-case (if it ever
happens)


> @@ -3306,10 +3314,12 @@ err:
> (uintptr_t) args->buffers_ptr,
> exec_list,
> sizeof(*exec_list) *
> args->buffer_count);
> - if (ret)
> + if (ret) {
> + ret = -EFAULT;
> DRM_ERROR("failed to copy %d exec entries "
> "back to user (%d)\n",
> args->buffer_count, ret);
> + }
> }
>
> /* Copy the updated relocations out regardless of current
> error




[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2009-04-07 09:27    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site