Re: [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> 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).
> 
> v3: To create a typo introduced when retyping
> v4: Prevent info==NULL dereference in early boot
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com>
> Tested-by: Gabriel Feceoru <gabriel.feceoru@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 72 +++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 153ea7a3fcf6..5d4be71bdf22 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
>  	dev_priv->fbdev = NULL;
>  }
>  
> +static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct fb_info *info;
> +
> +	if (dev_priv->fbdev == NULL)
> +		return NULL;
> +
> +	info = dev_priv->fbdev->helper.fbdev;
> +	if (info == NULL)
> +		return NULL;
> +
> +	if (info->screen_base == NULL)
> +		return NULL;
> +

This is rather verbose to my liking, I'd rather be dropping those '==
NULL' and convert to !. But either way to me.

> +	return dev_priv->fbdev;
> +}
> +
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> +	struct intel_fbdev *ifbdev;
> +
> +	ifbdev = intel_fbdev_get(dev);
> +	if (ifbdev == NULL)
> +		return NULL;
> +
> +	if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
> +		return NULL;
> +
> +	return ifbdev;
> +}
> +
>  void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> -	struct fb_info *info;
> +	struct intel_fbdev *ifbdev;
>  
> -	if (!ifbdev)
> +	ifbdev = intel_fbdev_get(dev);
> +	if (ifbdev == NULL)
>  		return;
>  
> -	info = ifbdev->helper.fbdev;
> -
>  	if (synchronous) {
>  		/* Flush any pending work to turn the console on, and then
>  		 * wait to turn it off. It must be synchronous as we are
> @@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>  	 * been restored from swap. If the object is stolen however, it will be
>  	 * full of whatever garbage was left in there.
>  	 */
> -	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> +	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> +		struct fb_info *info = ifbdev->helper.fbdev;
>  		memset_io(info->screen_base, 0, info->screen_size);
> +	}
>  
>  	drm_fb_helper_set_suspend(&ifbdev->helper, state);
>  	console_unlock();
> @@ -807,32 +839,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;
> +	struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> +
> +	if (ifbdev == NULL)
> +		return;
>  
> -	async_synchronize_full();
> -	if (dev_priv->fbdev)
> -		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> +	drm_fb_helper_hotplug_event(&ifbdev->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 = intel_fbdev_get_if_active(dev);
>  
> -	async_synchronize_full();

What's with the async_synchronize_full() begin removed completely?

> -	if (!ifbdev)
> +	if (ifbdev == NULL)

Argh.

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

Above addressed,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

>  	}
>  }
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx