[Intel-gfx] [PATCH] drm/i915: Use drm_dev_unplug()

[Janusz Krzysztofik]

On Fri, 2019-04-05 at 09:24 +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-04-05 09:11:54)
> > On Fri, 2019-04-05 at 08:41 +0100, Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > > > From: Janusz Krzysztofik <janusz.krzysztofik at intel.com>
> > > > 
> > > > The driver does not currently support unbinding from a device
> > > > which
> > > > is
> > > > in use.  Since open file descriptors may still be pointing into
> > > > kernel
> > > > memory where the device structures used to be, entirely correct
> > > > kernel
> > > > panics protect the driver from being unbound as we should not
> > > > be
> > > > unbinding it before those dangling pointers have been made
> > > > safe.
> > > > 
> > > > According to the documentation found inside
> > > > drivers/gpu/drm/drm_drv.c,
> > > > drm_dev_unplug() should be used instead of drm_dev_unregister()
> > > > in
> > > > order to make a device inaccessible to users as soon as it is
> > > > unpluged.
> > > > Follow that advice to make those possibly dangling pointers
> > > > safe,
> > > > protected by DRM layer from a user who is otherwise left
> > > > pointing
> > > > into
> > > > possibly reused kernel memory after the driver has been unbound
> > > > from
> > > > the device.
> > > > 
> > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at intel.com
> > > > >
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 9df65d386d11..66163378c481 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1596,7 +1596,7 @@ static void i915_driver_unregister(struct
> > > > drm_i915_private *dev_priv)
> > > >         i915_pmu_unregister(dev_priv);
> > > >  
> > > >         i915_teardown_sysfs(dev_priv);
> > > > -       drm_dev_unregister(&dev_priv->drm);
> > > > +       drm_dev_unplug(&dev_priv->drm);
> > > 
> > > I think we may have our onion inverted here. We want to stop the
> > > users
> > > as the first step, then start removing the entries. (That will
> > > also
> > > nicely invert the order from register, which is what we typically
> > > expect).
> > > 
> > > After calling i915_driver_unregister(); call
> > > i915_gem_set_wedged() to
> > > immediately (give or take external fences) cancel inflight
> > > operations.
> > 
> > OK, thanks.  Do you prefer them squashed or as serparate patches?
> 
> Quite happy to do the s/unregister/unplug/ and move in one go. Have a
> pre-emptive
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> on that as that seems to be the right thing to do.
> 
> And there should be no issues in placing a i915_gem_set_wedged()
> immediately after the call to i915_driver_unregister, so if you
> include
> a line of commentary about why, for example
> 
> /*
>  * After unregistering the device to prevent any new users, cancel
>  * all in-flight requests so that we can quickly unbind the active
>  * resources.
>  */
> i915_gem_set_wedged(dev_priv);
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

I've given it some testing, no side effects with test workloads I've
tried, and looks like it at least helps to prevent from making the
device actually wedged.

With these two patches, plus the one we discussed yesterday, and yet
another one I'm going to submit soon, I'm now able to unbind the driver
from a device while a workload is running on it, unload the module,
reload it and successfully perform basic GEM health checks, all in a
quick succession :-).

Unfortunately, not 100% reproducible, as well as not the case with
device unplug simulated by writing 1 to device/remove sysfs file.
Surely that needs the work you describe below to be done first.

Thanks for your cooperation,
Janusz


> 
> I think overall though, we need to go through i915_driver_unload()
> and
> push the module cleanup operations to i915_driver_release -- that
> will
> take a bit of surgery to separate the different phases that are
> currently smashed together.
> -Chris
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel