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); }
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;
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
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;
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
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.
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Jakob Bornecrantz -
Jani Nikula