Re: [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> The existing drm_fb_helper_hotplug_event() function needs to take the
> top-level fb-helper lock. However, the function can also be called from
> code that has already taken this lock. Introduce an unlocked variant of
> this function that can be used in the latter case.
> 
> This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via
> drm_fb_helper_set_par(), so we also need to introduce an unlocked copy
> of that to avoid recursive locking issues.
> 
> Similarly, the drm_fb_helper_initial_config() function ends up calling
> drm_fb_helper_set_par(), via register_framebuffer(), and needs an
> unlocked variant to avoid recursive locking.
> 
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 167 +++++++++++++++++++++++++---------------
>  1 file changed, 104 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 860be51d92f6..21a90322531c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -491,18 +491,10 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  	return 0;
>  }
>  
> -/**
> - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> - * @fb_helper: fbcon to restore
> - *
> - * This should be called from driver's drm &drm_driver.lastclose callback
> - * when implementing an fbcon on top of kms using this helper. This ensures that
> - * the user isn't greeted with a black screen when e.g. X dies.
> - *
> - * RETURNS:
> - * Zero if everything went ok, negative error code otherwise.
> - */
> -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
> +
> +static int
> +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  	bool do_delayed;
> @@ -511,7 +503,8 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return -ENODEV;
>  
> -	mutex_lock(&fb_helper->lock);
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));

lockdep_assert_held is the new cool.

> +
>  	drm_modeset_lock_all(dev);
>  
>  	ret = restore_fbdev_mode(fb_helper);
> @@ -521,10 +514,31 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  		fb_helper->delayed_hotplug = false;
>  
>  	drm_modeset_unlock_all(dev);
> -	mutex_unlock(&fb_helper->lock);
>  
>  	if (do_delayed)
> -		drm_fb_helper_hotplug_event(fb_helper);
> +		__drm_fb_helper_hotplug_event(fb_helper);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> + * @fb_helper: fbcon to restore
> + *
> + * This should be called from driver's drm &drm_driver.lastclose callback
> + * when implementing an fbcon on top of kms using this helper. This ensures that
> + * the user isn't greeted with a black screen when e.g. X dies.
> + *
> + * RETURNS:
> + * Zero if everything went ok, negative error code otherwise.
> + */
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +{
> +	int ret;
> +
> +	mutex_lock(&fb_helper->lock);
> +	ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	return ret;
>  }
> @@ -1486,7 +1500,7 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  		return -EINVAL;
>  	}
>  
> -	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +	__drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);

Nah, you need the locking still for when this is called from userspace
through fbdev ioctl.
>  
>  	return 0;
>  }
> @@ -2333,6 +2347,46 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	kfree(enabled);
>  }
>  
> +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
> +					  int bpp_sel)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct fb_info *info;
> +	int ret;
> +
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	drm_setup_crtcs(fb_helper,
> +			dev->mode_config.max_width,
> +			dev->mode_config.max_height);
> +	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	mutex_unlock(&dev->mode_config.mutex);
> +	if (ret)
> +		return ret;
> +
> +	info = fb_helper->fbdev;
> +	info->var.pixclock = 0;
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> +		 info->node, info->fix.id);
> +
> +	mutex_lock(&kernel_fb_helper_lock);
> +	if (list_empty(&kernel_fb_helper_list))
> +		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> +	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +	mutex_unlock(&kernel_fb_helper_lock);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_fb_helper_initial_config - setup a sane initial connector configuration
>   * @fb_helper: fb_helper device struct
> @@ -2377,41 +2431,50 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>   */
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	struct fb_info *info;
>  	int ret;
>  
> +	mutex_lock(&fb_helper->lock);
> +	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
> +	mutex_unlock(&fb_helper->lock);

Yeah this won't work because it'll deadlock with the register_framebuffer.
You need to push it down so that it only protects the same critical
section as mode_config.mutex currently ddoes.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_initial_config);
> +
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	unsigned int width, height;
> +
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
> +
>  	mutex_lock(&dev->mode_config.mutex);
> -	drm_setup_crtcs(fb_helper,
> -			dev->mode_config.max_width,
> -			dev->mode_config.max_height);
> -	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> -	mutex_unlock(&dev->mode_config.mutex);
> -	if (ret)
> -		return ret;
>  
> -	info = fb_helper->fbdev;
> -	info->var.pixclock = 0;
> -	ret = register_framebuffer(info);
> -	if (ret < 0)
> -		return ret;
> +	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> +		fb_helper->delayed_hotplug = true;
> +		mutex_unlock(&dev->mode_config.mutex);
> +		return 0;
> +	}
>  
> -	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> -		 info->node, info->fix.id);
> +	DRM_DEBUG_KMS("\n");
>  
> -	mutex_lock(&kernel_fb_helper_lock);
> -	if (list_empty(&kernel_fb_helper_list))
> -		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +	width = dev->mode_config.max_width;
> +	height = dev->mode_config.max_height;
>  
> -	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -	mutex_unlock(&kernel_fb_helper_lock);
> +	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> +		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> +
> +	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	drm_fb_helper_set_par(fb_helper->fbdev);
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  
>  /**
>   * drm_fb_helper_hotplug_event - respond to a hotplug notification by
> @@ -2436,35 +2499,13 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   */
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	int err = 0;
> -
> -	if (!drm_fbdev_emulation)
> -		return 0;
> +	int ret;
>  
>  	mutex_lock(&fb_helper->lock);
> -	mutex_lock(&dev->mode_config.mutex);
> -
> -	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> -		fb_helper->delayed_hotplug = true;
> -		mutex_unlock(&dev->mode_config.mutex);
> -		goto unlock;
> -	}
> -
> -	DRM_DEBUG_KMS("\n");
> -
> -	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> -
> -	mutex_unlock(&dev->mode_config.mutex);
> +	ret = __drm_fb_helper_hotplug_event(fb_helper);
>  	mutex_unlock(&fb_helper->lock);

Yeah you can't hold the lock through the entire hotplug_event process,
otherwise the potential register_framebuffer will go boom on a deadlock.
I'll reply to one of the other patches with what I think should be done.
-Daniel

>  
> -	drm_fb_helper_set_par(fb_helper->fbdev);
> -
> -	return 0;
> -
> -unlock:
> -	mutex_unlock(&fb_helper->lock);
> -	return err;
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>  
> -- 
> 2.12.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux