linux-kernel - Re: [RESEND PATCH V1] accel/amdxdna: Call dma_buf_vmap

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <50380149-c9cd-4478-9fe0-93d95a1016af@linux.intel.com>
Date: Tue, 16 Sep 2025 17:55:12 +0200
From: "Falkowski, Maciej" <maciej.falkowski@...ux.intel.com>
To: Lizhi Hou <lizhi.hou@....com>, ogabbay@...nel.org,
 quic_jhugo@...cinc.com, dri-devel@...ts.freedesktop.org
Cc: linux-kernel@...r.kernel.org, max.zhen@....com, sonal.santan@....com,
 mario.limonciello@....com
Subject: Re: [RESEND PATCH V1] accel/amdxdna: Call dma_buf_vmap_unlocked() for
 imported object

On 9/15/2025 6:10 PM, Lizhi Hou wrote:

> In amdxdna_gem_obj_vmap(), calling dma_buf_vmap() triggers a kernel
> warning if LOCKDEP is enabled. So for imported object, use
> dma_buf_vmap_unlocked(). Then, use drm_gem_vmap() for other objects.
> The similar change applies to vunmap code.
>
> Fixes: bd72d4acda10 ("accel/amdxdna: Support user space allocated buffer")
> Signed-off-by: Lizhi Hou <lizhi.hou@....com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 38 +++++++++++------------------
>   1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index d407a36eb412..50950be189ae 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -392,35 +392,25 @@ static const struct dma_buf_ops amdxdna_dmabuf_ops = {
>   	.vunmap = drm_gem_dmabuf_vunmap,
>   };
>   
> -static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *map)
> +static int amdxdna_gem_obj_vmap(struct amdxdna_gem_obj *abo, struct iosys_map *map)
>   {
> -	struct amdxdna_gem_obj *abo = to_xdna_obj(obj);
> -
>   	iosys_map_clear(map);
>   
> -	dma_resv_assert_held(obj->resv);
> -
>   	if (is_import_bo(abo))
> -		dma_buf_vmap(abo->dma_buf, map);
> -	else
> -		drm_gem_shmem_object_vmap(obj, map);
> -
> -	if (!map->vaddr)
> -		return -ENOMEM;
> +		return dma_buf_vmap_unlocked(abo->dma_buf, map);
Hi,

The dma_buf_vmap_unlocked() will call iosys_map_clear at its start so that
in this case it will be called twice. Probably it will be optimize out, 
but maybe
its something to better omit.
>   
> -	return 0;
> +	return drm_gem_vmap(to_gobj(abo), map);
>   }
>   
> -static void amdxdna_gem_obj_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
> +static void amdxdna_gem_obj_vunmap(struct amdxdna_gem_obj *abo, struct iosys_map *map)
>   {
> -	struct amdxdna_gem_obj *abo = to_xdna_obj(obj);
> -
> -	dma_resv_assert_held(obj->resv);
> +	if (iosys_map_is_null(map))
> +		return;
>   
>   	if (is_import_bo(abo))
> -		dma_buf_vunmap(abo->dma_buf, map);
> -	else
> -		drm_gem_shmem_object_vunmap(obj, map);
> +		return dma_buf_vunmap_unlocked(abo->dma_buf, map);
I do also wonder what is the convention here on clearing iosys_map when 
returning.
The function drm_gem_vunmap will clear the map for callers while the 
other not.
I think at least comment explaining the logic will be necessary.

Best regards,
Maciej

> +
> +	return drm_gem_vunmap(to_gobj(abo), map);
>   }
>   
>   static struct dma_buf *amdxdna_gem_prime_export(struct drm_gem_object *gobj, int flags)
> @@ -468,7 +458,7 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
>   	if (abo->type == AMDXDNA_BO_DEV_HEAP)
>   		drm_mm_takedown(&abo->mm);
>   
> -	drm_gem_vunmap(gobj, &map);
> +	amdxdna_gem_obj_vunmap(abo, &map);
>   	mutex_destroy(&abo->lock);
>   
>   	if (is_import_bo(abo)) {
> @@ -489,8 +479,8 @@ static const struct drm_gem_object_funcs amdxdna_gem_shmem_funcs = {
>   	.pin = drm_gem_shmem_object_pin,
>   	.unpin = drm_gem_shmem_object_unpin,
>   	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> -	.vmap = amdxdna_gem_obj_vmap,
> -	.vunmap = amdxdna_gem_obj_vunmap,
> +	.vmap = drm_gem_shmem_object_vmap,
> +	.vunmap = drm_gem_shmem_object_vunmap,
>   	.mmap = amdxdna_gem_obj_mmap,
>   	.vm_ops = &drm_gem_shmem_vm_ops,
>   	.export = amdxdna_gem_prime_export,
> @@ -692,7 +682,7 @@ amdxdna_drm_create_dev_heap(struct drm_device *dev,
>   	abo->mem.dev_addr = client->xdna->dev_info->dev_mem_base;
>   	drm_mm_init(&abo->mm, abo->mem.dev_addr, abo->mem.size);
>   
> -	ret = drm_gem_vmap(to_gobj(abo), &map);
> +	ret = amdxdna_gem_obj_vmap(abo, &map);
>   	if (ret) {
>   		XDNA_ERR(xdna, "Vmap heap bo failed, ret %d", ret);
>   		goto release_obj;
> @@ -770,7 +760,7 @@ amdxdna_drm_create_cmd_bo(struct drm_device *dev,
>   	abo->type = AMDXDNA_BO_CMD;
>   	abo->client = filp->driver_priv;
>   
> -	ret = drm_gem_vmap(to_gobj(abo), &map);
> +	ret = amdxdna_gem_obj_vmap(abo, &map);
>   	if (ret) {
>   		XDNA_ERR(xdna, "Vmap cmd bo failed, ret %d", ret);
>   		goto release_obj;