[PATCH 1/2] drm: call pci_disable_device on module onload

[PATCH 1/2] drm: call pci_disable_device on module onload

[Daniel Vetter]

Otherwise we'll nicely leak this reference counter. Now thanks to the
awesome layering in the drm core, the enable call is done by the pci
boilerplate in drm_pci.c. But the disable can't be done without adding
yet another neat indirection layer just for that.

So take the simple way and sprinkle pci_disable_device over all pci
modesetting drivers.

Also don't forget these dear old legacy drivers, prinkle the
pci_disable_device call in drm_pci_exit to cover these, too.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_drv.c         |    2 ++
 drivers/gpu/drm/cirrus/cirrus_drv.c   |    2 ++
 drivers/gpu/drm/drm_pci.c             |    4 +++-
 drivers/gpu/drm/gma500/psb_drv.c      |    3 +++
 drivers/gpu/drm/i915/i915_drv.c       |    2 ++
 drivers/gpu/drm/mgag200/mgag200_drv.c |    2 ++
 drivers/gpu/drm/nouveau/nouveau_drv.c |    2 ++
 drivers/gpu/drm/radeon/radeon_drv.c   |    2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   |    2 ++
 9 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index d0c4574..6d26e53 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -72,6 +72,8 @@ ast_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
index d703823..d316ba3 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.c
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
@@ -45,6 +45,8 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 59e11e4..73218ac 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -455,8 +455,10 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
 	if (driver->driver_features & DRIVER_MODESET) {
 		pci_unregister_driver(pdriver);
 	} else {
-		list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item)
+		list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) {
+			pci_disable_device(dev->pdev);
 			drm_put_dev(dev);
+		}
 	}
 	DRM_INFO("Module unloaded\n");
 }
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index caba6e0..9d6b0be 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -584,6 +584,9 @@ static void psb_driver_preclose(struct drm_device *dev, struct drm_file *priv)
 static void psb_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
+
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1bf7597..e4df5a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -938,6 +938,8 @@ i915_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 3c8e04f..9513eab 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -51,6 +51,8 @@ static void mga_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.c b/drivers/gpu/drm/nouveau/nouveau_drv.c
index b1dc91d..ed667bb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.c
@@ -167,6 +167,8 @@ nouveau_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 1cb0a02..a0d9f4f 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -304,6 +304,8 @@ radeon_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index ee24d21..3691ff2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -985,6 +985,8 @@ static void vmw_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	pci_disable_device(dev->pdev);
+
 	drm_put_dev(dev);
 }
 
-- 
1.7.7.6

[PATCH 2/2] drm: fixup error path after drm_fill_in_dev

[Daniel Vetter]

Within drm_fill_in_dev we call drm_lastclose to clean up the dirt in
case of failures. But the callers of drm_fill_in_dev simply don't do
anything. Now from a cursory look drm_lastclose doesn't look like the
best cleanup function in itself, but whom am I to judge the drm error
paths. Imo before we could tackle this we need to move more of the
legacy drm services setup and teardown into the respective drivers,
that way drm_lastclose would become more manageble.

Anyway, make things at least consistent by adding drm_lastclose at the
right places in the error paths for all callers of drm_fill_in_dev.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_pci.c      |    8 +++++---
 drivers/gpu/drm/drm_platform.c |    8 +++++---
 drivers/gpu/drm/drm_usb.c      |    8 +++++---
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 73218ac..40737a8 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -334,14 +334,14 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
 
 	if ((ret = drm_fill_in_dev(dev, ent, driver))) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g2;
+		goto err_pci;
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		pci_set_drvdata(pdev, dev);
 		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 		if (ret)
-			goto err_g2;
+			goto err_drm_fill_in;
 	}
 
 	if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
@@ -375,7 +375,9 @@ err_g4:
 err_g3:
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_put_minor(&dev->control);
-err_g2:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_pci:
 	pci_disable_device(pdev);
 err_g1:
 	kfree(dev);
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 82431dc..177893a 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -60,14 +60,14 @@ int drm_get_platform_dev(struct platform_device *platdev,
 
 	if (ret) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g1;
+		goto err_kfree;
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		dev_set_drvdata(&platdev->dev, dev);
 		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 		if (ret)
-			goto err_g1;
+			goto err_drm_fill_in;
 	}
 
 	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
@@ -103,7 +103,9 @@ err_g3:
 err_g2:
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_put_minor(&dev->control);
-err_g1:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_kfree:
 	kfree(dev);
 	mutex_unlock(&drm_global_mutex);
 	return ret;
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index 37c9a52..2e179f1 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -25,13 +25,13 @@ int drm_get_usb_dev(struct usb_interface *interface,
 	ret = drm_fill_in_dev(dev, NULL, driver);
 	if (ret) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g1;
+		goto err_drm_fill_in;
 	}
 
 	usb_set_intfdata(interface, dev);
 	ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 	if (ret)
-		goto err_g1;
+		goto err_kfree;
 
 	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
 	if (ret)
@@ -63,7 +63,9 @@ err_g3:
 	drm_put_minor(&dev->primary);
 err_g2:
 	drm_put_minor(&dev->control);
-err_g1:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_kfree:
 	kfree(dev);
 	mutex_unlock(&drm_global_mutex);
 	return ret;
-- 
1.7.7.6

Re: [PATCH 1/2] drm: call pci_disable_device on module onload

[Jakob Bornecrantz]

On Fri, Jun 8, 2012 at 4:52 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Otherwise we'll nicely leak this reference counter. Now thanks to the
> awesome layering in the drm core, the enable call is done by the pci
> boilerplate in drm_pci.c. But the disable can't be done without adding
> yet another neat indirection layer just for that.
>
> So take the simple way and sprinkle pci_disable_device over all pci
> modesetting drivers.
>
> Also don't forget these dear old legacy drivers, prinkle the
> pci_disable_device call in drm_pci_exit to cover these, too.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Looks good, one question inlined.

CC: Stable?

> ---
>  drivers/gpu/drm/ast/ast_drv.c         |    2 ++
>  drivers/gpu/drm/cirrus/cirrus_drv.c   |    2 ++
>  drivers/gpu/drm/drm_pci.c             |    4 +++-
>  drivers/gpu/drm/gma500/psb_drv.c      |    3 +++
>  drivers/gpu/drm/i915/i915_drv.c       |    2 ++
>  drivers/gpu/drm/mgag200/mgag200_drv.c |    2 ++
>  drivers/gpu/drm/nouveau/nouveau_drv.c |    2 ++
>  drivers/gpu/drm/radeon/radeon_drv.c   |    2 ++
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   |    2 ++
>  9 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d0c4574..6d26e53 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -72,6 +72,8 @@ ast_pci_remove(struct pci_dev *pdev)
>  {
>        struct drm_device *dev = pci_get_drvdata(pdev);
>
> +       pci_disable_device(dev->pdev);
> +
>        drm_put_dev(dev);
>  }
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
> index d703823..d316ba3 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_drv.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
> @@ -45,6 +45,8 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
>  {
>        struct drm_device *dev = pci_get_drvdata(pdev);
>
> +       pci_disable_device(dev->pdev);
> +
>        drm_put_dev(dev);
>  }
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 59e11e4..73218ac 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -455,8 +455,10 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
>        if (driver->driver_features & DRIVER_MODESET) {
>                pci_unregister_driver(pdriver);
>        } else {
> -               list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item)
> +               list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) {
> +                       pci_disable_device(dev->pdev);

I'm assuming that we take a ref in the enter func as well?

Cheers, Jakob.

Re: [PATCH 1/2] drm: call pci_disable_device on module onload

[Daniel Vetter]

On Fri, Jun 08, 2012 at 06:03:57PM +0200, Jakob Bornecrantz wrote:
> On Fri, Jun 8, 2012 at 4:52 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Otherwise we'll nicely leak this reference counter. Now thanks to the
> > awesome layering in the drm core, the enable call is done by the pci
> > boilerplate in drm_pci.c. But the disable can't be done without adding
> > yet another neat indirection layer just for that.
> >
> > So take the simple way and sprinkle pci_disable_device over all pci
> > modesetting drivers.
> >
> > Also don't forget these dear old legacy drivers, prinkle the
> > pci_disable_device call in drm_pci_exit to cover these, too.
> >
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Looks good, one question inlined.
> 
> CC: Stable?

Hm, don't think, no one complained ;-) And we'll only hit this in
module-unload, which is full of much larger issues.

> > ---
> >  drivers/gpu/drm/ast/ast_drv.c         |    2 ++
> >  drivers/gpu/drm/cirrus/cirrus_drv.c   |    2 ++
> >  drivers/gpu/drm/drm_pci.c             |    4 +++-
> >  drivers/gpu/drm/gma500/psb_drv.c      |    3 +++
> >  drivers/gpu/drm/i915/i915_drv.c       |    2 ++
> >  drivers/gpu/drm/mgag200/mgag200_drv.c |    2 ++
> >  drivers/gpu/drm/nouveau/nouveau_drv.c |    2 ++
> >  drivers/gpu/drm/radeon/radeon_drv.c   |    2 ++
> >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   |    2 ++
> >  9 files changed, 20 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index d0c4574..6d26e53 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -72,6 +72,8 @@ ast_pci_remove(struct pci_dev *pdev)
> >  {
> >        struct drm_device *dev = pci_get_drvdata(pdev);
> >
> > +       pci_disable_device(dev->pdev);
> > +
> >        drm_put_dev(dev);
> >  }
> >
> > diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
> > index d703823..d316ba3 100644
> > --- a/drivers/gpu/drm/cirrus/cirrus_drv.c
> > +++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
> > @@ -45,6 +45,8 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
> >  {
> >        struct drm_device *dev = pci_get_drvdata(pdev);
> >
> > +       pci_disable_device(dev->pdev);
> > +
> >        drm_put_dev(dev);
> >  }
> >
> > diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> > index 59e11e4..73218ac 100644
> > --- a/drivers/gpu/drm/drm_pci.c
> > +++ b/drivers/gpu/drm/drm_pci.c
> > @@ -455,8 +455,10 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
> >        if (driver->driver_features & DRIVER_MODESET) {
> >                pci_unregister_driver(pdriver);
> >        } else {
> > -               list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item)
> > +               list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) {
> > +                       pci_disable_device(dev->pdev);
> 
> I'm assuming that we take a ref in the enter func as well?

Yeah, it's a bit convoluted: Drivers call drm_pci_init which either
registers a real pci driver (for modeset) or does the shadow attach dance.
In either case we later on end up in drm_get_pci_dev which then call
pci_enable_device. Imo that kind of low-level hw frobbing shouldn't be
done by the drm code (for suspend/resume we've moved the pci
enable/disable device calls into drivers a long time ago), but that's
material for an entirely different patch series ;-)

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm: fixup error path after drm_fill_in_dev

[Daniel Vetter]

Within drm_fill_in_dev we call drm_lastclose to clean up the dirt in
case of failures. But the callers of drm_fill_in_dev simply don't do
anything. Now from a cursory look drm_lastclose doesn't look like the
best cleanup function in itself, but whom am I to judge the drm error
paths. Imo before we could tackle this we need to move more of the
legacy drm services setup and teardown into the respective drivers,
that way drm_lastclose would become more manageble.

Anyway, make things at least consistent by adding drm_lastclose at the
right places in the error paths for all callers of drm_fill_in_dev.

v2: Fixup the error-path confusion in drm_usb.c, noticed by Jani
Nikula.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_pci.c      |    8 +++++---
 drivers/gpu/drm/drm_platform.c |    8 +++++---
 drivers/gpu/drm/drm_usb.c      |    8 +++++---
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 73218ac..40737a8 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -334,14 +334,14 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
 
 	if ((ret = drm_fill_in_dev(dev, ent, driver))) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g2;
+		goto err_pci;
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		pci_set_drvdata(pdev, dev);
 		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 		if (ret)
-			goto err_g2;
+			goto err_drm_fill_in;
 	}
 
 	if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
@@ -375,7 +375,9 @@ err_g4:
 err_g3:
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_put_minor(&dev->control);
-err_g2:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_pci:
 	pci_disable_device(pdev);
 err_g1:
 	kfree(dev);
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 82431dc..177893a 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -60,14 +60,14 @@ int drm_get_platform_dev(struct platform_device *platdev,
 
 	if (ret) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g1;
+		goto err_kfree;
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		dev_set_drvdata(&platdev->dev, dev);
 		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 		if (ret)
-			goto err_g1;
+			goto err_drm_fill_in;
 	}
 
 	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
@@ -103,7 +103,9 @@ err_g3:
 err_g2:
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_put_minor(&dev->control);
-err_g1:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_kfree:
 	kfree(dev);
 	mutex_unlock(&drm_global_mutex);
 	return ret;
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index 37c9a52..f9d645f 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -25,13 +25,13 @@ int drm_get_usb_dev(struct usb_interface *interface,
 	ret = drm_fill_in_dev(dev, NULL, driver);
 	if (ret) {
 		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
-		goto err_g1;
+		goto err_kfree;
 	}
 
 	usb_set_intfdata(interface, dev);
 	ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
 	if (ret)
-		goto err_g1;
+		goto err_drm_fill_in;
 
 	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
 	if (ret)
@@ -63,7 +63,9 @@ err_g3:
 	drm_put_minor(&dev->primary);
 err_g2:
 	drm_put_minor(&dev->control);
-err_g1:
+err_drm_fill_in:
+	drm_lastclose(dev);
+err_kfree:
 	kfree(dev);
 	mutex_unlock(&drm_global_mutex);
 	return ret;
-- 
1.7.7.6

Re: [PATCH 2/2] drm: fixup error path after drm_fill_in_dev

[Jani Nikula]

On Fri, 08 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Within drm_fill_in_dev we call drm_lastclose to clean up the dirt in
> case of failures. But the callers of drm_fill_in_dev simply don't do
> anything. Now from a cursory look drm_lastclose doesn't look like the
> best cleanup function in itself, but whom am I to judge the drm error
> paths. Imo before we could tackle this we need to move more of the
> legacy drm services setup and teardown into the respective drivers,
> that way drm_lastclose would become more manageble.
>
> Anyway, make things at least consistent by adding drm_lastclose at the
> right places in the error paths for all callers of drm_fill_in_dev.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      |    8 +++++---
>  drivers/gpu/drm/drm_platform.c |    8 +++++---
>  drivers/gpu/drm/drm_usb.c      |    8 +++++---
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 73218ac..40737a8 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -334,14 +334,14 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
>  
>  	if ((ret = drm_fill_in_dev(dev, ent, driver))) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g2;
> +		goto err_pci;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		pci_set_drvdata(pdev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g2;
> +			goto err_drm_fill_in;
>  	}
>  
>  	if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
> @@ -375,7 +375,9 @@ err_g4:
>  err_g3:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g2:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_pci:
>  	pci_disable_device(pdev);
>  err_g1:
>  	kfree(dev);
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> index 82431dc..177893a 100644
> --- a/drivers/gpu/drm/drm_platform.c
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -60,14 +60,14 @@ int drm_get_platform_dev(struct platform_device *platdev,
>  
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_kfree;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		dev_set_drvdata(&platdev->dev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g1;
> +			goto err_drm_fill_in;
>  	}
>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
> @@ -103,7 +103,9 @@ err_g3:
>  err_g2:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
> index 37c9a52..2e179f1 100644
> --- a/drivers/gpu/drm/drm_usb.c
> +++ b/drivers/gpu/drm/drm_usb.c
> @@ -25,13 +25,13 @@ int drm_get_usb_dev(struct usb_interface *interface,
>  	ret = drm_fill_in_dev(dev, NULL, driver);
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_drm_fill_in;
>  	}
>  
>  	usb_set_intfdata(interface, dev);
>  	ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  	if (ret)
> -		goto err_g1;
> +		goto err_kfree;

The above gotos err_drm_fill_in/err_kfree should switch places.

BR,
Jani.

>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
>  	if (ret)
> @@ -63,7 +63,9 @@ err_g3:
>  	drm_put_minor(&dev->primary);
>  err_g2:
>  	drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> -- 
> 1.7.7.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: fixup error path after drm_fill_in_dev

[Jani Nikula]

On Tue, 12 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Within drm_fill_in_dev we call drm_lastclose to clean up the dirt in
> case of failures. But the callers of drm_fill_in_dev simply don't do
> anything. Now from a cursory look drm_lastclose doesn't look like the
> best cleanup function in itself, but whom am I to judge the drm error
> paths. Imo before we could tackle this we need to move more of the
> legacy drm services setup and teardown into the respective drivers,
> that way drm_lastclose would become more manageble.
>
> Anyway, make things at least consistent by adding drm_lastclose at the
> right places in the error paths for all callers of drm_fill_in_dev.
>
> v2: Fixup the error-path confusion in drm_usb.c, noticed by Jani
> Nikula.

You could just claim you planted it there for me. ;)

On the series,
Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com>

>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      |    8 +++++---
>  drivers/gpu/drm/drm_platform.c |    8 +++++---
>  drivers/gpu/drm/drm_usb.c      |    8 +++++---
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 73218ac..40737a8 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -334,14 +334,14 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
>  
>  	if ((ret = drm_fill_in_dev(dev, ent, driver))) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g2;
> +		goto err_pci;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		pci_set_drvdata(pdev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g2;
> +			goto err_drm_fill_in;
>  	}
>  
>  	if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
> @@ -375,7 +375,9 @@ err_g4:
>  err_g3:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g2:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_pci:
>  	pci_disable_device(pdev);
>  err_g1:
>  	kfree(dev);
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> index 82431dc..177893a 100644
> --- a/drivers/gpu/drm/drm_platform.c
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -60,14 +60,14 @@ int drm_get_platform_dev(struct platform_device *platdev,
>  
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_kfree;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		dev_set_drvdata(&platdev->dev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g1;
> +			goto err_drm_fill_in;
>  	}
>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
> @@ -103,7 +103,9 @@ err_g3:
>  err_g2:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
> index 37c9a52..f9d645f 100644
> --- a/drivers/gpu/drm/drm_usb.c
> +++ b/drivers/gpu/drm/drm_usb.c
> @@ -25,13 +25,13 @@ int drm_get_usb_dev(struct usb_interface *interface,
>  	ret = drm_fill_in_dev(dev, NULL, driver);
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_kfree;
>  	}
>  
>  	usb_set_intfdata(interface, dev);
>  	ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  	if (ret)
> -		goto err_g1;
> +		goto err_drm_fill_in;
>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
>  	if (ret)
> @@ -63,7 +63,9 @@ err_g3:
>  	drm_put_minor(&dev->primary);
>  err_g2:
>  	drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> -- 
> 1.7.7.6