[1/4] drm: Complain if drivers still use the ->load callback
diff mbox series

Message ID 20200128104602.1459802-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/4] drm: Complain if drivers still use the ->load callback
Related show

Commit Message

Daniel Vetter Jan. 28, 2020, 10:45 a.m. UTC
Kinda time to get this sorted. The locking around this really is not
nice.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 6 ++++++
 include/drm/drm_drv.h     | 3 +++
 2 files changed, 9 insertions(+)

Comments

Chris Wilson Jan. 28, 2020, 10:47 a.m. UTC | #1
Quoting Daniel Vetter (2020-01-28 10:45:58)
> Kinda time to get this sorted. The locking around this really is not
> nice.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>  include/drm/drm_drv.h     | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7c18a980cd4b..8deff75b484c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  
>         mutex_lock(&drm_global_mutex);
>  
> +       if (dev->driver->load) {
> +               if (!drm_core_check_feature(dev, DRIVER_LEGACY))
> +                       DRM_INFO("drm driver %s is using deprecated ->load callback\n",
> +                                dev->driver->name);

DRM_WARN() if the plan is to remove it?

That should encourage people to complain louder.
-Chris
Chris Wilson Jan. 28, 2020, 11:01 a.m. UTC | #2
Quoting Chris Wilson (2020-01-28 10:47:59)
> Quoting Daniel Vetter (2020-01-28 10:45:58)
> > Kinda time to get this sorted. The locking around this really is not
> > nice.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 6 ++++++
> >  include/drm/drm_drv.h     | 3 +++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 7c18a980cd4b..8deff75b484c 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> >  
> >         mutex_lock(&drm_global_mutex);
> >  
> > +       if (dev->driver->load) {
> > +               if (!drm_core_check_feature(dev, DRIVER_LEGACY))
> > +                       DRM_INFO("drm driver %s is using deprecated ->load callback\n",
> > +                                dev->driver->name);
> 
> DRM_WARN() if the plan is to remove it?

Either way though,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Thomas Zimmermann Jan. 28, 2020, 1:41 p.m. UTC | #3
Hi

Am 28.01.20 um 11:45 schrieb Daniel Vetter:
> Kinda time to get this sorted. The locking around this really is not
> nice.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>  include/drm/drm_drv.h     | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7c18a980cd4b..8deff75b484c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  
>  	mutex_lock(&drm_global_mutex);
>  
> +	if (dev->driver->load) {
> +		if (!drm_core_check_feature(dev, DRIVER_LEGACY))
> +			DRM_INFO("drm driver %s is using deprecated ->load callback\n",
> +				 dev->driver->name);
> +	}
> +
>  	ret = drm_minor_register(dev, DRM_MINOR_RENDER);
>  	if (ret)
>  		goto err_minors;
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 77685ed7aa65..77bc63de0a91 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -173,6 +173,9 @@ struct drm_driver {
>  	 *
>  	 * This is deprecated, do not use!
>  	 *
> +	 * FIXME: A few non-DRIVER_LEGACY drivers still use this, and should be
> +	 * converted.
> +	 *

I recently converted several of them. The status here is that only
radeon and amdgpu still use load. I've only not been able to convert
them because they do some debugfs registering that relies on the device
being registered early. I've not had time to convert the code.

On the patch:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>


>  	 * Returns:
>  	 *
>  	 * Zero on success, non-zero value on failure.
>
Daniel Vetter Jan. 28, 2020, 2:53 p.m. UTC | #4
On Tue, Jan 28, 2020 at 02:41:06PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.01.20 um 11:45 schrieb Daniel Vetter:
> > Kinda time to get this sorted. The locking around this really is not
> > nice.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 6 ++++++
> >  include/drm/drm_drv.h     | 3 +++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 7c18a980cd4b..8deff75b484c 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> >  
> >  	mutex_lock(&drm_global_mutex);
> >  
> > +	if (dev->driver->load) {
> > +		if (!drm_core_check_feature(dev, DRIVER_LEGACY))
> > +			DRM_INFO("drm driver %s is using deprecated ->load callback\n",
> > +				 dev->driver->name);
> > +	}
> > +
> >  	ret = drm_minor_register(dev, DRM_MINOR_RENDER);
> >  	if (ret)
> >  		goto err_minors;
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index 77685ed7aa65..77bc63de0a91 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -173,6 +173,9 @@ struct drm_driver {
> >  	 *
> >  	 * This is deprecated, do not use!
> >  	 *
> > +	 * FIXME: A few non-DRIVER_LEGACY drivers still use this, and should be
> > +	 * converted.
> > +	 *
> 
> I recently converted several of them. The status here is that only
> radeon and amdgpu still use load. I've only not been able to convert
> them because they do some debugfs registering that relies on the device
> being registered early. I've not had time to convert the code.

Oh nice, this sounds great. We have an outreachy project to helpt simplify
the debugfs stuff, so might be worth waiting for that. I guess I'll keep
this patch meanwhile (expect if Alex or Harry ack it, they'll see the
fallout if we merge this early).
-Daniel


> On the patch:
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> 
> >  	 * Returns:
> >  	 *
> >  	 * Zero on success, non-zero value on failure.
> > 
> 
> -- 
> 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 Jan. 28, 2020, 3:02 p.m. UTC | #5
On Tue, Jan 28, 2020 at 10:47:59AM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2020-01-28 10:45:58)
> > Kinda time to get this sorted. The locking around this really is not
> > nice.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 6 ++++++
> >  include/drm/drm_drv.h     | 3 +++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 7c18a980cd4b..8deff75b484c 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> >  
> >         mutex_lock(&drm_global_mutex);
> >  
> > +       if (dev->driver->load) {
> > +               if (!drm_core_check_feature(dev, DRIVER_LEGACY))
> > +                       DRM_INFO("drm driver %s is using deprecated ->load callback\n",
> > +                                dev->driver->name);
> 
> DRM_WARN() if the plan is to remove it?

Consensus from the security check work that Kees Cook is doing seems to
be:
- Put new thing in place, convert lots
- Start to do opt-in/informational stuff
- Once you're sure it's all gone, put in the big splat that kills the
  box/driver. Apparently radeon/amdgpu are the hold-outs, once those are
  done I think I'll just outright disable ->load/unload for
  !DRIVER_LEGACY.

Cheers, Daniel

> That should encourage people to complain louder.
> -Chris
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7c18a980cd4b..8deff75b484c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -948,6 +948,12 @@  int drm_dev_register(struct drm_device *dev, unsigned long flags)
 
 	mutex_lock(&drm_global_mutex);
 
+	if (dev->driver->load) {
+		if (!drm_core_check_feature(dev, DRIVER_LEGACY))
+			DRM_INFO("drm driver %s is using deprecated ->load callback\n",
+				 dev->driver->name);
+	}
+
 	ret = drm_minor_register(dev, DRM_MINOR_RENDER);
 	if (ret)
 		goto err_minors;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 77685ed7aa65..77bc63de0a91 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -173,6 +173,9 @@  struct drm_driver {
 	 *
 	 * This is deprecated, do not use!
 	 *
+	 * FIXME: A few non-DRIVER_LEGACY drivers still use this, and should be
+	 * converted.
+	 *
 	 * Returns:
 	 *
 	 * Zero on success, non-zero value on failure.