Re: [PATCH 06/27] habanalabs: use memhash_node_export_put() in hl_release_dmabuf()

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Sun, Feb 12, 2023 at 10:44:33PM +0200, Oded Gabbay wrote:
> From: Tomer Tayar <ttayar@habana.ai>
> 
> The same mutex lock/unlock and counter decrementing in
> hl_release_dmabuf() is already done in the memhash_node_export_put()
> helper function.
> 
> Signed-off-by: Tomer Tayar <ttayar@habana.ai>
> Reviewed-by: Oded Gabbay <ogabbay@kernel.org>
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
>  drivers/accel/habanalabs/common/memory.c | 89 ++++++++++++------------
>  1 file changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/accel/habanalabs/common/memory.c b/drivers/accel/habanalabs/common/memory.c
> index e6474d38afc4..7b3809853dd5 100644
> --- a/drivers/accel/habanalabs/common/memory.c
> +++ b/drivers/accel/habanalabs/common/memory.c
> @@ -1779,6 +1779,47 @@ static void hl_unmap_dmabuf(struct dma_buf_attachment *attachment,
>  	kfree(sgt);
>  }
>  
> +static struct hl_vm_hash_node *memhash_node_export_get(struct hl_ctx *ctx, u64 addr)
> +{
> +	struct hl_device *hdev = ctx->hdev;
> +	struct hl_vm_hash_node *hnode;
> +
> +	/* get the memory handle */
> +	mutex_lock(&ctx->mem_hash_lock);
> +	hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr)
> +		if (addr == hnode->vaddr)
> +			break;
> +
> +	if (!hnode) {

This looks suspicious, I think hnode can be not-NULL here and has
hnode->vaddr different than searched addr, in case there is 
hash collision and we iterate over hlist where there is no
searched addr. Not 100% sure about that though.

I think would be better to provide helper like this:

	hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr)
		if (addr == hnode->vaddr)
			return hnode;
	return NULL;

which is basically standard way how hash_for_each_possible() used.


Regards
Stanislaw