Re: Regression X Hangs at bootup -- PATCH

[Eric Anholt]

On Tue, 2009-04-07 at 09:23 +0200, Florian Mickler wrote:
> On Mon, 06 Apr 2009 19:03:55 -0700
> Eric Anholt <eric@xxxxxxxxxx> 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)

drm_free's other arguments are unused memory debug leftovers.  I've got
a patch I need to push at airlied to remove
drm_malloc/drm_calloc/drm_free.

-- 
Eric Anholt
eric@xxxxxxxxxx                         eric.anholt@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part