[02/51] drm/i915: Don't clear drvdata in ->release
diff mbox series

Message ID 20200221210319.2245170-3-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • drm managed resources, v2
Related show

Commit Message

Daniel Vetter Feb. 21, 2020, 9:02 p.m. UTC
For two reasons:

- The driver core clears this already for us after we're unloaded in
  __device_release_driver().

- It's way too late, the drm_device ->release callback might massively
  outlive the underlying physical device, since a drm_device can't be
  kept alive by open drm_file or well really anything else userspace
  is still hanging onto. So if we clear this ourselves, we should
  clear it in the pci ->remove callback, not in the drm_device
  ->relase callback.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Chris Wilson Feb. 21, 2020, 9:36 p.m. UTC | #1
Quoting Daniel Vetter (2020-02-21 21:02:30)
> For two reasons:
> 
> - The driver core clears this already for us after we're unloaded in
>   __device_release_driver().

Even if we abort before loading?

History notes that i915_pci_remove was called with a stale pointer on
error.
-Chris
Daniel Vetter Feb. 22, 2020, 9:48 a.m. UTC | #2
On Fri, Feb 21, 2020 at 10:36 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2020-02-21 21:02:30)
> > For two reasons:
> >
> > - The driver core clears this already for us after we're unloaded in
> >   __device_release_driver().
>
> Even if we abort before loading?
>
> History notes that i915_pci_remove was called with a stale pointer on
> error.

So even if there's a bug we still have the problem that clearing the
pci_drvdata in our drm_driver->release hook is way too late. You could
already have bound a new driver to the underlying device. So if driver
core doesn't clear drvdata on bind failure and we need to clear this
ourselves, then this line here could actually clear the drvdata of the
next driver instance bound to the pci device. Not that that's ever
going to happen outside of very contrived testing.

But looking at really_probe() in base/dd.c we do clear drvdata on
failure. So no idea how/why that stale drvdata came to be. Anyway
that's kinda why I cc'ed Greg, so he could confirm that this is
correct.
-Daniel
Daniel Vetter Feb. 22, 2020, 9:50 a.m. UTC | #3
On Sat, Feb 22, 2020 at 10:48 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Fri, Feb 21, 2020 at 10:36 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Daniel Vetter (2020-02-21 21:02:30)
> > > For two reasons:
> > >
> > > - The driver core clears this already for us after we're unloaded in
> > >   __device_release_driver().
> >
> > Even if we abort before loading?
> >
> > History notes that i915_pci_remove was called with a stale pointer on
> > error.
>
> So even if there's a bug we still have the problem that clearing the
> pci_drvdata in our drm_driver->release hook is way too late. You could
> already have bound a new driver to the underlying device. So if driver
> core doesn't clear drvdata on bind failure and we need to clear this
> ourselves, then this line here could actually clear the drvdata of the
> next driver instance bound to the pci device. Not that that's ever
> going to happen outside of very contrived testing.
>
> But looking at really_probe() in base/dd.c we do clear drvdata on
> failure. So no idea how/why that stale drvdata came to be. Anyway
> that's kinda why I cc'ed Greg, so he could confirm that this is
> correct.

Looking at git history, this was fixed in driver core in

commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
Author: Hans de Goede <hdegoede@redhat.com>
Date:   Wed May 23 00:09:34 2012 +0200

    device-core: Ensure drvdata = NULL when no driver is bound

I'll add that to the commit message.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 759d333448e1..8b8a9c9a9b2a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1391,9 +1391,6 @@  static void i915_driver_destroy(struct drm_i915_private *i915)
 
 	drm_dev_fini(&i915->drm);
 	kfree(i915);
-
-	/* And make sure we never chase our dangling pointer from pci_dev */
-	pci_set_drvdata(pdev, NULL);
 }
 
 /**