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
