Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1604320685-14995-1-git-send-email-tiantao6@hisilicon.com> <7a06d5ba-27c6-0762-662c-fee1f8ddbc2e@suse.de>
In-Reply-To: <7a06d5ba-27c6-0762-662c-fee1f8ddbc2e@suse.de>
From:   Daniel Vetter <daniel@ffwll.ch>
Date:   Mon, 2 Nov 2020 14:05:56 +0100
Message-ID: <CAKMK7uF2+D0ZxwnxuQ9NOhSH5g96zyCiEw4Nc-4j54eTf+9wKg@mail.gmail.com>
Subject: Re: [PATCH] drm/irq: Modify the return value type of drm_irq_uninstall
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Tian Tao <tiantao6@hisilicon.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Dave Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Nov 2, 2020 at 1:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrot=
e:
>
> Hi
>
> Am 02.11.20 um 13:38 schrieb Tian Tao:
> > There is no driver to use the return value of drm_irq_uninstal,
> > so modify the return value type of drm_irq_uninstal to void.
> >
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c | 13 ++++++-------
> >  include/drm/drm_irq.h     |  2 +-
> >  2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 7537a3d..45e6471 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -166,14 +166,14 @@ EXPORT_SYMBOL(drm_irq_install);
> >   * Returns:
> >   * Zero on success or a negative error code on failure.
> >   */
> > -int drm_irq_uninstall(struct drm_device *dev)
> > +void drm_irq_uninstall(struct drm_device *dev)
> >  {
> >       unsigned long irqflags;
> >       bool irq_enabled;
> >       int i;
> >
> >       if (!dev->irq_enabled || !dev)
> > -             return 0;
> > +             return;
> >
> >       irq_enabled =3D dev->irq_enabled;
> >       dev->irq_enabled =3D false;
> > @@ -200,8 +200,8 @@ int drm_irq_uninstall(struct drm_device *dev)
> >               spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> >       }
> >
> > -     if (!irq_enabled)
> > -             return -EINVAL;
> > +     if (!drm_WARN_ON(dev, !irq_enabled))
> > +             return;
> >
> >       DRM_DEBUG("irq=3D%d\n", dev->irq);
> >
> > @@ -213,7 +213,6 @@ int drm_irq_uninstall(struct drm_device *dev)
> >
> >       free_irq(dev->irq, dev);
> >
> > -     return 0;
> >  }
> >  EXPORT_SYMBOL(drm_irq_uninstall);
> >
> > @@ -250,10 +249,10 @@ int drm_legacy_irq_control(struct drm_device *dev=
, void *data,
> >               return ret;
> >       case DRM_UNINST_HANDLER:
> >               mutex_lock(&dev->struct_mutex);
> > -             ret =3D drm_irq_uninstall(dev);
> > +             drm_irq_uninstall(dev);
>
> Oh, there actually is a user of this result! I grep'ed for this but
> didn't see it. I'm sorry for misleading you here.
>
> This is ioctl code and who which program depends on it.So we cannot
> actually drop the result code.
>
> I'll just ack your original patch, or you could add the managed
> interface that I described and convert hibmc to it. Your choice, let me
> know.

It's old UMS gunk, no one cares :-)

If you're paranoid, make an internal __drm_irq_uninstall function
which keeps the return value. But what we probably want here is to
just split this up into 2 functions, drm_legacy_irq_uninstall, which
does the validation and additional check, and then calls
drm_irq_uninstall, which is for kms drivers, and which just has a
WARN_ON(!dev->irq_installed) or so to catch driver bugs.
-Daniel

>
> Best regards
> Thomas
>
> >               mutex_unlock(&dev->struct_mutex);
> >
> > -             return ret;
> > +             return 0;
> >       default:
> >               return -EINVAL;
> >       }
> > diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> > index d77f6e6..d9f6ec0 100644
> > --- a/include/drm/drm_irq.h
> > +++ b/include/drm/drm_irq.h
> > @@ -27,6 +27,6 @@
> >  struct drm_device;
> >
> >  int drm_irq_install(struct drm_device *dev, int irq);
> > -int drm_irq_uninstall(struct drm_device *dev);
> > +void drm_irq_uninstall(struct drm_device *dev);
> >
> >  #endif
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany
> (HRB 36809, AG N=C3=BCrnberg)
> Gesch=C3=A4ftsf=C3=BChrer: Felix Imend=C3=B6rffer


--=20
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch