Re: [PATCH v4] gpu: drm: i915: Change return type to vm_fault_t

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 31 May 2018, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> On Mon, May 21, 2018 at 4:48 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
>> On Thu, May 17, 2018 at 10:40 AM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
>>> On Thu, May 17, 2018 at 12:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>> Quoting Souptick Joarder (2018-05-16 20:12:20)
>>>>> Use new return type vm_fault_t for fault handler. For
>>>>> now, this is just documenting that the function returns
>>>>> a VM_FAULT value rather than an errno. Once all instances
>>>>> are converted, vm_fault_t will become a distinct type.
>>>>>
>>>>> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>>>>>
>>>>> Fixed one checkpatch.pl warning inside WARN_ONCE.
>>>>>
>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>>>> ---
>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>>> index dd89abd..732abdf 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>>>>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>>>>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see i915_gem_mmap_gtt_version).
>>>>>   */
>>>>> -int i915_gem_fault(struct vm_fault *vmf)
>>>>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>>>>  {
>>>>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>>>>>         struct vm_area_struct *area = vmf->vma;
>>>>> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
>>>>>         struct i915_vma *vma;
>>>>>         pgoff_t page_offset;
>>>>>         unsigned int flags;
>>>>> -       int ret;
>>>>> +       int err;
>>>>> +       vm_fault_t ret = VM_FAULT_SIGBUS;
>>>>>
>>>>>         /* We don't use vmf->pgoff since that has the fake offset */
>>>>>         page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>>>>> @@ -2027,8 +2028,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>>>>>                 ret = VM_FAULT_SIGBUS;
>>>>>                 break;
>>>>>         default:
>>>>> -               WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", ret);
>>>>> -               ret = VM_FAULT_SIGBUS;
>>>>> +               WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, err);
>>>>>                 break;
>>>>
>>>> Even simpler would be to use e.g. return VM_FAULT_SIGBUS for each case
>>>> above. No early initialisation of use-once variables allowing the
>>>> compiler to do it's job. For a smaller patch, you can even skip the
>>>> s/ret/err/
>>>> -Chris
>>>
>>> Chris,
>>> I prefer to use return once at the end of the function rather than
>>> writing multiple return statement (Current code is doing similar).
>>> But if you think other way, I can make that change :)
>>
>> If no further comment, we would like to get this patch
>> in queue for 4.18

For gpu drivers, that ship had sailed before you sent the patch. It'll
be v4.19.

I'll let Chris comment if changes are needed or not.

BR,
Jani.



>
> We need to get this patch in queue for 4.18.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx