On Sun, Feb 12, 2023 at 10:44:33PM +0200, Oded Gabbay wrote:
> From: Tomer Tayar <ttayar@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Reviewed-by: Oded Gabbay <ogabbay@xxxxxxxxxx>
> Signed-off-by: Oded Gabbay <ogabbay@xxxxxxxxxx>
> ---
> 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
