* [PATCH] drm/i915: Use drm_dev_unplug()
@ 2019-04-05 7:26 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 7:26 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, Janusz Krzysztofik
From: Janusz Krzysztofik <janusz.krzysztofik@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@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);
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] drm/i915: Use drm_dev_unplug()
@ 2019-04-05 7:26 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 7:26 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: Janusz Krzysztofik, David Airlie, intel-gfx, linux-kernel, dri-devel
From: Janusz Krzysztofik <janusz.krzysztofik@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@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);
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
2019-04-05 7:26 ` Janusz Krzysztofik
(?)
@ 2019-04-05 7:41 ` Chris Wilson
2019-04-05 8:11 ` Janusz Krzysztofik
2019-04-15 9:29 ` Daniel Vetter
-1 siblings, 2 replies; 22+ messages in thread
From: Chris Wilson @ 2019-04-05 7:41 UTC (permalink / raw)
To: Jani Nikula, Janusz Krzysztofik, Joonas Lahtinen, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, michal.wajdeczko, intel-gfx,
dri-devel, linux-kernel, Janusz Krzysztofik
Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> From: Janusz Krzysztofik <janusz.krzysztofik@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@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.
-Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
2019-04-05 7:41 ` Chris Wilson
@ 2019-04-05 8:11 ` Janusz Krzysztofik
2019-04-05 8:24 ` Chris Wilson
2019-04-15 9:29 ` Daniel Vetter
1 sibling, 1 reply; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 8:11 UTC (permalink / raw)
To: Chris Wilson
Cc: David Airlie, intel-gfx, linux-kernel, dri-devel,
michal.wajdeczko, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
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@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@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?
Thanks,
Janusz
> -Chris
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Use drm_dev_unplug()
2019-04-05 7:26 ` Janusz Krzysztofik
(?)
(?)
@ 2019-04-05 8:23 ` Patchwork
-1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-04-05 8:23 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use drm_dev_unplug()
URL : https://patchwork.freedesktop.org/series/59041/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5877 -> Patchwork_12691
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12691 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12691, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/59041/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12691:
### IGT changes ###
#### Possible regressions ####
* igt@i915_module_load@reload:
- fi-kbl-r: PASS -> INCOMPLETE
- fi-whl-u: PASS -> INCOMPLETE
- fi-pnv-d510: PASS -> INCOMPLETE
- fi-skl-iommu: PASS -> INCOMPLETE
- fi-skl-6700k2: NOTRUN -> INCOMPLETE
- fi-bsw-kefka: PASS -> INCOMPLETE
- fi-bdw-5557u: PASS -> INCOMPLETE
- fi-skl-guc: PASS -> INCOMPLETE
- fi-kbl-guc: PASS -> INCOMPLETE
- fi-cfl-8109u: PASS -> INCOMPLETE
- fi-cfl-8700k: PASS -> INCOMPLETE
- fi-snb-2520m: PASS -> INCOMPLETE
- fi-hsw-4770: PASS -> INCOMPLETE
- fi-cfl-guc: PASS -> INCOMPLETE
- fi-skl-6770hq: PASS -> INCOMPLETE
- fi-bsw-n3050: NOTRUN -> INCOMPLETE
- fi-ilk-650: PASS -> INCOMPLETE
- fi-ivb-3770: PASS -> INCOMPLETE
- fi-skl-lmem: PASS -> INCOMPLETE
- fi-hsw-peppy: PASS -> INCOMPLETE
- fi-skl-6260u: PASS -> INCOMPLETE
- fi-kbl-x1275: PASS -> INCOMPLETE
- fi-bwr-2160: PASS -> INCOMPLETE
* igt@runner@aborted:
- fi-ilk-650: NOTRUN -> FAIL
- fi-pnv-d510: NOTRUN -> FAIL
- fi-bdw-gvtdvm: NOTRUN -> FAIL
- fi-cfl-8109u: NOTRUN -> FAIL
- fi-hsw-peppy: NOTRUN -> FAIL
- fi-icl-u2: NOTRUN -> FAIL
- fi-snb-2520m: NOTRUN -> FAIL
- fi-bxt-j4205: NOTRUN -> FAIL
- fi-ivb-3770: NOTRUN -> FAIL
- fi-byt-j1900: NOTRUN -> FAIL
- fi-cfl-guc: NOTRUN -> FAIL
- fi-icl-y: NOTRUN -> FAIL
- fi-bsw-n3050: NOTRUN -> FAIL
- fi-kbl-x1275: NOTRUN -> FAIL
- fi-bsw-kefka: NOTRUN -> FAIL
- fi-kbl-8809g: NOTRUN -> FAIL
- fi-byt-clapper: NOTRUN -> FAIL
- fi-kbl-r: NOTRUN -> FAIL
- fi-bdw-5557u: NOTRUN -> FAIL
- fi-bwr-2160: NOTRUN -> FAIL
- fi-byt-n2820: NOTRUN -> FAIL
- fi-snb-2600: NOTRUN -> FAIL
- fi-elk-e7500: NOTRUN -> FAIL
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_module_load@reload:
- {fi-kbl-7567u}: PASS -> INCOMPLETE
* igt@runner@aborted:
- {fi-icl-u3}: NOTRUN -> FAIL
- {fi-kbl-7567u}: NOTRUN -> FAIL
Known issues
------------
Here are the changes found in Patchwork_12691 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_suspend@basic-s4-devices:
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
* igt@i915_module_load@reload:
- fi-kbl-8809g: PASS -> INCOMPLETE [fdo#103665]
- fi-byt-j1900: PASS -> INCOMPLETE [fdo#102657]
- fi-apl-guc: PASS -> INCOMPLETE [fdo#103927]
- fi-skl-6600u: PASS -> INCOMPLETE [fdo#104108]
- fi-elk-e7500: PASS -> INCOMPLETE [fdo#103989]
- fi-byt-clapper: PASS -> INCOMPLETE [fdo#102657]
- fi-bdw-gvtdvm: PASS -> INCOMPLETE [fdo#105600]
- fi-icl-u2: PASS -> INCOMPLETE [fdo#107713]
- fi-skl-gvtdvm: PASS -> INCOMPLETE [fdo#105600]
- fi-bxt-j4205: PASS -> INCOMPLETE [fdo#103927]
- fi-byt-n2820: PASS -> INCOMPLETE [fdo#102657]
- fi-snb-2600: PASS -> INCOMPLETE [fdo#105411]
- fi-icl-y: PASS -> INCOMPLETE [fdo#107713]
* igt@kms_busy@basic-flip-a:
- fi-bsw-n3050: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_chamelium@dp-hpd-fast:
- fi-skl-6700k2: NOTRUN -> SKIP [fdo#109271] +23
* igt@kms_chamelium@hdmi-crc-fast:
- fi-bsw-n3050: NOTRUN -> SKIP [fdo#109271] +44
* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: PASS -> FAIL [fdo#103167]
* igt@runner@aborted:
- fi-skl-guc: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6700k2: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6600u: NOTRUN -> FAIL [fdo#104108]
- fi-skl-lmem: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6770hq: NOTRUN -> FAIL [fdo#104108]
- fi-skl-gvtdvm: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6260u: NOTRUN -> FAIL [fdo#104108]
#### Possible fixes ####
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#103989]: https://bugs.freedesktop.org/show_bug.cgi?id=103989
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#105600]: https://bugs.freedesktop.org/show_bug.cgi?id=105600
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
Participating hosts (48 -> 43)
------------------------------
Additional (2): fi-skl-6700k2 fi-bsw-n3050
Missing (7): fi-kbl-soraka fi-hsw-4770r fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-gdg-551
Build changes
-------------
* Linux: CI_DRM_5877 -> Patchwork_12691
CI_DRM_5877: 03f3a57c6df4ef469bd9f528bb9e0201ddd0ee14 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4929: 8770e24fe4008404da769c16b7edac6142225ad7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12691: 272df22420ed1239c2c171edc46277c88282e684 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
272df22420ed drm/i915: Use drm_dev_unplug()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12691/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
2019-04-05 8:11 ` Janusz Krzysztofik
@ 2019-04-05 8:24 ` Chris Wilson
0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-04-05 8:24 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: David Airlie, intel-gfx, linux-kernel, dri-devel,
michal.wajdeczko, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
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@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@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@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@chris-wilson.co.uk>
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
@ 2019-04-05 8:24 ` Chris Wilson
0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-04-05 8:24 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: David Airlie, intel-gfx, linux-kernel, dri-devel
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@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@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@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@chris-wilson.co.uk>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
2019-04-05 8:24 ` Chris Wilson
(?)
@ 2019-04-05 11:38 ` Janusz Krzysztofik
-1 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 11:38 UTC (permalink / raw)
To: Chris Wilson
Cc: David Airlie, intel-gfx, linux-kernel, dri-devel, Rodrigo Vivi,
michal.wajdeczko
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@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@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@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/2] Stop users from using the device on driver unbind
@ 2019-04-05 13:02 Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 13:02 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, Janusz Krzysztofik
Use drm_dev_unplug() to have device resources protected from user access
by DRM layer as soon as the driver is going to be unbound. Also, cancel
all pending work so associated resources can be quickly released.
Janusz Krzysztofik (2):
drm/i915: Use drm_dev_unplug()
drm/i915: Mark GEM wedged right after marking device unplugged
drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
I'm resending these two patches together in series to make the robot
happy about the second one. Also, I've added the Suggested-by: clause
to credit actual Chris' contribution.
Thanks,
Janusz
--
2.20.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] drm/i915: Use drm_dev_unplug()
2019-04-05 13:02 [PATCH 0/2] Stop users from using the device on driver unbind Janusz Krzysztofik
@ 2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 16:20 ` ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind Patchwork
2 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 13:02 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, Janusz Krzysztofik
From: Janusz Krzysztofik <janusz.krzysztofik@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. Once done, also cancel inflight operations immediately by
calling i915_gem_set_wedged().
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
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);
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 1/2] drm/i915: Use drm_dev_unplug()
@ 2019-04-05 13:02 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 13:02 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: Janusz Krzysztofik, David Airlie, intel-gfx, linux-kernel, dri-devel
From: Janusz Krzysztofik <janusz.krzysztofik@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. Once done, also cancel inflight operations immediately by
calling i915_gem_set_wedged().
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
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);
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] drm/i915: Mark GEM wedged right after marking device unplugged
2019-04-05 13:02 [PATCH 0/2] Stop users from using the device on driver unbind Janusz Krzysztofik
@ 2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 16:20 ` ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind Patchwork
2 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 13:02 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, Janusz Krzysztofik
As soon as a device is considered unplugged, not only prevent pending
users from accessing the device structures but also cancel all their
pending requests so all consumed resources can be cleaned up as soon
as possible.
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66163378c481..03a563ce7e6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1598,6 +1598,13 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
i915_teardown_sysfs(dev_priv);
drm_dev_unplug(&dev_priv->drm);
+ /*
+ * 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);
+
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] drm/i915: Mark GEM wedged right after marking device unplugged
@ 2019-04-05 13:02 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-05 13:02 UTC (permalink / raw)
To: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi
Cc: David Airlie, intel-gfx, linux-kernel, dri-devel
As soon as a device is considered unplugged, not only prevent pending
users from accessing the device structures but also cancel all their
pending requests so all consumed resources can be cleaned up as soon
as possible.
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66163378c481..03a563ce7e6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1598,6 +1598,13 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
i915_teardown_sysfs(dev_priv);
drm_dev_unplug(&dev_priv->drm);
+ /*
+ * 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);
+
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind
2019-04-05 13:02 [PATCH 0/2] Stop users from using the device on driver unbind Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
@ 2019-04-05 16:20 ` Patchwork
2019-04-05 16:26 ` Chris Wilson
2 siblings, 1 reply; 22+ messages in thread
From: Patchwork @ 2019-04-05 16:20 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
== Series Details ==
Series: Stop users from using the device on driver unbind
URL : https://patchwork.freedesktop.org/series/59064/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5881 -> Patchwork_12699
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12699 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12699, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/59064/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12699:
### IGT changes ###
#### Possible regressions ####
* igt@i915_module_load@reload:
- fi-kbl-r: PASS -> INCOMPLETE
- fi-whl-u: PASS -> INCOMPLETE
- fi-pnv-d510: PASS -> INCOMPLETE
- fi-skl-iommu: PASS -> INCOMPLETE
- fi-hsw-4770r: PASS -> INCOMPLETE
- fi-skl-6700k2: PASS -> INCOMPLETE
- fi-bsw-kefka: PASS -> INCOMPLETE
- fi-bdw-5557u: PASS -> INCOMPLETE
- fi-skl-guc: PASS -> INCOMPLETE
- fi-kbl-guc: PASS -> INCOMPLETE
- fi-cfl-8109u: PASS -> INCOMPLETE
- fi-cfl-8700k: PASS -> INCOMPLETE
- fi-snb-2520m: PASS -> INCOMPLETE
- fi-hsw-4770: PASS -> INCOMPLETE
- fi-cfl-guc: PASS -> INCOMPLETE
- fi-skl-6770hq: PASS -> INCOMPLETE
- fi-bsw-n3050: PASS -> INCOMPLETE
- fi-ilk-650: PASS -> INCOMPLETE
- fi-ivb-3770: PASS -> INCOMPLETE
- fi-skl-lmem: PASS -> INCOMPLETE
- fi-hsw-peppy: PASS -> INCOMPLETE
- fi-skl-6260u: PASS -> INCOMPLETE
- fi-kbl-x1275: PASS -> INCOMPLETE
- fi-gdg-551: PASS -> INCOMPLETE
* igt@runner@aborted:
- fi-ilk-650: NOTRUN -> FAIL
- fi-pnv-d510: NOTRUN -> FAIL
- fi-bdw-gvtdvm: NOTRUN -> FAIL
- fi-cfl-8109u: NOTRUN -> FAIL
- fi-hsw-peppy: NOTRUN -> FAIL
- fi-icl-u2: NOTRUN -> FAIL
- fi-gdg-551: NOTRUN -> FAIL
- fi-snb-2520m: NOTRUN -> FAIL
- fi-hsw-4770: NOTRUN -> FAIL
- fi-bxt-j4205: NOTRUN -> FAIL
- fi-whl-u: NOTRUN -> FAIL
- fi-ivb-3770: NOTRUN -> FAIL
- fi-byt-j1900: NOTRUN -> FAIL
- fi-cfl-guc: NOTRUN -> FAIL
- fi-icl-y: NOTRUN -> FAIL
- fi-bsw-n3050: NOTRUN -> FAIL
- fi-blb-e6850: NOTRUN -> FAIL
- fi-kbl-x1275: NOTRUN -> FAIL
- fi-bsw-kefka: NOTRUN -> FAIL
- fi-cfl-8700k: NOTRUN -> FAIL
- fi-hsw-4770r: NOTRUN -> FAIL
- fi-kbl-8809g: NOTRUN -> FAIL
- fi-byt-clapper: NOTRUN -> FAIL
- fi-kbl-r: NOTRUN -> FAIL
- fi-bdw-5557u: NOTRUN -> FAIL
- fi-bwr-2160: NOTRUN -> FAIL
- fi-byt-n2820: NOTRUN -> FAIL
- fi-kbl-guc: NOTRUN -> FAIL
- fi-snb-2600: NOTRUN -> FAIL
- fi-elk-e7500: NOTRUN -> FAIL
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_module_load@reload:
- {fi-kbl-7567u}: PASS -> INCOMPLETE
* igt@runner@aborted:
- {fi-icl-u3}: NOTRUN -> FAIL
- {fi-kbl-7567u}: NOTRUN -> FAIL
Known issues
------------
Here are the changes found in Patchwork_12699 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_suspend@basic-s4-devices:
- fi-icl-u2: PASS -> DMESG-WARN [fdo#109638]
* igt@i915_module_load@reload:
- fi-kbl-8809g: PASS -> INCOMPLETE [fdo#103665]
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
- fi-byt-j1900: PASS -> INCOMPLETE [fdo#102657]
- fi-apl-guc: PASS -> INCOMPLETE [fdo#103927]
- fi-skl-6600u: PASS -> INCOMPLETE [fdo#104108]
- fi-elk-e7500: PASS -> INCOMPLETE [fdo#103989]
- fi-byt-clapper: PASS -> INCOMPLETE [fdo#102657]
- fi-bdw-gvtdvm: PASS -> INCOMPLETE [fdo#105600]
- fi-icl-u2: PASS -> INCOMPLETE [fdo#107713]
- fi-skl-gvtdvm: PASS -> INCOMPLETE [fdo#105600]
- fi-bxt-j4205: PASS -> INCOMPLETE [fdo#103927]
- fi-byt-n2820: PASS -> INCOMPLETE [fdo#102657]
- fi-snb-2600: PASS -> INCOMPLETE [fdo#105411]
- fi-bwr-2160: PASS -> INCOMPLETE [k.org#201317]
- fi-icl-y: PASS -> INCOMPLETE [fdo#107713]
* igt@runner@aborted:
- fi-skl-iommu: NOTRUN -> FAIL [fdo#104108]
- fi-skl-guc: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6700k2: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6600u: NOTRUN -> FAIL [fdo#104108]
- fi-skl-lmem: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6770hq: NOTRUN -> FAIL [fdo#104108]
- fi-skl-gvtdvm: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6260u: NOTRUN -> FAIL [fdo#104108]
#### Possible fixes ####
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#103989]: https://bugs.freedesktop.org/show_bug.cgi?id=103989
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#105600]: https://bugs.freedesktop.org/show_bug.cgi?id=105600
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#109638]: https://bugs.freedesktop.org/show_bug.cgi?id=109638
[k.org#201317]: https://bugzilla.kernel.org/show_bug.cgi?id=201317
Participating hosts (49 -> 45)
------------------------------
Missing (4): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_5881 -> Patchwork_12699
CI_DRM_5881: b070175c76da1440a747fd023ee6253e573055f8 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4931: 019f892e5d1a0a9643cb726c47ce2d99c14b444f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12699: 494aa340c3a1fa4fac95457839cf7acb1f64d6f6 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
494aa340c3a1 drm/i915: Mark GEM wedged right after marking device unplugged
df465066fd31 drm/i915: Use drm_dev_unplug()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12699/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind
2019-04-05 16:20 ` ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind Patchwork
@ 2019-04-05 16:26 ` Chris Wilson
2019-04-05 17:37 ` Chris Wilson
0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2019-04-05 16:26 UTC (permalink / raw)
To: Janusz Krzysztofik, Patchwork; +Cc: intel-gfx
Quoting Patchwork (2019-04-05 17:20:39)
> == Series Details ==
>
> Series: Stop users from using the device on driver unbind
> URL : https://patchwork.freedesktop.org/series/59064/
> State : failure
>
> == Summary ==
>
> CI Bug Log - changes from CI_DRM_5881 -> Patchwork_12699
> ====================================================
>
> Summary
> -------
>
> **FAILURE**
>
> Serious unknown changes coming with Patchwork_12699 absolutely need to be
> verified manually.
>
> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_12699, please notify your bug team to allow them
> to document this new failure mode, which will reduce false positives in CI.
>
> External URL: https://patchwork.freedesktop.org/api/1.0/series/59064/revisions/1/mbox/
>
> Possible new issues
> -------------------
>
> Here are the unknown changes that may have been introduced in Patchwork_12699:
>
> ### IGT changes ###
>
> #### Possible regressions ####
>
> * igt@i915_module_load@reload:
2 issues, it appears:
<4> [271.799080] WARN_ON(dev_priv->mm.object_count)
<4> [271.799241] WARNING: CPU: 0 PID: 3288 at drivers/gpu/drm/i915/i915_gem.c:5145 i915_gem_cleanup_early+0x104/0x110 [i915]
<4> [271.799249] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915(-) mei_hdcp x86_pkg_temp_thermal btusb coretemp btrtl btbcm btintel bluetooth crct10dif_pclmul crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e ecdh_generic snd_pcm mei_me ptp prime_numbers pps_core mei [last unloaded: snd_hda_intel]
<4> [271.799302] CPU: 0 PID: 3288 Comm: i915_module_loa Tainted: G U 5.1.0-rc3-CI-Patchwork_12699+ #1
<4> [271.799307] Hardware name: /NUC6i7KYB, BIOS KYSKLi70.86A.0059.2018.1122.1431 11/22/2018
<4> [271.799406] RIP: 0010:i915_gem_cleanup_early+0x104/0x110 [i915]
<4> [271.799412] Code: 00 00 48 c7 c2 d0 6b 3d a0 48 c7 c7 ca 5c 2c a0 e8 c1 b5 ec e0 0f 0b 48 c7 c6 68 c0 3f a0 48 c7 c7 63 88 42 a0 e8 9c 77 de e0 <0f> 0b e9 40 ff ff ff 0f 1f 44 00 00 e8 5b 7e 00 00 31 c0 c3 0f 1f
<4> [271.799417] RSP: 0018:ffffc90000453dd0 EFLAGS: 00010282
<4> [271.799423] RAX: 0000000000000000 RBX: ffff88849afd0000 RCX: 0000000000000000
<4> [271.799428] RDX: 0000000000000006 RSI: ffff88849ee130b8 RDI: ffffffff8211dc4d
<4> [271.799432] RBP: ffff88849afd7630 R08: 00000000028bc995 R09: 0000000000000000
<4> [271.799436] R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa04a81e0
<4> [271.799440] R13: 0000000000000000 R14: 0000000000000000 R15: ffffffffa04a82d0
<4> [271.799446] FS: 00007f31e8cec980(0000) GS:ffff8884aee00000(0000) knlGS:0000000000000000
<4> [271.799451] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [271.799455] CR2: 00007ffea58773d8 CR3: 000000044cfc6003 CR4: 00000000003606f0
<4> [271.799459] Call Trace:
<4> [271.799531] i915_driver_cleanup_early+0x30/0x70 [i915]
<4> [271.799603] i915_driver_release+0xa/0x30 [i915]
<4> [271.799672] i915_driver_unload+0x6a/0x120 [i915]
<4> [271.799748] i915_pci_remove+0x19/0x30 [i915]
<4> [271.799765] pci_device_remove+0x36/0xb0
we leaked objects (don't recall that in recent times)
and
<4> [271.812054] general protection fault: 0000 [#1] PREEMPT SMP PTI
<4> [271.812079] CPU: 3 PID: 3288 Comm: i915_module_loa Tainted: G U W 5.1.0-rc3-CI-Patchwork_12699+ #1
<4> [271.812094] Hardware name: /NUC6i7KYB, BIOS KYSKLi70.86A.0059.2018.1122.1431 11/22/2018
<4> [271.812172] RIP: 0010:intel_uncore_forcewake_get+0x0/0xa0 [i915]
<4> [271.812183] Code: e8 fd ff ff 66 0f 1f 44 00 00 48 83 c7 60 e9 17 e0 d9 e0 0f 1f 80 00 00 00 00 e9 5b ed f6 ff 90 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7f 78 00 74 49 41 55 41 54 55 53 89 f5 4c 8b 67 08 48 89 fb
<4> [271.812208] RSP: 0018:ffffc90000453d78 EFLAGS: 00010282
<4> [271.812217] RAX: 6b6b6b6b6b6b6b6b RBX: ffff88849afd75f0 RCX: 0000000000000000
<4> [271.812228] RDX: 0000000000000001 RSI: 00000000000001ff RDI: 6b6b6b6b6b6b7a1b
<4> [271.812239] RBP: ffff888498992158 R08: 000000000029049a R09: ffff8884ac8af000
<4> [271.812249] R10: ffff8884ac8afc40 R11: ffff8884ad266f98 R12: ffff88849afd7630
<4> [271.812260] R13: ffff88849afd0000 R14: 0000000000000000 R15: ffffffffa04a82d0
<4> [271.812271] FS: 00007f31e8cec980(0000) GS:ffff8884aeec0000(0000) knlGS:0000000000000000
<4> [271.812283] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [271.812292] CR2: 000055f1c5db3128 CR3: 000000044cfc6004 CR4: 00000000003606e0
<4> [271.812303] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4> [271.812314] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
<4> [271.812324] Call Trace:
<4> [271.812367] reset_prepare+0x34/0x60 [i915]
<4> [271.812411] __i915_gem_set_wedged.part.4+0x55/0x190 [i915]
<4> [271.812425] ? _raw_spin_unlock_irqrestore+0x39/0x60
<4> [271.812469] i915_gem_set_wedged+0x56/0x60 [i915]
<4> [271.812511] i915_driver_unload+0x72/0x120 [i915]
<4> [271.812553] i915_pci_remove+0x19/0x30 [i915]
<4> [271.812565] pci_device_remove+0x36/0xb0
<4> [271.812574] device_release_driver_internal+0xdf/0x1d0
<4> [271.812584] driver_detach+0x3e/0x80
<4> [271.812593] bus_remove_driver+0x53/0xd0
<4> [271.812602] pci_unregister_driver+0x25/0xa0
<4> [271.812669] i915_exit+0x16/0x1c [i915]
where we try to wedge but the device is full of poison 0x6b. We are not
even in the fault-injection test, so that device should be after a
successful load.
Hmm, certainly didn't expect the latter.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind
2019-04-05 16:26 ` Chris Wilson
@ 2019-04-05 17:37 ` Chris Wilson
2019-04-08 8:19 ` Janusz Krzysztofik
0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2019-04-05 17:37 UTC (permalink / raw)
To: Janusz Krzysztofik, Patchwork; +Cc: intel-gfx
Quoting Chris Wilson (2019-04-05 17:26:46)
> Quoting Patchwork (2019-04-05 17:20:39)
> > == Series Details ==
> >
> > Series: Stop users from using the device on driver unbind
> > URL : https://patchwork.freedesktop.org/series/59064/
> > State : failure
> >
> > == Summary ==
> >
> > CI Bug Log - changes from CI_DRM_5881 -> Patchwork_12699
> > ====================================================
> >
> > Summary
> > -------
> >
> > **FAILURE**
> >
> > Serious unknown changes coming with Patchwork_12699 absolutely need to be
> > verified manually.
> >
> > If you think the reported changes have nothing to do with the changes
> > introduced in Patchwork_12699, please notify your bug team to allow them
> > to document this new failure mode, which will reduce false positives in CI.
> >
> > External URL: https://patchwork.freedesktop.org/api/1.0/series/59064/revisions/1/mbox/
> >
> > Possible new issues
> > -------------------
> >
> > Here are the unknown changes that may have been introduced in Patchwork_12699:
> >
> > ### IGT changes ###
> >
> > #### Possible regressions ####
> >
> > * igt@i915_module_load@reload:
>
> 2 issues, it appears:
>
> <4> [271.799080] WARN_ON(dev_priv->mm.object_count)
> <4> [271.799241] WARNING: CPU: 0 PID: 3288 at drivers/gpu/drm/i915/i915_gem.c:5145 i915_gem_cleanup_early+0x104/0x110 [i915]
> <4> [271.799249] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915(-) mei_hdcp x86_pkg_temp_thermal btusb coretemp btrtl btbcm btintel bluetooth crct10dif_pclmul crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e ecdh_generic snd_pcm mei_me ptp prime_numbers pps_core mei [last unloaded: snd_hda_intel]
> <4> [271.799302] CPU: 0 PID: 3288 Comm: i915_module_loa Tainted: G U 5.1.0-rc3-CI-Patchwork_12699+ #1
> <4> [271.799307] Hardware name: /NUC6i7KYB, BIOS KYSKLi70.86A.0059.2018.1122.1431 11/22/2018
> <4> [271.799406] RIP: 0010:i915_gem_cleanup_early+0x104/0x110 [i915]
> <4> [271.799412] Code: 00 00 48 c7 c2 d0 6b 3d a0 48 c7 c7 ca 5c 2c a0 e8 c1 b5 ec e0 0f 0b 48 c7 c6 68 c0 3f a0 48 c7 c7 63 88 42 a0 e8 9c 77 de e0 <0f> 0b e9 40 ff ff ff 0f 1f 44 00 00 e8 5b 7e 00 00 31 c0 c3 0f 1f
> <4> [271.799417] RSP: 0018:ffffc90000453dd0 EFLAGS: 00010282
> <4> [271.799423] RAX: 0000000000000000 RBX: ffff88849afd0000 RCX: 0000000000000000
> <4> [271.799428] RDX: 0000000000000006 RSI: ffff88849ee130b8 RDI: ffffffff8211dc4d
> <4> [271.799432] RBP: ffff88849afd7630 R08: 00000000028bc995 R09: 0000000000000000
> <4> [271.799436] R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa04a81e0
> <4> [271.799440] R13: 0000000000000000 R14: 0000000000000000 R15: ffffffffa04a82d0
> <4> [271.799446] FS: 00007f31e8cec980(0000) GS:ffff8884aee00000(0000) knlGS:0000000000000000
> <4> [271.799451] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [271.799455] CR2: 00007ffea58773d8 CR3: 000000044cfc6003 CR4: 00000000003606f0
> <4> [271.799459] Call Trace:
> <4> [271.799531] i915_driver_cleanup_early+0x30/0x70 [i915]
> <4> [271.799603] i915_driver_release+0xa/0x30 [i915]
> <4> [271.799672] i915_driver_unload+0x6a/0x120 [i915]
> <4> [271.799748] i915_pci_remove+0x19/0x30 [i915]
> <4> [271.799765] pci_device_remove+0x36/0xb0
So this is the bizarre part. We end up in the final i915_driver_release
because it appears that drm_dev_unplug() drops a reference. I couldn't
see where...
[ 24.960676] WARNING: CPU: 2 PID: 637 at drivers/gpu/drm/drm_drv.c:895 drm_dev_put+0x8/0x60
[ 24.960735] Modules linked in: nls_ascii nls_cp437 vfat fat crct10dif_pclmul crc32_pclmul crc32c_intel i915(-) aesni_intel aes_x86_64 crypto_simd cryptd glue_helper intel_cstate intel_uncore intel_rapl_perf efivars i2c_i801 intel_gtt drm_kms_helper ahci libahci video button efivarfs
[ 24.960848] CPU: 2 PID: 637 Comm: i915_module_loa Tainted: G BU 5.1.0-rc3+ #526
[ 24.960897] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017
[ 24.960952] RIP: 0010:drm_dev_put+0x8/0x60
[ 24.960993] Code: 48 8d 7b 60 e8 d9 8b c7 ff 48 8b 7b 60 5b 5d e9 0e 4f c7 ff 48 89 df e8 06 c2 ff ff e9 3f ff ff ff 90 48 85 ff 75 01 c3 55 53 <0f> 0b f0 ff 4f 14 0f 88 64 b7 2d 00 74 03 5b 5d c3 48 89 fb 48 8d
[ 24.961066] RSP: 0018:ffff88872587fc80 EFLAGS: 00010286
[ 24.961107] RAX: 0000000000000000 RBX: ffff88873f020000 RCX: ffffffff81680444
[ 24.961151] RDX: dffffc0000000000 RSI: dffffc0000000000 RDI: ffff88873f020000
[ 24.961195] RBP: ffff88873f02ad88 R08: 0000000000000000 R09: fffffbfff04824c5
[ 24.961240] R10: fffffbfff04824c5 R11: ffffffff8241262b R12: ffff88881ab067a0
[ 24.961284] R13: ffffffffa0618c00 R14: ffff88881ab06660 R15: ffff88881ab06960
[ 24.961330] FS: 00007fdba43279c0(0000) GS:ffff88881f500000(0000) knlGS:0000000000000000
[ 24.961377] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 24.961618] CR2: 00007fffc0229f80 CR3: 0000000726506001 CR4: 00000000001606e0
[ 24.961662] Call Trace:
[ 24.961773] i915_driver_unload+0x72/0x130 [i915]
[ 24.961888] i915_pci_remove+0x2a/0x50 [i915]
[ 24.961929] pci_device_remove+0xaa/0x180
[ 24.961968] ? pcibios_free_irq+0x10/0x10
[ 24.962005] ? up_read+0xc2/0xe0
[ 24.962041] device_release_driver_internal+0x12b/0x260
[ 24.962081] driver_detach+0x6f/0xca
[ 24.962117] bus_remove_driver+0xc4/0x141
[ 24.962157] pci_unregister_driver+0x32/0xf0
[ 24.962274] i915_exit+0x16/0x1c [i915]
[ 24.962312] __x64_sys_delete_module+0x20e/0x2b0
[ 24.962351] ? __ia32_sys_delete_module+0x2b0/0x2b0
[ 24.962390] ? lockdep_hardirqs_on+0x11/0x250
[ 24.962428] ? lockdep_hardirqs_off+0x1a/0x100
[ 24.962465] ? trace_hardirqs_off_thunk+0x1a/0x1c
[ 24.962504] do_syscall_64+0x5d/0x200
[ 24.962542] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 24.962581] RIP: 0033:0x7fdba6189137
which gdb insists is the drm_dev_unplug() call. Oh drm-tip, not dinq.
Noralf has been playing.
We have
commit ba3bf37e150a99b51b13f5cebf89715685d21212
Author: Noralf Trønnes <noralf@tronnes.org>
Date: Fri Feb 8 15:01:03 2019 +0100
drm/drv: drm_dev_unplug(): Move out drm_dev_put() call
This makes it possible to use drm_dev_unplug() with the upcoming
devm_drm_dev_init() which will do drm_dev_put() in its release callback.
but drm-tip has a mismash of trees and a conflict that brings the
drm_dev_put() here right back in.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind
2019-04-05 17:37 ` Chris Wilson
@ 2019-04-08 8:19 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-08 8:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Friday, April 5, 2019 7:37:04 PM CEST Chris Wilson wrote:
> Quoting Chris Wilson (2019-04-05 17:26:46)
>
> > Quoting Patchwork (2019-04-05 17:20:39)
> >
> > > == Series Details ==
> > >
> > > Series: Stop users from using the device on driver unbind
> > > URL : https://patchwork.freedesktop.org/series/59064/
> > > State : failure
> > >
> > > == Summary ==
> > >
> > > CI Bug Log - changes from CI_DRM_5881 -> Patchwork_12699
> > > ====================================================
> > >
> > > Summary
> > > -------
> > >
> > > **FAILURE**
> > >
> > > Serious unknown changes coming with Patchwork_12699 absolutely need to
> > > be
> > > verified manually.
> > >
> > > If you think the reported changes have nothing to do with the changes
> > > introduced in Patchwork_12699, please notify your bug team to allow
> > > them
> > > to document this new failure mode, which will reduce false positives
> > > in CI.
> > >
> > > External URL:
> > > https://patchwork.freedesktop.org/api/1.0/series/59064/revisions/1/mb
> > > ox/> >
> > > Possible new issues
> > > -------------------
> > >
> > > Here are the unknown changes that may have been introduced in
Patchwork_12699:
> > > ### IGT changes ###
> > >
> > > #### Possible regressions ####
> > >
> > > * igt@i915_module_load@reload:
> > 2 issues, it appears:
> >
> > <4> [271.799080] WARN_ON(dev_priv->mm.object_count)
> > <4> [271.799241] WARNING: CPU: 0 PID: 3288 at
> > drivers/gpu/drm/i915/i915_gem.c:5145 i915_gem_cleanup_early+0x104/0x110
> > [i915] <4> [271.799249] Modules linked in: vgem snd_hda_codec_hdmi
> > snd_hda_codec_realtek snd_hda_codec_generic i915(-) mei_hdcp
> > x86_pkg_temp_thermal btusb coretemp btrtl btbcm btintel bluetooth
> > crct10dif_pclmul crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel
> > snd_hda_core e1000e ecdh_generic snd_pcm mei_me ptp prime_numbers
> > pps_core mei [last unloaded: snd_hda_intel] <4> [271.799302] CPU: 0 PID:
> > 3288 Comm: i915_module_loa Tainted: G U
> > 5.1.0-rc3-CI-Patchwork_12699+ #1 <4> [271.799307] Hardware name:
> > /NUC6i7KYB, BIOS KYSKLi70.86A.0059.2018.1122.1431 11/22/2018 <4>
> > [271.799406] RIP: 0010:i915_gem_cleanup_early+0x104/0x110 [i915] <4>
> > [271.799412] Code: 00 00 48 c7 c2 d0 6b 3d a0 48 c7 c7 ca 5c 2c a0 e8 c1
> > b5 ec e0 0f 0b 48 c7 c6 68 c0 3f a0 48 c7 c7 63 88 42 a0 e8 9c 77 de e0
> > <0f> 0b e9 40 ff ff ff 0f 1f 44 00 00 e8 5b 7e 00 00 31 c0 c3 0f 1f <4>
> > [271.799417] RSP: 0018:ffffc90000453dd0 EFLAGS: 00010282
> > <4> [271.799423] RAX: 0000000000000000 RBX: ffff88849afd0000 RCX:
> > 0000000000000000 <4> [271.799428] RDX: 0000000000000006 RSI:
> > ffff88849ee130b8 RDI: ffffffff8211dc4d <4> [271.799432] RBP:
> > ffff88849afd7630 R08: 00000000028bc995 R09: 0000000000000000 <4>
> > [271.799436] R10: 0000000000000000 R11: 0000000000000000 R12:
> > ffffffffa04a81e0 <4> [271.799440] R13: 0000000000000000 R14:
> > 0000000000000000 R15: ffffffffa04a82d0 <4> [271.799446] FS:
> > 00007f31e8cec980(0000) GS:ffff8884aee00000(0000) knlGS:0000000000000000
> > <4> [271.799451] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4>
> > [271.799455] CR2: 00007ffea58773d8 CR3: 000000044cfc6003 CR4:
> > 00000000003606f0 <4> [271.799459] Call Trace:
> > <4> [271.799531] i915_driver_cleanup_early+0x30/0x70 [i915]
> > <4> [271.799603] i915_driver_release+0xa/0x30 [i915]
> > <4> [271.799672] i915_driver_unload+0x6a/0x120 [i915]
> > <4> [271.799748] i915_pci_remove+0x19/0x30 [i915]
> > <4> [271.799765] pci_device_remove+0x36/0xb0
>
> So this is the bizarre part. We end up in the final i915_driver_release
> because it appears that drm_dev_unplug() drops a reference. I couldn't
> see where...
>
> [ 24.960676] WARNING: CPU: 2 PID: 637 at drivers/gpu/drm/drm_drv.c:895
> drm_dev_put+0x8/0x60 [ 24.960735] Modules linked in: nls_ascii nls_cp437
> vfat fat crct10dif_pclmul crc32_pclmul crc32c_intel i915(-) aesni_intel
> aes_x86_64 crypto_simd cryptd glue_helper intel_cstate intel_uncore
> intel_rapl_perf efivars i2c_i801 intel_gtt drm_kms_helper ahci libahci
> video button efivarfs [ 24.960848] CPU: 2 PID: 637 Comm: i915_module_loa
> Tainted: G BU 5.1.0-rc3+ #526 [ 24.960897] Hardware name:
> Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS
> BNKBL357.86A.0052.2017.0918.1346 09/18/2017 [ 24.960952] RIP:
> 0010:drm_dev_put+0x8/0x60
> [ 24.960993] Code: 48 8d 7b 60 e8 d9 8b c7 ff 48 8b 7b 60 5b 5d e9 0e 4f
> c7 ff 48 89 df e8 06 c2 ff ff e9 3f ff ff ff 90 48 85 ff 75 01 c3 55 53
> <0f> 0b f0 ff 4f 14 0f 88 64 b7 2d 00 74 03 5b 5d c3 48 89 fb 48 8d [
> 24.961066] RSP: 0018:ffff88872587fc80 EFLAGS: 00010286
> [ 24.961107] RAX: 0000000000000000 RBX: ffff88873f020000 RCX:
> ffffffff81680444 [ 24.961151] RDX: dffffc0000000000 RSI: dffffc0000000000
> RDI: ffff88873f020000 [ 24.961195] RBP: ffff88873f02ad88 R08:
> 0000000000000000 R09: fffffbfff04824c5 [ 24.961240] R10: fffffbfff04824c5
> R11: ffffffff8241262b R12: ffff88881ab067a0 [ 24.961284] R13:
> ffffffffa0618c00 R14: ffff88881ab06660 R15: ffff88881ab06960 [ 24.961330]
> FS: 00007fdba43279c0(0000) GS:ffff88881f500000(0000)
> knlGS:0000000000000000 [ 24.961377] CS: 0010 DS: 0000 ES: 0000 CR0:
> 0000000080050033
> [ 24.961618] CR2: 00007fffc0229f80 CR3: 0000000726506001 CR4:
> 00000000001606e0 [ 24.961662] Call Trace:
> [ 24.961773] i915_driver_unload+0x72/0x130 [i915]
> [ 24.961888] i915_pci_remove+0x2a/0x50 [i915]
> [ 24.961929] pci_device_remove+0xaa/0x180
> [ 24.961968] ? pcibios_free_irq+0x10/0x10
> [ 24.962005] ? up_read+0xc2/0xe0
> [ 24.962041] device_release_driver_internal+0x12b/0x260
> [ 24.962081] driver_detach+0x6f/0xca
> [ 24.962117] bus_remove_driver+0xc4/0x141
> [ 24.962157] pci_unregister_driver+0x32/0xf0
> [ 24.962274] i915_exit+0x16/0x1c [i915]
> [ 24.962312] __x64_sys_delete_module+0x20e/0x2b0
> [ 24.962351] ? __ia32_sys_delete_module+0x2b0/0x2b0
> [ 24.962390] ? lockdep_hardirqs_on+0x11/0x250
> [ 24.962428] ? lockdep_hardirqs_off+0x1a/0x100
> [ 24.962465] ? trace_hardirqs_off_thunk+0x1a/0x1c
> [ 24.962504] do_syscall_64+0x5d/0x200
> [ 24.962542] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 24.962581] RIP: 0033:0x7fdba6189137
>
> which gdb insists is the drm_dev_unplug() call. Oh drm-tip, not dinq.
> Noralf has been playing.
>
> We have
>
> commit ba3bf37e150a99b51b13f5cebf89715685d21212
> Author: Noralf Trønnes <noralf@tronnes.org>
> Date: Fri Feb 8 15:01:03 2019 +0100
>
> drm/drv: drm_dev_unplug(): Move out drm_dev_put() call
>
> This makes it possible to use drm_dev_unplug() with the upcoming
> devm_drm_dev_init() which will do drm_dev_put() in its release callback.
>
> but drm-tip has a mismash of trees and a conflict that brings the
> drm_dev_put() here right back in.
Yeah, I can see the "drm: Fix drm_release() and device unplug" patch,
which is patch 1/2 of the series that introduced commit ba3bf37e150a
("drm/drv: drm_dev_unplug(): Move out drm_dev_put() call"), is applied
twice in drm-tip, one instance coming from drm-fixes as
commit 3f04e0a6cfeb, the other from drm-next as 1ee57d4d75fb.
(there were two). Playing with it, I learned by excercise it was
certainly too early for us to drop the reference at that point. I took
the opportunity that drm_dev_put() was finally moved out of
drm_dev_unplug() so we could use it in place of drm_dev_unregister().
Janusz
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
2019-04-05 7:41 ` Chris Wilson
@ 2019-04-15 9:29 ` Daniel Vetter
2019-04-15 9:29 ` Daniel Vetter
1 sibling, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2019-04-15 9:29 UTC (permalink / raw)
To: Chris Wilson
Cc: Jani Nikula, Janusz Krzysztofik, Joonas Lahtinen, Rodrigo Vivi,
David Airlie, Daniel Vetter, michal.wajdeczko, intel-gfx,
dri-devel, linux-kernel, Janusz Krzysztofik
On Fri, Apr 05, 2019 at 08:41:16AM +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > From: Janusz Krzysztofik <janusz.krzysztofik@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@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.
I think we still need the above patch, since drm_dev_unplug ==
drm_dev_unregister + "make sure userspace can't get at us anymore". We
could/should probably drop drm_dev_unplug and move that additional code to
drm_dev_unregister, but there's some minutea in how we refcount the
drm_device between the two. So not quite as clean a job.
There's also drm_put_dev (not to be mistaken with drm_dev_put), for added
confusion. I think ideally we'd unify all three of drm_dev_unregister,
drm_dev_unplug and drm_put_dev to one, deprecating all the others. But
that's work :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/i915: Use drm_dev_unplug()
@ 2019-04-15 9:29 ` Daniel Vetter
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2019-04-15 9:29 UTC (permalink / raw)
To: Chris Wilson
Cc: Janusz Krzysztofik, David Airlie, intel-gfx, linux-kernel, dri-devel
On Fri, Apr 05, 2019 at 08:41:16AM +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-04-05 08:26:57)
> > From: Janusz Krzysztofik <janusz.krzysztofik@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@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.
I think we still need the above patch, since drm_dev_unplug ==
drm_dev_unregister + "make sure userspace can't get at us anymore". We
could/should probably drop drm_dev_unplug and move that additional code to
drm_dev_unregister, but there's some minutea in how we refcount the
drm_device between the two. So not quite as clean a job.
There's also drm_put_dev (not to be mistaken with drm_dev_put), for added
confusion. I think ideally we'd unify all three of drm_dev_unregister,
drm_dev_unplug and drm_put_dev to one, deprecating all the others. But
that's work :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: Use drm_dev_unplug()
2019-04-05 13:02 ` Janusz Krzysztofik
(?)
@ 2019-04-15 9:32 ` Daniel Vetter
2019-04-18 14:51 ` [PATCH v2 0/1] Stop users from using the device on driver unbind Janusz Krzysztofik
-1 siblings, 1 reply; 22+ messages in thread
From: Daniel Vetter @ 2019-04-15 9:32 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Joonas Lahtinen, Jani Nikula, Rodrigo Vivi, David Airlie,
Daniel Vetter, Chris Wilson, michal.wajdeczko, intel-gfx,
dri-devel, linux-kernel, Janusz Krzysztofik
On Fri, Apr 05, 2019 at 03:02:34PM +0200, Janusz Krzysztofik wrote:
> From: Janusz Krzysztofik <janusz.krzysztofik@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. Once done, also cancel inflight operations immediately by
> calling i915_gem_set_wedged().
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> 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);
>
> i915_gem_shrinker_unregister(dev_priv);
> }
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 0/1] Stop users from using the device on driver unbind
2019-04-15 9:32 ` Daniel Vetter
@ 2019-04-18 14:51 ` Janusz Krzysztofik
2019-04-18 14:51 ` [PATCH v2 1/1] drm/i915: Use drm_dev_unplug() Janusz Krzysztofik
0 siblings, 1 reply; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-18 14:51 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, janusz.krzysztofik,
janusz.krzysztofik
Use drm_dev_unplug() to have device resources protected from user access
by DRM layer as soon as the driver is going to be unbound.
Janusz Krzysztofik (1):
drm/i915: Use drm_dev_unplug()
drivers/gpu/drm/i915/i915_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Since this patch should be now safe for use if merged with current
drm-next or drm-tip branch which no longer suffer from incorrectly
resolved merge confilct that was breaking it, finally fixed by commit
bd53280ef042 ("drm/drv: Fix incorrect resolution of merge conflict"),
I'm resending it with Daniel's Reviewed-by: added.
Former patch 2/2 has been dropped as it is already in drm-intel-next as
commit 141f3767e7b8 ("drm/i915: Mark GEM wedged right after marking
device unplugged"). BTW, the wersion I sent was screwed up, not
reflecting Chris' intention precisely enough, but Chris was vigilant and
fixed it. Sorry Chris.
Thanks,
Janusz
--
2.20.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/1] drm/i915: Use drm_dev_unplug()
2019-04-18 14:51 ` [PATCH v2 0/1] Stop users from using the device on driver unbind Janusz Krzysztofik
@ 2019-04-18 14:51 ` Janusz Krzysztofik
0 siblings, 0 replies; 22+ messages in thread
From: Janusz Krzysztofik @ 2019-04-18 14:51 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
Cc: David Airlie, Daniel Vetter, Chris Wilson, michal.wajdeczko,
intel-gfx, dri-devel, linux-kernel, janusz.krzysztofik,
janusz.krzysztofik, Daniel Vetter
From: Janusz Krzysztofik <janusz.krzysztofik@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@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
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);
i915_gem_shrinker_unregister(dev_priv);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-04-18 14:51 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 13:02 [PATCH 0/2] Stop users from using the device on driver unbind Janusz Krzysztofik
2019-04-05 13:02 ` [PATCH 1/2] drm/i915: Use drm_dev_unplug() Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-15 9:32 ` Daniel Vetter
2019-04-18 14:51 ` [PATCH v2 0/1] Stop users from using the device on driver unbind Janusz Krzysztofik
2019-04-18 14:51 ` [PATCH v2 1/1] drm/i915: Use drm_dev_unplug() Janusz Krzysztofik
2019-04-05 13:02 ` [PATCH 2/2] drm/i915: Mark GEM wedged right after marking device unplugged Janusz Krzysztofik
2019-04-05 13:02 ` Janusz Krzysztofik
2019-04-05 16:20 ` ✗ Fi.CI.BAT: failure for Stop users from using the device on driver unbind Patchwork
2019-04-05 16:26 ` Chris Wilson
2019-04-05 17:37 ` Chris Wilson
2019-04-08 8:19 ` Janusz Krzysztofik
-- strict thread matches above, loose matches on Subject: below --
2019-04-05 7:26 [PATCH] drm/i915: Use drm_dev_unplug() Janusz Krzysztofik
2019-04-05 7:26 ` Janusz Krzysztofik
2019-04-05 7:41 ` Chris Wilson
2019-04-05 8:11 ` Janusz Krzysztofik
2019-04-05 8:24 ` Chris Wilson
2019-04-05 8:24 ` Chris Wilson
2019-04-05 11:38 ` Janusz Krzysztofik
2019-04-15 9:29 ` Daniel Vetter
2019-04-15 9:29 ` Daniel Vetter
2019-04-05 8:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.