linux-kernel - Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170225193630.gvuntw37fbec6jwy@phenom.ffwll.local>
Date:   Sat, 25 Feb 2017 20:36:31 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        Rongrong Zou <zourongrong@...il.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        Chen Feng <puck.chen@...ilicon.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Sean Paul <seanpaul@...omium.org>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb
 initialization race

On Fri, Feb 24, 2017 at 05:25:16PM -0800, John Stultz wrote:
> In some cases I've been seeing a race where two framebuffers
> would be initialized, as kirin_fbdev_output_poll_changed()
> might get called quickly in succession, resulting in the
> initialization happening twice. This could cause the system
> to boot up with a blank screen.
> 
> This patch adds a simple mutex to serialize it and seems to
> avoid the race.
> 
> Suggestions or feedback would be greatly appreciated!

I feel a bit like a broken record, but:

Instead of reinventing broken delayed fbdev setup everywhere, can we
polish Thierry's patches to move that into the fbdev helpers in the core?
hisilicon isn't the only driver the (re)invents this weel, it'd be really
awesome if we could fix this bug just once ...

For reference, the patches:

http://markmail.org/message/d3jc4vebkndtvlkf#query:+page:1+mid:d3jc4vebkndtvlkf+state:results

I guess Thierry got sidetracked on these, but except for a bit of locking
scheme polish I think they've been mostly ready. Shouldn't be much work to
refresh them, hunt for other new drivers reinventing this wheel, do the
locking polish maybe and then get them landed.

Thanks, Daniel

> 
> Cc: Xinliang Liu <z.liuxinliang@...ilicon.com>
> Cc: Rongrong Zou <zourongrong@...il.com>
> Cc: Xinwei Kong <kong.kongxinwei@...ilicon.com>
> Cc: Chen Feng <puck.chen@...ilicon.com>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: Sean Paul <seanpaul@...omium.org>
> Cc: dri-devel@...ts.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> index 7ec93ae..b83556a 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> @@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>  {
>  	struct kirin_drm_private *priv = dev->dev_private;
>  
> +	mutex_lock(&priv->fb_lock);
>  	if (priv->fbdev) {
>  		drm_fbdev_cma_hotplug_event(priv->fbdev);
>  	} else {
> @@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>  		if (IS_ERR(priv->fbdev))
>  			priv->fbdev = NULL;
>  	}
> +	mutex_unlock(&priv->fb_lock);
>  }
>  #endif
>  
> @@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device *dev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	mutex_init(&priv->fb_lock);
> +
>  	dev->dev_private = priv;
>  	dev_set_drvdata(dev->dev, dev);
>  
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> index 7f60c649..9b6d2b1 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> @@ -23,6 +23,7 @@ struct kirin_drm_private {
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  	struct drm_fbdev_cma *fbdev;
>  #endif
> +	struct mutex fb_lock;
>  };
>  
>  extern const struct kirin_dc_ops ade_dc_ops;
> -- 
> 2.7.4
> 

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