linux-kernel - Re: [PATCH v15 16/19] drm/etnaviv: Call etnaviv_gem_obj_add() in ernaviv_gem_new

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: <67cce03c96f5e4d08d837b77c723fdaf214f7c0d.camel@pengutronix.de>
Date: Tue, 01 Oct 2024 16:39:01 +0200
From: Lucas Stach <l.stach@...gutronix.de>
To: Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: Christian Gmeiner <christian.gmeiner@...il.com>, Russell King
	 <linux+etnaviv@...linux.org.uk>, dri-devel@...ts.freedesktop.org, 
	etnaviv@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 16/19] drm/etnaviv: Call etnaviv_gem_obj_add() in
 ernaviv_gem_new_private()

Am Sonntag, dem 08.09.2024 um 17:43 +0800 schrieb Sui Jingfeng:
> The etnaviv_gem_obj_add() a common operation, the 'etnaviv_drm_private::
> gem_list' is being used to record(track) all of the etnaviv GEM buffer
> object created in this driver.
> 
> Once a etnaviv GEM buffer object has been allocated successfully, we
> should add it into the global etnaviv_drm_private::gem_list'. Because
> we need to free it and remove it free the list if the invoke of the
> subsequent functions fail.
> 
> The only way that destroy etnaviv GEM buffer object is the implementation
> of etnaviv_gem_free_object() function. The etnaviv_gem_free_object() first
> remove the etnaviv GEM object from the list, then destroy its mapping and
> finally free its memory footprint. Therefore, in order to corresponding
> this, we add the freshly created etnaviv GEM buffer object immediately
> after it was successfully created.
> 
> A benifit is that we only need to call etnaviv_gem_obj_add() once now,
> since the ernaviv_gem_new_private() has been unified. Make the
> etnaviv_gem_obj_add() static is a next nature thing.
> 
Seeing this patch, I would really ask you to drop patch 11 from this
series and go the other way around: remove etnaviv_gem_obj_add() here
altogether and simply inline the few lines of code into
etnaviv_gem_new_private().

Regards,
Lucas

> Signed-off-by: Sui Jingfeng <sui.jingfeng@...ux.dev>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 8 +++-----
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h       | 1 -
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 --
>  3 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 27e4a93c981c..ee799c02d0aa 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -584,7 +584,7 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj)
>  	kfree(etnaviv_obj);
>  }
>  
> -void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
> +static void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
>  {
>  	struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
>  	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
> @@ -719,8 +719,6 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  	 */
>  	mapping_set_gfp_mask(obj->filp->f_mapping, priv->shm_gfp_mask);
>  
> -	etnaviv_gem_obj_add(dev, obj);
> -
>  	ret = drm_gem_handle_create(file, obj, handle);
>  
>  	/* drop reference from allocate - handle holds it now */
> @@ -751,6 +749,8 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  		drm_gem_private_object_init(dev, obj, size);
>  	}
>  
> +	etnaviv_gem_obj_add(dev, obj);
> +
>  	*res = to_etnaviv_bo(obj);
>  
>  	return 0;
> @@ -849,8 +849,6 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
>  	etnaviv_obj->userptr.mm = current->mm;
>  	etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
>  
> -	etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
> -
>  	ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
>  
>  	/* drop reference from allocate - handle holds it now */
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index b174a9e4cc48..8d8fc5b3a541 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -121,7 +121,6 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  			    bool shmem, const struct etnaviv_gem_ops *ops,
>  			    struct etnaviv_gem_object **res);
>  
> -void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
>  struct page **etnaviv_gem_get_pages(struct etnaviv_gem_object *obj);
>  void etnaviv_gem_put_pages(struct etnaviv_gem_object *obj);
>  
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index 64a858a0b0cf..04ee31461b8c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -127,8 +127,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> -	etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
> -
>  	return &etnaviv_obj->base;
>  
>  fail: