On Fri, Apr 15, 2016 at 03:10:39PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> No need to hold the lock while assigning the variable.
>
> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
Not sure why exactly I put that under the lock, but the only thing that
can race here is rmfb while addfb2 is still doing it's thing, with a
correctly guess (easy to do since they're fully deterministic) fb_id.
But that race can't happen because rmfb checks whether the fb is
associated with the drm_file, and if not bails out. And since
mutex_lock/unlock together are a full barrier gcc can't even reorder
things so that it would be possible to return a 0 fb_id.
I think I convinced myself that this is totally safe ;-)
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_crtc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1863879..21cb998 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3405,11 +3405,11 @@ int drm_mode_addfb2(struct drm_device *dev,
> if (IS_ERR(fb))
> return PTR_ERR(fb);
>
> - /* Transfer ownership to the filp for reaping on close */
> -
> DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
> - mutex_lock(&file_priv->fbs_lock);
> r->fb_id = fb->base.id;
> +
> + /* Transfer ownership to the filp for reaping on close */
> + mutex_lock(&file_priv->fbs_lock);
> list_add(&fb->filp_head, &file_priv->fbs);
> mutex_unlock(&file_priv->fbs_lock);
>
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
