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

[Daniel Vetter <daniel@ffwll.ch>]

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