On Wed, 2026-08-19 at 10:59 +0200, Jiri Slaby wrote:
On 14. 07. 26, 10:43, Philipp Stanner wrote:
On Tue, 2026-07-14 at 09:53 +0200, Philipp Stanner wrote:
On Mon, 2026-07-13 at 10:58 +0200, Jiri Slaby wrote:
Hi,
On 20. 01. 26, 11:54, Christian König wrote:
Some driver use fence->ops to test if a fence was initialized or not. The problem is that this utilizes internal behavior of the dma_fence implementation.
So better abstract that into a function.
v2: use a flag instead of testing fence->ops, rename the function, move to the beginning of the patch set.
...
--- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -146,7 +146,7 @@ qxl_release_free(struct qxl_device *qdev, idr_remove(&qdev->release_idr, release->id); spin_unlock(&qdev->release_idr_lock);
- if (release->base.ops) {
- if (dma_fence_was_initialized(&release->base)) {
Could you verify the cause with sth like
if (release->base.ops && dma_fence_was_initialized(…)) {
Forget about that, probably would not work or cause other issues because the NULL-setting on signal().
I'd then probably try to verify it with a separate boolean in struct release. Though I also don't get why the fence-initialized check does not do the trick.
Hi, have you come up with something yet?
Well, I was more like suggesting this as a debug option [to you] :D
Regardless, looking at the code again, I would say that this might be a race, but I don't know enough about QXL to say for sure.
dma_fence_init() is (of course) not ordered:
static void __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) { BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
kref_init(&fence->refcount); /* * While it is counter intuitive to protect a constant function pointer * table by RCU it allows modules to wait for an RCU grace period * before they unload, to make sure that nobody is executing their * functions any more. */ RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list); fence->context = context; fence->seqno = seqno; fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
(Should this maybe be set_bit() btw?)
The fact that QXL could run into qxl_release_free() with an uninitialized fence hints at the fact that this might race, so DMA_FENCE_FLAG_INITIALIZED_BIT could be set / read before kref_init() ran.
Maybe one way to verify / debug that would be to move spin_unlock(&qdev->release_idr_lock) downwards so it also guards dma_fence_was_initialized(), and also lock the initialization of the fence (in qxl_release_fence_buffer_objects() ?) with said lock.
If that's possible. Just brainstorming a bit for ways how to debug.
QXL does a few tricky things with the release->base.ops pointer. qxl_release_alloc() sets it to NULL, and only qxl_release_fence_buffer_objects() then actually sets it. So this could be the race? Setting of the ops pointer got replaced by setting of the fence-flag.
P.
On Wed, 2026-08-19 at 14:33 +0200, Philipp Stanner wrote:
[…]
Regardless, looking at the code again, I would say that this might be a race, but I don't know enough about QXL to say for sure.
dma_fence_init() is (of course) not ordered:
static void __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) { BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
kref_init(&fence->refcount); /* * While it is counter intuitive to protect a constant function pointer * table by RCU it allows modules to wait for an RCU grace period * before they unload, to make sure that nobody is executing their * functions any more. */ RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list); fence->context = context; fence->seqno = seqno; fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
(Should this maybe be set_bit() btw?)
The fact that QXL could run into qxl_release_free() with an uninitialized fence hints at the fact that this might race, so DMA_FENCE_FLAG_INITIALIZED_BIT could be set / read before kref_init() ran.
Maybe one way to verify / debug that would be to move spin_unlock(&qdev->release_idr_lock) downwards so it also guards dma_fence_was_initialized(), and also lock the initialization of the fence (in qxl_release_fence_buffer_objects() ?) with said lock.
If that's possible. Just brainstorming a bit for ways how to debug.
QXL does a few tricky things with the release->base.ops pointer. qxl_release_alloc() sets it to NULL, and only qxl_release_fence_buffer_objects() then actually sets it. So this could be the race? Setting of the ops pointer got replaced by setting of the fence-flag.
P.
Could you test something like this? (not even compile-tested, just an idea)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 87797bea91cb..df1aa48b2809 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -1075,7 +1075,6 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, INIT_LIST_HEAD(&fence->cb_list); fence->context = context; fence->seqno = seqno; - fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT); if (lock) { fence->extern_lock = lock; } else { @@ -1084,6 +1083,8 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, } fence->error = 0;
+ smp_mb(); + fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT); trace_dma_fence_init(fence); }
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index b52ab692b22e..40ffdcafaac1 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -292,7 +292,12 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); */ static inline bool dma_fence_was_initialized(struct dma_fence *fence) { - return fence && test_bit(DMA_FENCE_FLAG_INITIALIZED_BIT, &fence->flags); + bool init; + + init = fence && test_bit(DMA_FENCE_FLAG_INITIALIZED_BIT, &fence->flags); + smp_mb(); + + return init; }
/**
linaro-mm-sig@lists.linaro.org
-
Philipp Stanner