On 8/5/26 09:27, Christian König wrote:
...>> +struct dma_buf_io_fence {
+ struct dma_fence base;
+ spinlock_t lock;
+};
Upstream has change to allow embedding the spinlock into the dma_fence, so this
structure here is most likely not necessary any more.
ok
+static const char *dma_buf_io_fence_drv_name(struct dma_fence *fence)
+{
+ /* default fence release kfree's the base pointer */
+ BUILD_BUG_ON(offsetof(struct dma_buf_io_fence, base));
+
+ return "dma-buf-io-ctx";
+}
...>> +static void dma_buf_io_map_release_work(struct work_struct *work)
+{
+ struct dma_buf_io_map *map = container_of(work, struct dma_buf_io_map,
+ release_work);
+ struct dma_buf_io_fence *fence = map->fence;
+ struct dma_buf_io_ctx *ctx = map->ctx;
+ struct dma_buf *dmabuf = ctx->dmabuf;
+
+ /* the release path must wait for fences */
+ if (WARN_ON_ONCE(refcount_read(&ctx->refs) == 0))
+ return;
Stuff like that is usually illegal.
Should be fine, it's just a warning. The map holds a ctx
reference so can't be 0. IIRC, it was synchronised a bit
differently before. I can kill it, refcount_inc() has the
same warning anyway.
And why are you using refcount directly instead of kref?
Not sure it'd make much difference here.
+
+ /* Prevent from destoying the ctx while unmapping */
+ refcount_inc(&ctx->refs);
+
+ /*
+ * There are no more requests using the map, we can signal the fence.
+ * It should be done before taking the resv lock as someone could be
+ * waiting for the fence while holding the lock.
+ */
+ dma_fence_signal(&fence->base);
Signaling fences has a whole bunch of very strict rules associated with it.
E.g. you can't alocate memory for example.
Are you sure you actually need and want a dma_fence here?
Waiting for potentially a ton of IO synchronously on invalidate
sounds like a bad idea though. Hmm.
+
+ 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.
+ percpu_ref_exit(&map->refs);
+ kfree(map);
...>> +struct dma_buf_io_map *dma_buf_io_create_map(struct dma_buf_io_ctx *ctx)
+{
+ struct dma_buf *dmabuf = ctx->dmabuf;
+ struct dma_buf_io_map *map;
+ long ret;
+
+retry:
+ /*
+ * ->dmabuf_map() will be calling dma_buf_map_attachment(), for which
+ * we'll need to wait for fences. Do a bit nicer and try to wait
+ * without the resv lock first.
+ */
Clear NAK to that. Always wait while holding the resv lock if you can!
It is absolutely not beneficial to do this outside of the lock and usually just
hides problems instead and prevent fixing them.
Ok
...
+ 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?
--
Pavel Begunkov
