[bug report] drm/i915: Pass around sg_table to get_pages/put_pages backend

[bug report] drm/i915: Pass around sg_table to get_pages/put_pages backend

[Dan Carpenter]

[ This code is old, but it's pretty confusing so I decided to send the
  email anyway.  - dan ]

Hello Chris Wilson,

The patch 03ac84f1830e: "drm/i915: Pass around sg_table to
get_pages/put_pages backend" from Oct 28, 2016, leads to the
following static checker warning:

	drivers/gpu/drm/i915/i915_gem_userptr.c:540 __i915_gem_userptr_get_pages_worker()
	warn: passing zero to 'ERR_PTR'

drivers/gpu/drm/i915/i915_gem_userptr.c
   507          ret = -ENOMEM;
                ^^^^^^^^^^^^^^

   508          pinned = 0;
   509  
   510          pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
   511          if (pvec != NULL) {
   512                  struct mm_struct *mm = obj->userptr.mm->mm;
   513                  unsigned int flags = 0;
   514  
   515                  if (!i915_gem_object_is_readonly(obj))
   516                          flags |= FOLL_WRITE;
   517  
   518                  ret = -EFAULT;
                        ^^^^^^^^^^^^^

   519                  if (mmget_not_zero(mm)) {
   520                          down_read(&mm->mmap_sem);
   521                          while (pinned < npages) {
   522                                  ret = get_user_pages_remote
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
ret can be negative or zero or positive.

   523                                          (work->task, mm,
   524                                           obj->userptr.ptr + pinned * PAGE_SIZE,
   525                                           npages - pinned,
   526                                           flags,
   527                                           pvec + pinned, NULL, NULL);
   528                                  if (ret < 0)
   529                                          break;
   530  
   531                                  pinned += ret;
   532                          }
   533                          up_read(&mm->mmap_sem);
   534                          mmput(mm);
   535                  }
   536          }
   537  
   538          mutex_lock(&obj->mm.lock);
   539          if (obj->userptr.work == &work->work) {
   540                  struct sg_table *pages = ERR_PTR(ret);
                                         ^^^^^^^^^^^^^^^^^^^^
It looks to me like the static checker could right that "ret" might not
be a negative error code.  Is that intentional?

   541  
   542                  if (pinned == npages) {
   543                          pages = __i915_gem_userptr_alloc_pages(obj, pvec,
   544                                                                 npages);
   545                          if (!IS_ERR(pages)) {
   546                                  pinned = 0;
   547                                  pages = NULL;
   548                          }
   549                  }
   550  
   551                  obj->userptr.work = ERR_CAST(pages);
   552                  if (IS_ERR(pages))
   553                          __i915_gem_userptr_set_active(obj, false);
   554          }
   555          mutex_unlock(&obj->mm.lock);
   556  
   557          release_pages(pvec, pinned);
   558          kvfree(pvec);
   559  
   560          i915_gem_object_put(obj);
   561          put_task_struct(work->task);
   562          kfree(work);
   563  }

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx