linux-kernel - Re: [PATCH drm/hisilicon 2/2] drm/hisilicon: Code refactoring for hibmc_drv

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 31 Jul 2020 09:47:59 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Tian Tao <tiantao6@...ilicon.com>, airlied@...ux.ie,
        daniel@...ll.ch, kraxel@...hat.com, alexander.deucher@....com,
        tglx@...utronix.de, dri-devel@...ts.freedesktop.org,
        xinliang.liu@...aro.org, linux-kernel@...r.kernel.org
Cc:     linuxarm@...wei.com
Subject: Re: [PATCH drm/hisilicon 2/2] drm/hisilicon: Code refactoring for
 hibmc_drv_de

Hi

Am 31.07.20 um 03:05 schrieb Tian Tao:
> code refactoring for hibmc_drv_de.c, no actual function changes.

No, there's tons of functional changes. The memory used to be allocated
with devres helpers and released automatically. In rare circumstances,
the memory's release could have happened before the DRM device got
released, which would have caused memory corruption of some kind.

Now you're embedding the data structures in struct hibmc_drm_private,
which is good. The whole release problem has been resolved, because
struct hibmc_drm_private is allocated with drmm_kzalloc and always
released with the DRM device. You're commit message should mention all this.

> 
> Signed-off-by: Tian Tao <tiantao6@...ilicon.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 55 ++++++-------------------
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |  2 +
>  2 files changed, 15 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 66132eb..af24c72 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -157,37 +157,6 @@ static const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
>  	.atomic_update = hibmc_plane_atomic_update,
>  };
>  
> -static struct drm_plane *hibmc_plane_init(struct hibmc_drm_private *priv)
> -{
> -	struct drm_device *dev = priv->dev;
> -	struct drm_plane *plane;
> -	int ret = 0;
> -
> -	plane = devm_kzalloc(dev->dev, sizeof(*plane), GFP_KERNEL);
> -	if (!plane) {
> -		DRM_ERROR("failed to alloc memory when init plane\n");
> -		return ERR_PTR(-ENOMEM);
> -	}
> -	/*
> -	 * plane init
> -	 * TODO: Now only support primary plane, overlay planes
> -	 * need to do.
> -	 */
> -	ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
> -				       channel_formats1,
> -				       ARRAY_SIZE(channel_formats1),
> -				       NULL,
> -				       DRM_PLANE_TYPE_PRIMARY,
> -				       NULL);
> -	if (ret) {
> -		DRM_ERROR("failed to init plane: %d\n", ret);
> -		return ERR_PTR(ret);
> -	}
> -
> -	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
> -	return plane;
> -}
> -
>  static void hibmc_crtc_dpms(struct drm_crtc *crtc, int dpms)
>  {
>  	struct hibmc_drm_private *priv = crtc->dev->dev_private;
> @@ -534,22 +503,24 @@ static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>  int hibmc_de_init(struct hibmc_drm_private *priv)
>  {
>  	struct drm_device *dev = priv->dev;
> -	struct drm_crtc *crtc;
> -	struct drm_plane *plane;
> +	struct drm_crtc *crtc = &priv->crtc;
> +	struct drm_plane *plane = &priv->plane;
>  	int ret;
>  
> -	plane = hibmc_plane_init(priv);
> -	if (IS_ERR(plane)) {
> -		DRM_ERROR("failed to create plane: %ld\n", PTR_ERR(plane));
> -		return PTR_ERR(plane);
> -	}
> +	ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
> +				       channel_formats1,
> +				       ARRAY_SIZE(channel_formats1),
> +				       NULL,
> +				       DRM_PLANE_TYPE_PRIMARY,
> +				       NULL);
>  
> -	crtc = devm_kzalloc(dev->dev, sizeof(*crtc), GFP_KERNEL);
> -	if (!crtc) {
> -		DRM_ERROR("failed to alloc memory when init crtc\n");
> -		return -ENOMEM;
> +	if (ret) {
> +		DRM_ERROR("failed to init plane: %d\n", ret);
> +		return ret;
>  	}
>  
> +	drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
> +
>  	ret = drm_crtc_init_with_planes(dev, crtc, plane,
>  					NULL, &hibmc_crtc_funcs, NULL);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index a683763..91ef15c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -28,6 +28,8 @@ struct hibmc_drm_private {
>  
>  	/* drm */
>  	struct drm_device  *dev;
> +	struct drm_plane plane;

There was a TODO comment that mentioned overlay planes. I suggest
calling this field primary_plane, so it's clear which plane it represents.

Best regards
Thomas

> +	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
>  	bool mode_config_initialized;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



Download attachment "signature.asc" of type "application/pgp-signature" (517 bytes)