On 28. 08. 26, 9:29, Jiri Slaby wrote:
On 19. 08. 26, 14:51, Philipp Stanner wrote:
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)
It makes the system dead during early boot :P.
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,
This missing piece was here:
- fence->flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT);
+ flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT);
}
fence->error = 0;
+ smp_mb();
+ fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
trace_dma_fence_init(fence);
}
But it does not help either...
What helps is indeed the revert back to:
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -147,7 +147,7 @@ qxl_release_free(struct qxl_device *qdev,
idr_remove(&qdev->release_idr, release->id);
spin_unlock(&qdev->release_idr_lock);
- if (dma_fence_was_initialized(&release->base)) {
+ if (release->base.ops) {
WARN_ON(list_empty(&release->bos));
qxl_release_free_list(release);
Or the bool flag appears to help too:
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -144,6 +144,7 @@ enum {
#define QXL_MAX_RES 96
struct qxl_release {
struct dma_fence base;
+ bool uses_fence;
int id;
int type;
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -97,6 +97,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
return -ENOMEM;
}
release->base.ops = NULL;
+ release->uses_fence = false;
release->type = type;
release->release_offset = 0;
release->surface_release_id = 0;
@@ -147,7 +148,7 @@ qxl_release_free(struct qxl_device *qdev,
idr_remove(&qdev->release_idr, release->id);
spin_unlock(&qdev->release_idr_lock);
- if (dma_fence_was_initialized(&release->base)) {
+ if (release->uses_fence) {
WARN_ON(list_empty(&release->bos));
qxl_release_free_list(release);
@@ -431,6 +432,7 @@ void qxl_release_fence_buffer_objects(struct
qxl_release *release)
*/
dma_fence_init(&release->base, &qxl_fence_ops, &qdev->release_lock,
release->id | 0xf0000000, release->base.seqno);
+ release->uses_fence = true;
trace_dma_fence_emit(&release->base);
list_for_each_entry(entry, &release->bos, list) {
thanks,
--
js
suse labs
