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 at 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 at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
