drm/i915: Protect fbdev across slow or failed initialisation

Submitted by Chris Wilson on Feb. 3, 2016, 9:17 a.m.

Details

Message ID 1454491057-16258-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show
Series "drm/i915: Protect fbdev across slow or failed initialisation" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson Feb. 3, 2016, 9:17 a.m.
If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 09840f4380f9..6218bc5370a1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -114,6 +114,20 @@  static struct fb_ops intelfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
+static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
+{
+	struct fb_info *info;
+
+	if (ifbdev == NULL)
+		return false;
+
+	info = ifbdev->helper.fbdev;
+	if (!info->screen_base)
+		return false;
+
+	return info->state == FBINFO_STATE_RUNNING;
+}
+
 static int intelfb_alloc(struct drm_fb_helper *helper,
 			 struct drm_fb_helper_surface_size *sizes)
 {
@@ -753,6 +767,8 @@  void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 		return;
 
 	info = ifbdev->helper.fbdev;
+	if (!info->screen_base)
+		return;
 
 	if (synchronous) {
 		/* Flush any pending work to turn the console on, and then
@@ -794,29 +810,24 @@  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 = dev->dev_private;
-	if (dev_priv->fbdev)
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (intel_fbdev_active(dev_priv->fbdev))
 		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
 }
 
 void intel_fbdev_restore_mode(struct drm_device *dev)
 {
-	int ret;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_fbdev *ifbdev = dev_priv->fbdev;
-	struct drm_fb_helper *fb_helper;
+	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
 
-	if (!ifbdev)
+	if (!intel_fbdev_active(ifbdev))
 		return;
 
-	fb_helper = &ifbdev->helper;
-
-	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
-	if (ret) {
-		DRM_DEBUG("failed to restore crtc mode\n");
-	} else {
-		mutex_lock(&fb_helper->dev->struct_mutex);
+	if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
+		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);
+	} else {
+		DRM_DEBUG("failed to restore crtc mode\n");
 	}
 }

Comments

Hi,

On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure.

This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails").

Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
interesting to know if it can be reproduced at all with 4.5-rc2.

Best regards,

Lukas

> Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4380f9..6218bc5370a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
>  	.fb_debug_leave = drm_fb_helper_debug_leave,
>  };
>  
> +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> +{
> +	struct fb_info *info;
> +
> +	if (ifbdev == NULL)
> +		return false;
> +
> +	info = ifbdev->helper.fbdev;
> +	if (!info->screen_base)
> +		return false;
> +
> +	return info->state == FBINFO_STATE_RUNNING;
> +}
> +
>  static int intelfb_alloc(struct drm_fb_helper *helper,
>  			 struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>  		return;
>  
>  	info = ifbdev->helper.fbdev;
> +	if (!info->screen_base)
> +		return;
>  
>  	if (synchronous) {
>  		/* Flush any pending work to turn the console on, and then
> @@ -794,29 +810,24 @@ 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 = dev->dev_private;
> -	if (dev_priv->fbdev)
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (intel_fbdev_active(dev_priv->fbdev))
>  		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>  }
>  
>  void intel_fbdev_restore_mode(struct drm_device *dev)
>  {
> -	int ret;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> -	struct drm_fb_helper *fb_helper;
> +	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>  
> -	if (!ifbdev)
> +	if (!intel_fbdev_active(ifbdev))
>  		return;
>  
> -	fb_helper = &ifbdev->helper;
> -
> -	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> -	if (ret) {
> -		DRM_DEBUG("failed to restore crtc mode\n");
> -	} else {
> -		mutex_lock(&fb_helper->dev->struct_mutex);
> +	if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> +		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);
> +	} else {
> +		DRM_DEBUG("failed to restore crtc mode\n");
>  	}
>  }
> -- 
> 2.7.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Cool, thank you.
I dont believe I can easily reproduce it, it has only happend few times
(and i reboot my lappy >2 times per day).

//
Gustav

2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de>:

> Hi,
>
> On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> > If the initialisation fails, we may be left with a dangling pointer with
> > an incomplete fbdev structure.
>
> This shouldn't happen with 4.5, the fbdev is now clobbered if
> initialization
> fails, the existing "if (dev_priv->fbdev)" checks should thus be
> sufficient.
> See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails").
>
> Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
> interesting to know if it can be reproduced at all with 4.5-rc2.
>
> Best regards,
>
> Lukas
>
> > Here we want to disable internal calls
> > into fbdev. Similarly, the initialisation may be slow and we haven't yet
> > enabled the fbdev (e.g. quick suspend or last-close before the async init
> > completes).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 41
> ++++++++++++++++++++++++--------------
> >  1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 09840f4380f9..6218bc5370a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> >       .fb_debug_leave = drm_fb_helper_debug_leave,
> >  };
> >
> > +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> > +{
> > +     struct fb_info *info;
> > +
> > +     if (ifbdev == NULL)
> > +             return false;
> > +
> > +     info = ifbdev->helper.fbdev;
> > +     if (!info->screen_base)
> > +             return false;
> > +
> > +     return info->state == FBINFO_STATE_RUNNING;
> > +}
> > +
> >  static int intelfb_alloc(struct drm_fb_helper *helper,
> >                        struct drm_fb_helper_surface_size *sizes)
> >  {
> > @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev,
> int state, bool synchronous
> >               return;
> >
> >       info = ifbdev->helper.fbdev;
> > +     if (!info->screen_base)
> > +             return;
> >
> >       if (synchronous) {
> >               /* Flush any pending work to turn the console on, and then
> > @@ -794,29 +810,24 @@ 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 = dev->dev_private;
> > -     if (dev_priv->fbdev)
> > +     struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +     if (intel_fbdev_active(dev_priv->fbdev))
> >               drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >  }
> >
> >  void intel_fbdev_restore_mode(struct drm_device *dev)
> >  {
> > -     int ret;
> > -     struct drm_i915_private *dev_priv = dev->dev_private;
> > -     struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > -     struct drm_fb_helper *fb_helper;
> > +     struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> >
> > -     if (!ifbdev)
> > +     if (!intel_fbdev_active(ifbdev))
> >               return;
> >
> > -     fb_helper = &ifbdev->helper;
> > -
> > -     ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> > -     if (ret) {
> > -             DRM_DEBUG("failed to restore crtc mode\n");
> > -     } else {
> > -             mutex_lock(&fb_helper->dev->struct_mutex);
> > +     if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) ==
> 0) {
> > +             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);
> > +     } else {
> > +             DRM_DEBUG("failed to restore crtc mode\n");
> >       }
> >  }
> > --
> > 2.7.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Thanks for your quick response.
Yes it is not easily be reproduced in native. In  iVGT we startup several VMs  simultaneously, it can be reproduced in several cycles, upon 1/10 fail rate.
Need to use GUI mode but not text mode to reproduce this issue.

BRs,
Weinan Li


From: Gustav F├Ągerlind [mailto:gustav.fagerlind@gmail.com]

Sent: Thursday, February 04, 2016 1:08 AM
To: Lukas Wunner
Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Li, Weinan Z
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation

Cool, thank you.
I dont believe I can easily reproduce it, it has only happend few times (and i reboot my lappy >2 times per day).

//
Gustav

2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de<mailto:lukas@wunner.de>>:
Hi,

On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with

> an incomplete fbdev structure.


This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
initialization fails").

Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
interesting to know if it can be reproduced at all with 4.5-rc2.

Best regards,

Lukas

> Here we want to disable internal calls

> into fbdev. Similarly, the initialisation may be slow and we haven't yet

> enabled the fbdev (e.g. quick suspend or last-close before the async init

> completes).

>

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580

> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com<mailto:weinan.z.li@intel.com>>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>>

> ---

>  drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------

>  1 file changed, 26 insertions(+), 15 deletions(-)

>

> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c

> index 09840f4380f9..6218bc5370a1 100644

> --- a/drivers/gpu/drm/i915/intel_fbdev.c

> +++ b/drivers/gpu/drm/i915/intel_fbdev.c

> @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {

>       .fb_debug_leave = drm_fb_helper_debug_leave,

>  };

>

> +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)

> +{

> +     struct fb_info *info;

> +

> +     if (ifbdev == NULL)

> +             return false;

> +

> +     info = ifbdev->helper.fbdev;

> +     if (!info->screen_base)

> +             return false;

> +

> +     return info->state == FBINFO_STATE_RUNNING;

> +}

> +

>  static int intelfb_alloc(struct drm_fb_helper *helper,

>                        struct drm_fb_helper_surface_size *sizes)

>  {

> @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous

>               return;

>

>       info = ifbdev->helper.fbdev;

> +     if (!info->screen_base)

> +             return;

>

>       if (synchronous) {

>               /* Flush any pending work to turn the console on, and then

> @@ -794,29 +810,24 @@ 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 = dev->dev_private;

> -     if (dev_priv->fbdev)

> +     struct drm_i915_private *dev_priv = to_i915(dev);

> +

> +     if (intel_fbdev_active(dev_priv->fbdev))

>               drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);

>  }

>

>  void intel_fbdev_restore_mode(struct drm_device *dev)

>  {

> -     int ret;

> -     struct drm_i915_private *dev_priv = dev->dev_private;

> -     struct intel_fbdev *ifbdev = dev_priv->fbdev;

> -     struct drm_fb_helper *fb_helper;

> +     struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;

>

> -     if (!ifbdev)

> +     if (!intel_fbdev_active(ifbdev))

>               return;

>

> -     fb_helper = &ifbdev->helper;

> -

> -     ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);

> -     if (ret) {

> -             DRM_DEBUG("failed to restore crtc mode\n");

> -     } else {

> -             mutex_lock(&fb_helper->dev->struct_mutex);

> +     if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {

> +             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);

> +     } else {

> +             DRM_DEBUG("failed to restore crtc mode\n");

>       }

>  }

> --

> 2.7.0

>

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org>

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx