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

[Tomer Tayar]

On Thu, Feb 16, 2023 at 13:48 Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com> wrote:
> > 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 at habana.ai>
> > Reviewed-by: Oded Gabbay <ogabbay at kernel.org>
> > Signed-off-by: Oded Gabbay <ogabbay at 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

Thanks Stanislaw, we will add such a helper and use it here and in another place with a similar pattern.
If that is okay, we will do it in another patch, as this one only moves an existing function for code reuse.