[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

2016-11-28 Thread Daniel Vetter 
On Mon, Nov 28, 2016 at 08:38:21AM +, Chris Wilson wrote:
> On Mon, Nov 28, 2016 at 08:55:58AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 23, 2016 at 02:04:17PM +, Chris Wilson wrote:
> > > Though we only walk the kernel_fb_helper_list inside a panic (or single
> > > thread debugging), we still need to protect the list manipulation on
> > > creating/removing a framebuffer device in order to prevent list
> > > corruption.
> > > 
> > > Signed-off-by: Chris Wilson 
> > 
> > I guess this explains the moved hunk in patch 1. Still feels misplaced,
> > but with or without moving that around:
> 
> No, that had to be moved to pull the register_framebuffer out from
> underneath the lock (as it causes a lock recursion into the fbdev trying
> to do a modeset).

Ah right, I missed that. Can you pls add that the commit message and
address Jani's question/comment when resending? Then I can pluck these 3
up.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

2016-11-28 Thread Daniel Vetter 
On Wed, Nov 23, 2016 at 02:04:17PM +, Chris Wilson wrote:
> Though we only walk the kernel_fb_helper_list inside a panic (or single
> thread debugging), we still need to protect the list manipulation on
> creating/removing a framebuffer device in order to prevent list
> corruption.
> 
> Signed-off-by: Chris Wilson 

I guess this explains the moved hunk in patch 1. Still feels misplaced,
but with or without moving that around:

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a19afc7eccde..2ac2f462d37b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
>"Enable legacy fbdev emulation [default=true]");
>  
>  static LIST_HEAD(kernel_fb_helper_list);
> +static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
>  /**
>   * DOC: fbdev helpers
> @@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>   if (!drm_fbdev_emulation)
>   return;
>  
> + mutex_lock(_fb_helper_lock);
>   if (!list_empty(_helper->kernel_fb_list)) {
>   list_del(_helper->kernel_fb_list);
>   if (list_empty(_fb_helper_list)) {
>   unregister_sysrq_key('v', 
> _drm_fb_helper_restore_op);
>   }
>   }
> + mutex_unlock(_fb_helper_lock);
>  
>   drm_fb_helper_crtc_free(fb_helper);
>  
> @@ -2257,10 +2260,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper 
> *fb_helper, int bpp_sel)
>   dev_info(dev->dev, "fb%d: %s frame buffer device\n",
>info->node, info->fix.id);
>  
> + mutex_lock(_fb_helper_lock);
>   if (list_empty(_fb_helper_list))
>   register_sysrq_key('v', _drm_fb_helper_restore_op);
>  
>   list_add(_helper->kernel_fb_list, _fb_helper_list);
> + mutex_unlock(_fb_helper_lock);
>  
>   return 0;
>  }
> -- 
> 2.10.2
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

2016-11-28 Thread Chris Wilson 
On Mon, Nov 28, 2016 at 08:55:58AM +0100, Daniel Vetter wrote:
> On Wed, Nov 23, 2016 at 02:04:17PM +, Chris Wilson wrote:
> > Though we only walk the kernel_fb_helper_list inside a panic (or single
> > thread debugging), we still need to protect the list manipulation on
> > creating/removing a framebuffer device in order to prevent list
> > corruption.
> > 
> > Signed-off-by: Chris Wilson 
> 
> I guess this explains the moved hunk in patch 1. Still feels misplaced,
> but with or without moving that around:

No, that had to be moved to pull the register_framebuffer out from
underneath the lock (as it causes a lock recursion into the fbdev trying
to do a modeset).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

2016-11-23 Thread Chris Wilson 
Though we only walk the kernel_fb_helper_list inside a panic (or single
thread debugging), we still need to protect the list manipulation on
creating/removing a framebuffer device in order to prevent list
corruption.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_fb_helper.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a19afc7eccde..2ac2f462d37b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
 "Enable legacy fbdev emulation [default=true]");

 static LIST_HEAD(kernel_fb_helper_list);
+static DEFINE_MUTEX(kernel_fb_helper_lock);

 /**
  * DOC: fbdev helpers
@@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return;

+   mutex_lock(_fb_helper_lock);
if (!list_empty(_helper->kernel_fb_list)) {
list_del(_helper->kernel_fb_list);
if (list_empty(_fb_helper_list)) {
unregister_sysrq_key('v', 
_drm_fb_helper_restore_op);
}
}
+   mutex_unlock(_fb_helper_lock);

drm_fb_helper_crtc_free(fb_helper);

@@ -2257,10 +2260,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
dev_info(dev->dev, "fb%d: %s frame buffer device\n",
 info->node, info->fix.id);

+   mutex_lock(_fb_helper_lock);
if (list_empty(_fb_helper_list))
register_sysrq_key('v', _drm_fb_helper_restore_op);

list_add(_helper->kernel_fb_list, _fb_helper_list);
+   mutex_unlock(_fb_helper_lock);

return 0;
 }
-- 
2.10.2