From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm: call pci_disable_device on module onload Date: Fri, 8 Jun 2012 18:16:29 +0200 Message-ID: <20120608161629.GD5761@phenom.ffwll.local> References: <1339167175-1447-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F5589E9BA for ; Fri, 8 Jun 2012 09:14:59 -0700 (PDT) Received: by werc12 with SMTP id c12so839951wer.36 for ; Fri, 08 Jun 2012 09:14:58 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jakob Bornecrantz Cc: Daniel Vetter , Intel Graphics Development , DRI Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 08, 2012 at 06:03:57PM +0200, Jakob Bornecrantz wrote: > On Fri, Jun 8, 2012 at 4:52 PM, Daniel Vetter wr= ote: > > 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 > = > 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. > > --- > > =A0drivers/gpu/drm/ast/ast_drv.c =A0 =A0 =A0 =A0 | =A0 =A02 ++ > > =A0drivers/gpu/drm/cirrus/cirrus_drv.c =A0 | =A0 =A02 ++ > > =A0drivers/gpu/drm/drm_pci.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A04 +++- > > =A0drivers/gpu/drm/gma500/psb_drv.c =A0 =A0 =A0| =A0 =A03 +++ > > =A0drivers/gpu/drm/i915/i915_drv.c =A0 =A0 =A0 | =A0 =A02 ++ > > =A0drivers/gpu/drm/mgag200/mgag200_drv.c | =A0 =A02 ++ > > =A0drivers/gpu/drm/nouveau/nouveau_drv.c | =A0 =A02 ++ > > =A0drivers/gpu/drm/radeon/radeon_drv.c =A0 | =A0 =A02 ++ > > =A0drivers/gpu/drm/vmwgfx/vmwgfx_drv.c =A0 | =A0 =A02 ++ > > =A09 files changed, 20 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_dr= v.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) > > =A0{ > > =A0 =A0 =A0 =A0struct drm_device *dev =3D pci_get_drvdata(pdev); > > > > + =A0 =A0 =A0 pci_disable_device(dev->pdev); > > + > > =A0 =A0 =A0 =A0drm_put_dev(dev); > > =A0} > > > > diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirr= us/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) > > =A0{ > > =A0 =A0 =A0 =A0struct drm_device *dev =3D pci_get_drvdata(pdev); > > > > + =A0 =A0 =A0 pci_disable_device(dev->pdev); > > + > > =A0 =A0 =A0 =A0drm_put_dev(dev); > > =A0} > > > > 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, struc= t pci_driver *pdriver) > > =A0 =A0 =A0 =A0if (driver->driver_features & DRIVER_MODESET) { > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_unregister_driver(pdriver); > > =A0 =A0 =A0 =A0} else { > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 list_for_each_entry_safe(dev, tmp, &drive= r->device_list, driver_item) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 list_for_each_entry_safe(dev, tmp, &drive= r->device_list, driver_item) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_disable_device(dev->p= dev); > = > 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