* [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
@ 2016-07-13 17:34 Chris Wilson
2016-07-13 17:34 ` [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Chris Wilson @ 2016-07-13 17:34 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Mika Kuoppala
Since the suspend_work can arm itself if the console_lock() is currently
held elsewhere, simply calling flush_work() doesn't guarantee that the
work is idle upon return. To do so requires using cancel_work_sync().
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 86b00c6db1a6..ef17d88a1bc7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -768,7 +768,7 @@ void intel_fbdev_fini(struct drm_device *dev)
if (!ifbdev)
return;
- flush_work(&dev_priv->fbdev_suspend_work);
+ cancel_work_sync(&dev_priv->fbdev_suspend_work);
if (!current_is_async())
intel_fbdev_sync(ifbdev);
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use
2016-07-13 17:34 [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring Chris Wilson
@ 2016-07-13 17:34 ` Chris Wilson
2016-07-14 14:49 ` Daniel Vetter
2016-07-14 5:29 ` ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring Patchwork
2016-07-14 10:47 ` [PATCH 1/2] " Mika Kuoppala
2 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2016-07-13 17:34 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Mika Kuoppala
If the fbdev probing fails, and in our error path we fail to clear the
dev_priv->fbdev, then we can try and use a dangling fbdev pointer, and
in particular a NULL fb. This could also happen in pathological cases
where we try to operate on the fbdev prior to it being probed.
Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/intel_fbdev.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index ef17d88a1bc7..23129dcfba9d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -782,7 +782,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct fb_info *info;
- if (!ifbdev)
+ if (!ifbdev || !ifbdev->fb)
return;
info = ifbdev->helper.fbdev;
@@ -827,31 +827,28 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
void intel_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = to_i915(dev);
- if (dev_priv->fbdev)
- drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+ struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
+
+ if (ifbdev && ifbdev->fb)
+ drm_fb_helper_hotplug_event(&ifbdev->helper);
}
void intel_fbdev_restore_mode(struct drm_device *dev)
{
- int ret;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_fbdev *ifbdev = dev_priv->fbdev;
- struct drm_fb_helper *fb_helper;
+ struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
if (!ifbdev)
return;
intel_fbdev_sync(ifbdev);
+ if (!ifbdev->fb)
+ return;
- fb_helper = &ifbdev->helper;
-
- ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
- if (ret) {
+ if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper)) {
DRM_DEBUG("failed to restore crtc mode\n");
} else {
- mutex_lock(&fb_helper->dev->struct_mutex);
+ mutex_lock(&dev->struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
- mutex_unlock(&fb_helper->dev->struct_mutex);
+ mutex_unlock(&dev->struct_mutex);
}
}
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring
2016-07-13 17:34 [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring Chris Wilson
2016-07-13 17:34 ` [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use Chris Wilson
@ 2016-07-14 5:29 ` Patchwork
2016-07-14 10:47 ` [PATCH 1/2] " Mika Kuoppala
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2016-07-14 5:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring
URL : https://patchwork.freedesktop.org/series/9826/
State : success
== Summary ==
Series 9826v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9826/revisions/1/mbox
fi-kbl-qkkr total:237 pass:174 dwarn:25 dfail:4 fail:7 skip:27
fi-skl-i5-6260u total:237 pass:189 dwarn:27 dfail:2 fail:7 skip:12
fi-skl-i7-6700k total:237 pass:200 dwarn:2 dfail:0 fail:9 skip:26
fi-snb-i7-2600 total:237 pass:188 dwarn:0 dfail:0 fail:9 skip:40
ro-bdw-i5-5250u total:237 pass:210 dwarn:2 dfail:0 fail:9 skip:16
ro-bdw-i7-5557U total:237 pass:210 dwarn:3 dfail:0 fail:9 skip:15
ro-bdw-i7-5600u total:237 pass:195 dwarn:2 dfail:0 fail:9 skip:31
ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42
ro-byt-n2820 total:237 pass:177 dwarn:0 dfail:0 fail:7 skip:38
ro-hsw-i3-4010u total:237 pass:199 dwarn:5 dfail:2 fail:7 skip:24
ro-hsw-i7-4770r total:237 pass:204 dwarn:0 dfail:0 fail:9 skip:24
ro-ilk-i7-620lm total:237 pass:166 dwarn:0 dfail:0 fail:8 skip:63
ro-ilk1-i5-650 total:232 pass:166 dwarn:0 dfail:0 fail:8 skip:58
ro-ivb-i7-3770 total:237 pass:197 dwarn:0 dfail:0 fail:7 skip:33
ro-skl3-i5-6260u total:237 pass:213 dwarn:3 dfail:0 fail:9 skip:12
ro-snb-i7-2620M total:237 pass:187 dwarn:0 dfail:0 fail:9 skip:41
Results at /archive/results/CI_IGT_test/RO_Patchwork_1488/
5cd2699 drm-intel-nightly: 2016y-07m-13d-14h-43m-27s UTC integration manifest
27b9caa drm/i915/fbdev: Check for the framebuffer before use
1309c34 drm/i915/fbdev: Drain the suspend worker on retiring
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
2016-07-13 17:34 [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring Chris Wilson
2016-07-13 17:34 ` [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use Chris Wilson
2016-07-14 5:29 ` ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring Patchwork
@ 2016-07-14 10:47 ` Mika Kuoppala
2016-07-14 14:45 ` Daniel Vetter
2 siblings, 1 reply; 7+ messages in thread
From: Mika Kuoppala @ 2016-07-14 10:47 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Since the suspend_work can arm itself if the console_lock() is currently
> held elsewhere, simply calling flush_work() doesn't guarantee that the
> work is idle upon return. To do so requires using cancel_work_sync().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 86b00c6db1a6..ef17d88a1bc7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -768,7 +768,7 @@ void intel_fbdev_fini(struct drm_device *dev)
> if (!ifbdev)
> return;
>
> - flush_work(&dev_priv->fbdev_suspend_work);
> + cancel_work_sync(&dev_priv->fbdev_suspend_work);
> if (!current_is_async())
> intel_fbdev_sync(ifbdev);
>
> --
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
2016-07-14 10:47 ` [PATCH 1/2] " Mika Kuoppala
@ 2016-07-14 14:45 ` Daniel Vetter
2016-07-14 14:55 ` Chris Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2016-07-14 14:45 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: Daniel Vetter, intel-gfx
On Thu, Jul 14, 2016 at 01:47:49PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Since the suspend_work can arm itself if the console_lock() is currently
> > held elsewhere, simply calling flush_work() doesn't guarantee that the
> > work is idle upon return. To do so requires using cancel_work_sync().
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: stable@vger.kernel.org I presume? Checking for this should be part of
the review ...
-Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 86b00c6db1a6..ef17d88a1bc7 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -768,7 +768,7 @@ void intel_fbdev_fini(struct drm_device *dev)
> > if (!ifbdev)
> > return;
> >
> > - flush_work(&dev_priv->fbdev_suspend_work);
> > + cancel_work_sync(&dev_priv->fbdev_suspend_work);
> > if (!current_is_async())
> > intel_fbdev_sync(ifbdev);
> >
> > --
> > 2.8.1
--
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] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use
2016-07-13 17:34 ` [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use Chris Wilson
@ 2016-07-14 14:49 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2016-07-14 14:49 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, thierry.reding, Mika Kuoppala
On Wed, Jul 13, 2016 at 06:34:45PM +0100, Chris Wilson wrote:
> If the fbdev probing fails, and in our error path we fail to clear the
> dev_priv->fbdev, then we can try and use a dangling fbdev pointer, and
> in particular a NULL fb. This could also happen in pathological cases
> where we try to operate on the fbdev prior to it being probed.
>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Thierry Redding has a patch series in flight for generic delayed fbdev
setup, whether it's delayed to due async init or because there's nothing
yet connected (which some of the embedded drivers hack together). With
those patches most of these checks should become unecessary again.
Adding Thierry so he's aware that there's more to do when rebasing, and so
he knows where he can find reviewers ;-)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index ef17d88a1bc7..23129dcfba9d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -782,7 +782,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> struct intel_fbdev *ifbdev = dev_priv->fbdev;
> struct fb_info *info;
>
> - if (!ifbdev)
> + if (!ifbdev || !ifbdev->fb)
> return;
>
> info = ifbdev->helper.fbdev;
> @@ -827,31 +827,28 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> +
> + if (ifbdev && ifbdev->fb)
> + drm_fb_helper_hotplug_event(&ifbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>
> if (!ifbdev)
> return;
>
> intel_fbdev_sync(ifbdev);
> + if (!ifbdev->fb)
> + return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper)) {
> DRM_DEBUG("failed to restore crtc mode\n");
> } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> }
> }
> --
> 2.8.1
>
--
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] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
2016-07-14 14:45 ` Daniel Vetter
@ 2016-07-14 14:55 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2016-07-14 14:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Daniel Vetter
On Thu, Jul 14, 2016 at 04:45:52PM +0200, Daniel Vetter wrote:
> On Thu, Jul 14, 2016 at 01:47:49PM +0300, Mika Kuoppala wrote:
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> >
> > > Since the suspend_work can arm itself if the console_lock() is currently
> > > held elsewhere, simply calling flush_work() doesn't guarantee that the
> > > work is idle upon return. To do so requires using cancel_work_sync().
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Cc: stable@vger.kernel.org I presume? Checking for this should be part of
> the review ...
No known bug - unlikely since ~nobody unloads the module.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-14 14:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-13 17:34 [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring Chris Wilson
2016-07-13 17:34 ` [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use Chris Wilson
2016-07-14 14:49 ` Daniel Vetter
2016-07-14 5:29 ` ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring Patchwork
2016-07-14 10:47 ` [PATCH 1/2] " Mika Kuoppala
2016-07-14 14:45 ` Daniel Vetter
2016-07-14 14:55 ` Chris Wilson
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.