[PATCH] drm/irq: Modify the return value type of drm_irq_uninstall

[PATCH] drm/irq: Modify the return value type of drm_irq_uninstall

[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 = dev->irq_enabled;
 	dev->irq_enabled = 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=%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 = drm_irq_uninstall(dev);
+		drm_irq_uninstall(dev);
 		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
-- 
2.7.4

Re: [PATCH] drm/irq: Modify the return value type of drm_irq_uninstall

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2817 bytes --]

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 = dev->irq_enabled;
>  	dev->irq_enabled = 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=%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 = 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.

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ürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] drm/irq: Modify the return value type of drm_irq_uninstall

[Daniel Vetter]

On Mon, Nov 2, 2020 at 1:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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 = dev->irq_enabled;
> >       dev->irq_enabled = 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=%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 = 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ürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer



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