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
>
