[PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Daniel Vetter]

Oops, makes testing early boot failures in i915.ko a bit more pain, so
let's fix it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c2e00ed23195..d9e5c6fc52ea 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -982,6 +982,8 @@ static struct pci_driver i915_pci_driver = {
 	.driver.pm = &i915_pm_ops,
 };
 
+bool i915_no_driver_loaded;
+
 static int __init i915_init(void)
 {
 	driver.num_ioctls = i915_max_ioctl;
@@ -1011,6 +1013,7 @@ static int __init i915_init(void)
 		driver.get_vblank_timestamp = NULL;
 #ifndef CONFIG_DRM_I915_UMS
 		/* Silently fail loading to not upset userspace. */
+		i915_no_driver_loaded = true;
 		return 0;
 #endif
 	}
@@ -1020,6 +1023,9 @@ static int __init i915_init(void)
 
 static void __exit i915_exit(void)
 {
+	if (i915_no_driver_loaded)
+		return;
+
 	drm_pci_exit(&driver, &i915_pci_driver);
 }
 
-- 
1.8.3.1

Re: [PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Chris Wilson]

On Fri, Nov 15, 2013 at 11:28:39AM +0100, Daniel Vetter wrote:
> Oops, makes testing early boot failures in i915.ko a bit more pain, so
> let's fix it.

You already have a static driver structure, why not take advantage of
that here?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Daniel Vetter]

Oops, makes testing early boot failures in i915.ko a bit more pain, so
let's fix it.

v2: We already have a bit of static storage to track this (Chris).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c2e00ed23195..72fc9e33c78c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1020,6 +1020,11 @@ static int __init i915_init(void)
 
 static void __exit i915_exit(void)
 {
+#ifndef CONFIG_DRM_I915_UMS
+	if (!(driver.driver_features & DRIVER_MODESET))
+		return; /* Never loaded a driver. */
+#endif
+
 	drm_pci_exit(&driver, &i915_pci_driver);
 }
 
-- 
1.8.3.1

Re: [PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Chris Wilson]

On Fri, Nov 15, 2013 at 05:16:33PM +0100, Daniel Vetter wrote:
> Oops, makes testing early boot failures in i915.ko a bit more pain, so
> let's fix it.
> 
> v2: We already have a bit of static storage to track this (Chris).
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

How about just fixing the core not to die?

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index f00d7a9..3b9c7ce 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -375,13 +375,13 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
 
        DRM_DEBUG("\n");
 
+       if (driver->driver_features & DRIVER_MODESET)
+               return pci_register_driver(pdriver);
+
        INIT_LIST_HEAD(&driver->device_list);
        driver->kdriver.pci = pdriver;
        driver->bus = &drm_pci_bus;
 
-       if (driver->driver_features & DRIVER_MODESET)
-               return pci_register_driver(pdriver);
-
        /* If not using KMS, fall back to stealth mode manual scanning. */
        for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
                pid = &pdriver->id_table[i];
@@ -462,6 +462,11 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
        struct drm_device *dev, *tmp;
        DRM_DEBUG("\n");
 
+       if (!driver->kdriver.pci)
+               return;
+
+       WARN_ON(driver->kdriver.pci != pdriver);
+
        if (driver->driver_features & DRIVER_MODESET) {
                pci_unregister_driver(pdriver);
        } else {

We can drop the redundant pdriver later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Daniel Vetter]

On Fri, Nov 15, 2013 at 08:58:12PM +0000, Chris Wilson wrote:
> On Fri, Nov 15, 2013 at 05:16:33PM +0100, Daniel Vetter wrote:
> > Oops, makes testing early boot failures in i915.ko a bit more pain, so
> > let's fix it.
> > 
> > v2: We already have a bit of static storage to track this (Chris).
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> How about just fixing the core not to die?

This is for the disabled UMS code where we silently don't register
anything. So there's also nothing to clean up, i.e. we've called
drm_pci_exit without drm_pci_init. I don't think this is something we
should allow in the core.
-Daniel
> 
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index f00d7a9..3b9c7ce 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -375,13 +375,13 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
>  
>         DRM_DEBUG("\n");
>  
> +       if (driver->driver_features & DRIVER_MODESET)
> +               return pci_register_driver(pdriver);
> +
>         INIT_LIST_HEAD(&driver->device_list);
>         driver->kdriver.pci = pdriver;
>         driver->bus = &drm_pci_bus;
>  
> -       if (driver->driver_features & DRIVER_MODESET)
> -               return pci_register_driver(pdriver);
> -
>         /* If not using KMS, fall back to stealth mode manual scanning. */
>         for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
>                 pid = &pdriver->id_table[i];
> @@ -462,6 +462,11 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
>         struct drm_device *dev, *tmp;
>         DRM_DEBUG("\n");
>  
> +       if (!driver->kdriver.pci)
> +               return;
> +
> +       WARN_ON(driver->kdriver.pci != pdriver);
> +
>         if (driver->driver_features & DRIVER_MODESET) {
>                 pci_unregister_driver(pdriver);
>         } else {
> 
> We can drop the redundant pdriver later.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Paulo Zanoni]

2013/11/15 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Oops, makes testing early boot failures in i915.ko a bit more pain, so
> let's fix it.
>
> v2: We already have a bit of static storage to track this (Chris).
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I can reproduce the failure by booting with i915.modeset=0, then get
the oops when "rmmod i915".

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I also looked at Chris' patch. I think that since we take the early
"return 0" inside i915_init, we never call drm_pci_init, so it doesn't
make too much sense to fix the problem inside drm_pci_exit.

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c2e00ed23195..72fc9e33c78c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1020,6 +1020,11 @@ static int __init i915_init(void)
>
>  static void __exit i915_exit(void)
>  {
> +#ifndef CONFIG_DRM_I915_UMS
> +       if (!(driver.driver_features & DRIVER_MODESET))
> +               return; /* Never loaded a driver. */
> +#endif
> +
>         drm_pci_exit(&driver, &i915_pci_driver);
>  }
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Fix module unloading with DRM_I915_UMS=n

[Daniel Vetter]

On Fri, Nov 22, 2013 at 05:25:41PM -0200, Paulo Zanoni wrote:
> 2013/11/15 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Oops, makes testing early boot failures in i915.ko a bit more pain, so
> > let's fix it.
> >
> > v2: We already have a bit of static storage to track this (Chris).
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I can reproduce the failure by booting with i915.modeset=0, then get
> the oops when "rmmod i915".
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I also looked at Chris' patch. I think that since we take the early
> "return 0" inside i915_init, we never call drm_pci_init, so it doesn't
> make too much sense to fix the problem inside drm_pci_exit.

Thanks for review&testing, patch merged.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch