[PATCH 1/3] drm/i915: Refactor eb_relocate_vma for clarity

[Andi Shyti]

Hi Sebastian,

> @@ -1529,80 +1529,89 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  	struct drm_i915_gem_relocation_entry __user *urelocs =
>  		u64_to_user_ptr(entry->relocs_ptr);
>  	unsigned long remain = entry->relocation_count;
> +	int ret = 0;
>  
> -	if (unlikely(remain > N_RELOC(INT_MAX)))
> -		return -EINVAL;
> +	if (unlikely(remain > N_RELOC(INT_MAX))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Why? This doesn't look clearer to me.

>  
>  	/*
>  	 * We must check that the entire relocation array is safe
>  	 * to read. However, if the array is not writable the user loses
>  	 * the updated relocation values.
>  	 */
> -	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
> -		return -EFAULT;
> +	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs)))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}

same.

> -	do {
> -		struct drm_i915_gem_relocation_entry *r = stack;
> +	while (remain > 0) {
>  		unsigned int count =
>  			min_t(unsigned long, remain, ARRAY_SIZE(stack));
>  		unsigned int copied;
> -
>  		/*
>  		 * This is the fast path and we cannot handle a pagefault
> -		 * whilst holding the struct mutex lest the user pass in the
> -		 * relocations contained within a mmaped bo. For in such a case
> -		 * we, the page fault handler would call i915_gem_fault() and
> -		 * we would try to acquire the struct mutex again. Obviously
> -		 * this is bad and so lockdep complains vehemently.
> +		 * whilst holding the struct mutex lest the user pass in
> +		 * the relocations contained within a mmaped bo. For in
> +		 * such a case we, the page fault handler would call
> +		 * i915_gem_fault() and we would try to acquire the
> +		 * struct mutex again. Obviously this is bad and so
> +		 * lockdep complains vehemently.
>  		 */

Why have you done this change?

>  		pagefault_disable();
> -		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> +		copied = __copy_from_user_inatomic(stack, urelocs,
> +						   count * sizeof(stack[0]));

No need, the maximum allowed by the documentation is 100
characters.

>  		pagefault_enable();
> +
>  		if (unlikely(copied)) {
> -			remain = -EFAULT;
> +			ret = -EFAULT;
>  			goto out;
>  		}
>  
> -		remain -= count;
> -		do {
> +		for (unsigned int i = 0; i < count; ++i) {

Please don't declare variables inside the for loop.

> +			struct drm_i915_gem_relocation_entry *r =
> +				&stack[i];
>  			u64 offset = eb_relocate_entry(eb, ev, r);
>  
> -			if (likely(offset == 0)) {
> -			} else if ((s64)offset < 0) {
> -				remain = (int)offset;
> +			if (likely(offset == 0))
> +				continue;

you can leave a blank line here.

> +			if (unlikely((s64)offset < 0)) {
> +				ret = (int)offset;
>  				goto out;

...

>  out:
>  	reloc_cache_reset(&eb->reloc_cache, eb);
> -	return remain;
> +	return ret;

now, this function is also returning a different value, not the
remaining bytes anymore but 0 on success and -error on failure.
Is this something you wanted?

Overall the patch is not acceptable yet. You mixed several
cleanups while I would rather prefer one cleanup type per patch,
this make is easier to review and trace in the history.

Andi

>  }
>  
>  static int
> -- 
> 2.34.1
>