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.
