Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

[Pavel Begunkov]

On 8/6/26 08:29, Christian König wrote:
> On 8/5/26 12:59, Pavel Begunkov wrote:
> > On 8/5/26 09:27, Christian König wrote:
>
> > > > +
> > > > +    dma_resv_lock(dmabuf->resv, NULL);
> > > > +    ctx->dev_ops->unmap(ctx, map);
> > > > +    dma_resv_unlock(dmabuf->resv);
> > > > +
> > > > +    dma_fence_put(&fence->base);
> > >
> > > You should probably set map->fence to NULL after that.
> >
> > The map is freed two lines below, but I can add it as
> > a defensive measure.
>
> In that case it's ok, I've just haven't seen the kfree(map) below.
>
> > ...
> > > > +    ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > > > +    if (WARN_ON_ONCE(ret)) {
> > > > +        struct dma_fence *fence = &map->fence->base;
> > > > +
> > > > +        dma_fence_get(fence);
> > > > +        percpu_ref_kill(&map->refs);
> > > > +        dma_fence_wait(fence, false);
> > > > +        dma_fence_put(fence);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    dma_resv_add_fence(dmabuf->resv, &map->fence->base,
> > > > +               DMA_RESV_USAGE_KERNEL);
> > >
> > > That sequence is clearly incorrect!
> > >
> > > The fence must be created after dma_resv_reserve_fences(), otherwise you definately have an illegal memory operation here.
> >
> > I'm not sure what you mean, can you elaborate? I only cared about
> > pre-allocating it to avoid allocations here. We add / signal the fence
> > only once, no reuse. The map is going to be killed here, and if we
> > create a new map, it'll have its own fence.
> >
> > I can move the dma_fence_init() call here if that makes a difference?
>
> Yeah that is a good start, but you might need a bit more.
>
> Here is a summary of the usual procedure you need to follow when implementing a dma_fence backend:
>
> 1. Allocate your operation object, in this case here it's your mapping I think.
> 2. Prepare your operation, including all memory allocations.
> 3. Call dma_resv_reserve_fences() to reserve a fence slot.
> 4. Allocate and init your dma_fence object.

Looks I forgot it reply here, sorry for that. I killed dma fences in
v6 in favour of synchronous waiting, it turned to be less trouble even
outside of these fencing rules as I can rely on dma-resv lock less in
the code.

--
Pavel Begunkov