* [PATCH 1/2] drm: call pci_disable_device on module onload @ 2012-06-08 14:52 Daniel Vetter 2012-06-08 14:52 ` [PATCH 2/2] drm: fixup error path after drm_fill_in_dev Daniel Vetter 2012-06-08 16:03 ` [PATCH 1/2] drm: call pci_disable_device on module onload Jakob Bornecrantz 0 siblings, 2 replies; 7+ messages in thread From: Daniel Vetter @ 2012-06-08 14:52 UTC (permalink / raw) To: Intel Graphics Development, DRI Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm: fixup error path after drm_fill_in_dev 2012-06-08 14:52 [PATCH 1/2] drm: call pci_disable_device on module onload Daniel Vetter @ 2012-06-08 14:52 ` Daniel Vetter 2012-06-12 12:33 ` Jani Nikula 2012-06-08 16:03 ` [PATCH 1/2] drm: call pci_disable_device on module onload Jakob Bornecrantz 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-06-08 14:52 UTC (permalink / raw) To: Intel Graphics Development, DRI Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm: fixup error path after drm_fill_in_dev 2012-06-08 14:52 ` [PATCH 2/2] drm: fixup error path after drm_fill_in_dev Daniel Vetter @ 2012-06-12 12:33 ` Jani Nikula 2012-06-12 11:31 ` [PATCH] " Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Jani Nikula @ 2012-06-12 12:33 UTC (permalink / raw) To: Intel Graphics Development, DRI Development; +Cc: Daniel Vetter 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] drm: fixup error path after drm_fill_in_dev 2012-06-12 12:33 ` Jani Nikula @ 2012-06-12 11:31 ` Daniel Vetter 2012-06-12 12:50 ` Jani Nikula 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-06-12 11:31 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter, DRI Development 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: fixup error path after drm_fill_in_dev 2012-06-12 11:31 ` [PATCH] " Daniel Vetter @ 2012-06-12 12:50 ` Jani Nikula 0 siblings, 0 replies; 7+ messages in thread From: Jani Nikula @ 2012-06-12 12:50 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter, DRI Development 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm: call pci_disable_device on module onload 2012-06-08 14:52 [PATCH 1/2] drm: call pci_disable_device on module onload Daniel Vetter 2012-06-08 14:52 ` [PATCH 2/2] drm: fixup error path after drm_fill_in_dev Daniel Vetter @ 2012-06-08 16:03 ` Jakob Bornecrantz 2012-06-08 16:16 ` Daniel Vetter 1 sibling, 1 reply; 7+ messages in thread From: Jakob Bornecrantz @ 2012-06-08 16:03 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm: call pci_disable_device on module onload 2012-06-08 16:03 ` [PATCH 1/2] drm: call pci_disable_device on module onload Jakob Bornecrantz @ 2012-06-08 16:16 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2012-06-08 16:16 UTC (permalink / raw) To: Jakob Bornecrantz Cc: Daniel Vetter, Intel Graphics Development, DRI Development 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-06-12 12:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-08 14:52 [PATCH 1/2] drm: call pci_disable_device on module onload Daniel Vetter 2012-06-08 14:52 ` [PATCH 2/2] drm: fixup error path after drm_fill_in_dev Daniel Vetter 2012-06-12 12:33 ` Jani Nikula 2012-06-12 11:31 ` [PATCH] " Daniel Vetter 2012-06-12 12:50 ` Jani Nikula 2012-06-08 16:03 ` [PATCH 1/2] drm: call pci_disable_device on module onload Jakob Bornecrantz 2012-06-08 16:16 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox