[PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[Chris Wilson]

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

[PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use

[Daniel Vetter]

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

✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[Patchwork]

== 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

Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[Mika Kuoppala]

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

Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[Daniel Vetter]

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

Re: [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring

[Chris Wilson]

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