diff mbox

drm: Add deprecation warnings to the old midlayer callbacks

Message ID 20170601130026.15964-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson June 1, 2017, 1 p.m. UTC 
Daniel started a crusade a few years back to move control over the
initialisation and teardown into the driver rather than drm core, for
greater control and far fewer surprises. Help in that fight by adding
compiler warnings to the stale functions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

This is not as useful as I hoped. I wanted for gcc to emit a
compile-time warning when a deprecated field was assigned, however it
only emits when it is used. So we end up with a lot of noise in drm_core
and none in the drivers. However, the runtime warning is probably useful
and a motivator for change?

Should be made WARN_ONCE though

---
 drivers/gpu/drm/Kconfig      | 11 +++++++++++
 drivers/gpu/drm/drm_drv.c    |  4 ++--
 drivers/gpu/drm/drm_gem.c    |  2 +-
 include/drm/drm_deprecated.h | 35 +++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h        |  4 ++++
 5 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 include/drm/drm_deprecated.h

Comments

Emil Velikov June 1, 2017, 6:25 p.m. UTC | #1 
On 1 June 2017 at 14:00, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Daniel started a crusade a few years back to move control over the
> initialisation and teardown into the driver rather than drm core, for
> greater control and far fewer surprises. Help in that fight by adding
> compiler warnings to the stale functions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>
> This is not as useful as I hoped. I wanted for gcc to emit a
> compile-time warning when a deprecated field was assigned, however it
> only emits when it is used. So we end up with a lot of noise in drm_core
> and none in the drivers. However, the runtime warning is probably useful
> and a motivator for change?
>
> Should be made WARN_ONCE though
>
I recall suggesting something similar a while back, but Daniel was not
too happy with it.
Hope it goes well this time.

On the assignment issue - if you declare a __deprecated type you will
get the warning [1]
A couple of small questions:
 - Why the separate configure toggle?
 - Most other places use type __deprecated function() and type
variable __deprecated;
Worth doing the same here - is it just sugar or there's something to it?

-Emil

[1] Small example

$cat dep.c
#define __deprecated __attribute__((deprecated))
typedef int FANCY __deprecated;

struct foo {
    FANCY i __deprecated;
} f1 = {
    .i = 100,
};

int
main(void)
{
    f1.i = 2;
    return 0;
}

$gcc -Wall -Wextra dep.c
dep.c:5:5: warning: ‘FANCY’ is deprecated [-Wdeprecated-declarations]
    FANCY i __deprecated;
    ^~~~~
dep.c: In function ‘main’:
dep.c:13:5: warning: ‘i’ is deprecated [-Wdeprecated-declarations]
    f1.i = 2;
    ^~
dep.c:5:11: note: declared here
    FANCY i __deprecated;
          ^
kernel test robot June 1, 2017, 11:04 p.m. UTC | #2 
Hi Chris,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.12-rc3 next-20170601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-Add-deprecation-warnings-to-the-old-midlayer-callbacks/20170602-003053
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_gem.c: In function 'drm_gem_object_free':
>> drivers/gpu/drm/drm_gem.c:803:2: warning: 'gem_free_object' is deprecated (declared at include/drm/drm_drv.h:377) [-Wdeprecated-declarations]
   drivers/gpu/drm/drm_gem.c:806:3: warning: 'gem_free_object' is deprecated (declared at include/drm/drm_drv.h:377) [-Wdeprecated-declarations]
--
   drivers/gpu/drm/drm_drv.c: In function 'drm_dev_register':
>> drivers/gpu/drm/drm_drv.c:781:2: warning: 'load' is deprecated (declared at include/drm/drm_drv.h:86) [-Wdeprecated-declarations]
   drivers/gpu/drm/drm_drv.c:782:3: warning: 'load' is deprecated (declared at include/drm/drm_drv.h:86) [-Wdeprecated-declarations]
   drivers/gpu/drm/drm_drv.c: In function 'drm_dev_unregister':
>> drivers/gpu/drm/drm_drv.c:833:2: warning: 'unload' is deprecated (declared at include/drm/drm_drv.h:166) [-Wdeprecated-declarations]
   drivers/gpu/drm/drm_drv.c:834:3: warning: 'unload' is deprecated (declared at include/drm/drm_drv.h:166) [-Wdeprecated-declarations]

vim +/gem_free_object +803 drivers/gpu/drm/drm_gem.c

   787	 * @kref: kref of the object to free
   788	 *
   789	 * Called after the last reference to the object has been lost.
   790	 * Must be called holding &drm_device.struct_mutex.
   791	 *
   792	 * Frees the object
   793	 */
   794	void
   795	drm_gem_object_free(struct kref *kref)
   796	{
   797		struct drm_gem_object *obj =
   798			container_of(kref, struct drm_gem_object, refcount);
   799		struct drm_device *dev = obj->dev;
   800	
   801		if (dev->driver->gem_free_object_unlocked) {
   802			dev->driver->gem_free_object_unlocked(obj);
 > 803		} else if (DRM_DEPRECATED_WARN(dev->driver->gem_free_object)) {
   804			WARN_ON(!mutex_is_locked(&dev->struct_mutex));
   805	
   806			dev->driver->gem_free_object(obj);
   807		}
   808	}
   809	EXPORT_SYMBOL(drm_gem_object_free);
   810	
   811	/**

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot June 1, 2017, 11:06 p.m. UTC | #3 
Hi Chris,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.12-rc3 next-20170601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-Add-deprecation-warnings-to-the-old-midlayer-callbacks/20170602-003053
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_gem.c: In function 'drm_gem_object_free':
>> drivers/gpu/drm/drm_gem.c:803:2: warning: 'gem_free_object' is deprecated [-Wdeprecated-declarations]
     } else if (DRM_DEPRECATED_WARN(dev->driver->gem_free_object)) {
     ^
   In file included from include/drm/drmP.h:76:0,
                    from drivers/gpu/drm/drm_gem.c:39:
   include/drm/drm_drv.h:377:9: note: declared here
     void (*gem_free_object) (struct drm_gem_object *obj);
            ^~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_gem.c:806:3: warning: 'gem_free_object' is deprecated [-Wdeprecated-declarations]
      dev->driver->gem_free_object(obj);
      ^~~
   In file included from include/drm/drmP.h:76:0,
                    from drivers/gpu/drm/drm_gem.c:39:
   include/drm/drm_drv.h:377:9: note: declared here
     void (*gem_free_object) (struct drm_gem_object *obj);
            ^~~~~~~~~~~~~~~
--
   drivers/gpu/drm/drm_drv.c: In function 'drm_dev_register':
>> drivers/gpu/drm/drm_drv.c:781:2: warning: 'load' is deprecated [-Wdeprecated-declarations]
     if (DRM_DEPRECATED_WARN(dev->driver->load)) {
     ^~
   In file included from drivers/gpu/drm/drm_drv.c:36:0:
   include/drm/drm_drv.h:86:8: note: declared here
     int (*load) (struct drm_device *, unsigned long flags);
           ^~~~
   drivers/gpu/drm/drm_drv.c:782:3: warning: 'load' is deprecated [-Wdeprecated-declarations]
      ret = dev->driver->load(dev, flags);
      ^~~
   In file included from drivers/gpu/drm/drm_drv.c:36:0:
   include/drm/drm_drv.h:86:8: note: declared here
     int (*load) (struct drm_device *, unsigned long flags);
           ^~~~
   drivers/gpu/drm/drm_drv.c: In function 'drm_dev_unregister':
>> drivers/gpu/drm/drm_drv.c:833:2: warning: 'unload' is deprecated [-Wdeprecated-declarations]
     if (DRM_DEPRECATED_WARN(dev->driver->unload))
     ^~
   In file included from drivers/gpu/drm/drm_drv.c:36:0:
   include/drm/drm_drv.h:166:9: note: declared here
     void (*unload) (struct drm_device *);
            ^~~~~~
   drivers/gpu/drm/drm_drv.c:834:3: warning: 'unload' is deprecated [-Wdeprecated-declarations]
      dev->driver->unload(dev);
      ^~~
   In file included from drivers/gpu/drm/drm_drv.c:36:0:
   include/drm/drm_drv.h:166:9: note: declared here
     void (*unload) (struct drm_device *);
            ^~~~~~

vim +/gem_free_object +803 drivers/gpu/drm/drm_gem.c

   787	 * @kref: kref of the object to free
   788	 *
   789	 * Called after the last reference to the object has been lost.
   790	 * Must be called holding &drm_device.struct_mutex.
   791	 *
   792	 * Frees the object
   793	 */
   794	void
   795	drm_gem_object_free(struct kref *kref)
   796	{
   797		struct drm_gem_object *obj =
   798			container_of(kref, struct drm_gem_object, refcount);
   799		struct drm_device *dev = obj->dev;
   800	
   801		if (dev->driver->gem_free_object_unlocked) {
   802			dev->driver->gem_free_object_unlocked(obj);
 > 803		} else if (DRM_DEPRECATED_WARN(dev->driver->gem_free_object)) {
   804			WARN_ON(!mutex_is_locked(&dev->struct_mutex));
   805	
   806			dev->driver->gem_free_object(obj);
   807		}
   808	}
   809	EXPORT_SYMBOL(drm_gem_object_free);
   810	
   811	/**

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Daniel Vetter June 20, 2017, 9 a.m. UTC | #4 
On Thu, Jun 01, 2017 at 07:25:46PM +0100, Emil Velikov wrote:
> On 1 June 2017 at 14:00, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Daniel started a crusade a few years back to move control over the
> > initialisation and teardown into the driver rather than drm core, for
> > greater control and far fewer surprises. Help in that fight by adding
> > compiler warnings to the stale functions.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >
> > This is not as useful as I hoped. I wanted for gcc to emit a
> > compile-time warning when a deprecated field was assigned, however it
> > only emits when it is used. So we end up with a lot of noise in drm_core
> > and none in the drivers. However, the runtime warning is probably useful
> > and a motivator for change?
> >
> > Should be made WARN_ONCE though
> >
> I recall suggesting something similar a while back, but Daniel was not
> too happy with it.
> Hope it goes well this time.

The problem is that if we enable it by default people scream regressions!
If we enable it behind a Kconfig, Linus screams Kconfig! And no one else
will look at it.

I'm half-tempted to add cocci-check support to dim to catch these, but
that's also an imcomplete solution.

I'd love to enforce this with tooling though, one way or another, I just
haven't seen something that works well yet :-/
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 83cb2a88c204..530d56a2fa01 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -34,6 +34,17 @@  config DRM_DP_AUX_CHARDEV
 	  read and write values to arbitrary DPCD registers on the DP aux
 	  channel.
 
+config DRM_DEBUG_DEPRECATED
+	bool "Insert extra warnings if deprecated code is used"
+	default n
+	depends on DRM
+	help
+	  Enables extra warnings if deprecated code and callbacks are used.
+
+	  Recommended for driver developers only.
+
+	  If in doubt, say "N".
+
 config DRM_DEBUG_MM
 	bool "Insert extra checks and debug info into the DRM range managers"
 	default n
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb46a425..63f968ec1f05 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -778,7 +778,7 @@  int drm_dev_register(struct drm_device *dev, unsigned long flags)
 
 	dev->registered = true;
 
-	if (dev->driver->load) {
+	if (DRM_DEPRECATED_WARN(dev->driver->load)) {
 		ret = dev->driver->load(dev, flags);
 		if (ret)
 			goto err_minors;
@@ -830,7 +830,7 @@  void drm_dev_unregister(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_unregister_all(dev);
 
-	if (dev->driver->unload)
+	if (DRM_DEPRECATED_WARN(dev->driver->unload))
 		dev->driver->unload(dev);
 
 	if (dev->agp)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 8dc11064253d..c0998a9b1ba0 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -800,7 +800,7 @@  drm_gem_object_free(struct kref *kref)
 
 	if (dev->driver->gem_free_object_unlocked) {
 		dev->driver->gem_free_object_unlocked(obj);
-	} else if (dev->driver->gem_free_object) {
+	} else if (DRM_DEPRECATED_WARN(dev->driver->gem_free_object)) {
 		WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 		dev->driver->gem_free_object(obj);
diff --git a/include/drm/drm_deprecated.h b/include/drm/drm_deprecated.h
new file mode 100644
index 000000000000..e0debe496f28
--- /dev/null
+++ b/include/drm/drm_deprecated.h
@@ -0,0 +1,35 @@ 
+/*
+ * Copyright 2017 Intel Corp.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, 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.
+ */
+
+#ifndef _DRM_DEPRECATED_H_
+#define _DRM_DEPRECATED_H_
+
+#ifdef CONFIG_DRM_DEBUG_DEPRECATED
+#define drm_deprecated __deprecated
+#define DRM_DEPRECATED_WARN(cond) WARN(cond, "Use of %s is deprecated\n", #cond)
+#else
+#define drm_deprecated
+#define DRM_DEPRECATED_WARN(cond) (cond)
+#endif
+
+#endif
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 18f3181674e8..a2cb895cdfe3 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -29,6 +29,7 @@ 
 
 #include <linux/list.h>
 #include <linux/irqreturn.h>
+#include <drm/drm_deprecated.h>
 
 struct drm_device;
 struct drm_file;
@@ -81,6 +82,7 @@  struct drm_driver {
 	 *
 	 * Zero on success, non-zero value on failure.
 	 */
+	drm_deprecated
 	int (*load) (struct drm_device *, unsigned long flags);
 
 	/**
@@ -160,6 +162,7 @@  struct drm_driver {
 	 * the device.
 	 *
 	 */
+	drm_deprecated
 	void (*unload) (struct drm_device *);
 
 	/**
@@ -399,6 +402,7 @@  struct drm_driver {
 	 * This is deprecated and should not be used by new drivers. Use
 	 * @gem_free_object_unlocked instead.
 	 */
+	drm_deprecated
 	void (*gem_free_object) (struct drm_gem_object *obj);
 
 	/**