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

[Sebastian Brzezinka]

Hi Andi

On Wed Jul 16, 2025 at 1:49 PM UTC, Andi Shyti wrote:
> 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?
Function still returning the same value as before, but now we
don't reuse variable. Regardless, the caller treats any return
value the same. Still, the return value is either 0, an error,
or an offset, just like before.

Other than that, I'll fix the other comment in v2.
Thanks for the review.

-- 
Best regards,
Sebastian