Re: [PATCH] drm/i915: Clean up ancient doc comments for i915_ioc32.c

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

On Wed, 14 Feb 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-02-14 15:42:37)
>> On Wed, 14 Feb 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > As befitting a file dedicated to the mistakes of the past,
>> >
>> > drivers/gpu/drm/i915/i915_ioc32.c:2: warning: Cannot understand  * \file i915_ioc32.c
>> >  on line 2 - I thought it was a doc line
>> > drivers/gpu/drm/i915/i915_ioc32.c:82: warning: Function parameter or member 'filp' not described in 'i915_compat_ioctl'
>> > drivers/gpu/drm/i915/i915_ioc32.c:82: warning: Function parameter or member 'cmd' not described in 'i915_compat_ioctl'
>> > drivers/gpu/drm/i915/i915_ioc32.c:82: warning: Function parameter or member 'arg' not described in 'i915_compat_ioctl'
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/i915_ioc32.c | 38 ++++++++++++++++----------------------
>> >  1 file changed, 16 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c
>> > index 97f3a5640289..73599be641e7 100644
>> > --- a/drivers/gpu/drm/i915/i915_ioc32.c
>> > +++ b/drivers/gpu/drm/i915/i915_ioc32.c
>> > @@ -1,11 +1,6 @@
>> > -/**
>> > - * \file i915_ioc32.c
>> > - *
>> > +/*
>> >   * 32-bit ioctl compatibility routines for the i915 DRM.
>> >   *
>> > - * \author Alan Hourihane <alanh@fairlite.demon.co.uk>
>> > - *
>> > - *
>> >   * Copyright (C) Paul Mackerras 2005
>> >   * Copyright (C) Alan Hourihane 2005
>> >   * All Rights Reserved.
>> > @@ -28,6 +23,8 @@
>> >   * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> >   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> >   * IN THE SOFTWARE.
>> > + *
>> > + * Author: Alan Hourihane <alanh@fairlite.demon.co.uk>
>> >   */
>> >  #include <linux/compat.h>
>> >  
>> > @@ -55,9 +52,9 @@ static int compat_i915_getparam(struct file *file, unsigned int cmd,
>> >               return -EFAULT;
>> >  
>> >       request = compat_alloc_user_space(sizeof(*request));
>> > -     if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
>> > -         || __put_user(req32.param, &request->param)
>> > -         || __put_user((void __user *)(unsigned long)req32.value,
>> > +     if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) ||
>> > +         __put_user(req32.param, &request->param) ||
>> > +         __put_user((void __user *)(unsigned long)req32.value,
>> >                         &request->value))
>> >               return -EFAULT;
>> >  
>> > @@ -70,30 +67,27 @@ static drm_ioctl_compat_t *i915_compat_ioctls[] = {
>> >  };
>> >  
>> >  /**
>> > + * i915_compat_ioctl - handle the mistakes of the past
>> > + * @filp: the file pointer
>> > + * @cmd: the ioctl command (and encoded flags)
>> > + * @arg: the ioctl argument (from userspace)
>> > + *
>> >   * Called whenever a 32-bit process running under a 64-bit kernel
>> >   * performs an ioctl on /dev/dri/card<n>.
>> > - *
>> > - * \param filp file pointer.
>> > - * \param cmd command.
>> > - * \param arg user argument.
>> > - * \return zero on success or negative number on failure.
>> >   */
>> >  long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> >  {
>> >       unsigned int nr = DRM_IOCTL_NR(cmd);
>> > -     drm_ioctl_compat_t *fn = NULL;
>> > -     int ret;
>> > +     drm_ioctl_compat_t *fn;
>> >  
>> >       if (nr < DRM_COMMAND_BASE || nr >= DRM_COMMAND_END)
>> >               return drm_compat_ioctl(filp, cmd, arg);
>> >  
>> > +     fn = NULL;
>> >       if (nr < DRM_COMMAND_BASE + ARRAY_SIZE(i915_compat_ioctls))
>> >               fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];
>> > +     if (fn)
>> > +             return fn(filp, cmd, arg);
>> 
>> Not really fond of code juggling in a patch that's supposed to be about
>> doc comment updates.
>> 
>> But if you do this, you might just as well move the if (fn) check within
>> the preceding if block, and get rid of the fn = NULL initialization
>> altogheter.
>
> In the end I want it to become something like

So how about not juggling it in the patch at hand, and doing it properly
in a separate patch instead...? I kind of dislike what drive-by cleanups
do to git blame.

BR,
Jani.

>
>         fn = NULL;
>         if (nr < ARRAY_SIZE(i915_compat_ioctls))
>                 fn = i915_compat_ioctls[nr];
>         if (!fn) {
>                 const struct drm_ioctl_desc *ioctl;
>
>                 ioctl = &i915_ioctls[nr];
>                 if (ioctl->flags & DRM_DRIVER_IOCTL)
>                         fn = ioctl->ioctl;
>                 else
>                         fn = drm_ioctl;
>         }
>
>         return fn(filp, cmd, arg);

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