[PATCH] dma-buf: revert "use inline lock for the dma-fence-chain"

[PATCH] dma-buf: revert "use inline lock for the dma-fence-chain"

[Christian König]

This reverts commit a408c0ca0c411ca1ead995bdae3112a806c87556.

This causes a lockdep splat. Not really the right fix, but changing this
is more work than expected.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-fence-chain.c | 3 ++-
 include/linux/dma-fence-chain.h   | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index a707792b6025..a8a90acf4f34 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -245,6 +245,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 	struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
 	uint64_t context;
 
+	spin_lock_init(&chain->lock);
 	rcu_assign_pointer(chain->prev, prev);
 	chain->fence = fence;
 	chain->prev_seqno = 0;
@@ -260,7 +261,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 			seqno = max(prev->seqno, seqno);
 	}
 
-	dma_fence_init64(&chain->base, &dma_fence_chain_ops, NULL,
+	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
 			 context, seqno);
 
 	/*
diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
index df3beadf1515..5cd3ba53b4a1 100644
--- a/include/linux/dma-fence-chain.h
+++ b/include/linux/dma-fence-chain.h
@@ -46,6 +46,7 @@ struct dma_fence_chain {
 		 */
 		struct irq_work work;
 	};
+	spinlock_t lock;
 };
 
 
-- 
2.43.0

Re: [PATCH] dma-buf: revert "use inline lock for the dma-fence-chain"

[Matthew Brost]

On Mon, Feb 23, 2026 at 08:57:05PM +0100, Christian König wrote:
> This reverts commit a408c0ca0c411ca1ead995bdae3112a806c87556.
> 
> This causes a lockdep splat. Not really the right fix, but changing this
> is more work than expected.
> 

I think dma-fence-arrays will have the same issue.

Can’t we just assign a lockdep key for chains and arrays?

Typing as fast as I could, I came up with this, and it seems to work:

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index cd970eceaefb..d5cce24dca5b 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -200,6 +200,10 @@ void dma_fence_array_init(struct dma_fence_array *array,
                          u64 context, unsigned seqno,
                          bool signal_on_any)
 {
+#if IS_ENABLED(CONFIG_PROVE_LOCKING)
+       static struct lock_class_key dma_fence_array_lock_key;
+#endif
+
        WARN_ON(!num_fences || !fences);

        array->num_fences = num_fences;
@@ -208,6 +212,10 @@ void dma_fence_array_init(struct dma_fence_array *array,
                       seqno);
        init_irq_work(&array->work, irq_dma_fence_array_work);

+#if IS_ENABLED(CONFIG_PROVE_LOCKING)
+       lockdep_set_class(&array->base.inline_lock, &dma_fence_array_lock_key);
+#endif
+
        atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
        array->fences = fences;

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index a707792b6025..aa144b8c3534 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -242,6 +242,9 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
                          struct dma_fence *fence,
                          uint64_t seqno)
 {
+#if IS_ENABLED(CONFIG_PROVE_LOCKING)
+       static struct lock_class_key dma_fence_chain_lock_key;
+#endif
        struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
        uint64_t context;

@@ -263,6 +266,10 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
        dma_fence_init64(&chain->base, &dma_fence_chain_ops, NULL,
                         context, seqno);

+#if IS_ENABLED(CONFIG_PROVE_LOCKING)
+       lockdep_set_class(&chain->base.inline_lock, &dma_fence_chain_lock_key);
+#endif
+
        /*
         * Chaining dma_fence_chain container together is only allowed through
         * the prev fence and not through the contained fence.

Matt

> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-fence-chain.c | 3 ++-
>  include/linux/dma-fence-chain.h   | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a707792b6025..a8a90acf4f34 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -245,6 +245,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>  	struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
>  	uint64_t context;
>  
> +	spin_lock_init(&chain->lock);
>  	rcu_assign_pointer(chain->prev, prev);
>  	chain->fence = fence;
>  	chain->prev_seqno = 0;
> @@ -260,7 +261,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>  			seqno = max(prev->seqno, seqno);
>  	}
>  
> -	dma_fence_init64(&chain->base, &dma_fence_chain_ops, NULL,
> +	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>  			 context, seqno);
>  
>  	/*
> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
> index df3beadf1515..5cd3ba53b4a1 100644
> --- a/include/linux/dma-fence-chain.h
> +++ b/include/linux/dma-fence-chain.h
> @@ -46,6 +46,7 @@ struct dma_fence_chain {
>  		 */
>  		struct irq_work work;
>  	};
> +	spinlock_t lock;
>  };
>  
>  
> -- 
> 2.43.0
> 

Claude review: dma-buf: revert "use inline lock for the dma-fence-chain"

[Claude Code Review Bot]

Overall Series Review

Subject: dma-buf: revert "use inline lock for the dma-fence-chain"
Author: "=?UTF-8?q?Christian=20K=C3=B6nig?=" <ckoenig.leichtzumerken@gmail.com>
Patches: 2
Reviewed: 2026-02-24T09:42:37.426008

---

This is a single patch that reverts commit a408c0ca0c411ca1ead995bdae3112a806c87556, which changed dma-fence-chain to use an "inline lock" (passing NULL to `dma_fence_init64` and relying on a lock embedded within the `dma_fence` structure itself). The revert restores a per-chain-node `spinlock_t lock` field in `struct dma_fence_chain`, initializes it explicitly, and passes it to `dma_fence_init64`.

The stated reason is a lockdep splat. The patch is mechanically correct -- the spinlock is properly initialized before use, the field is properly placed in the struct, and the lifecycle is correct since the lock lives as long as the chain node. However, the commit message is minimal: it acknowledges this is "not really the right fix" but doesn't include the lockdep splat output or explain the root cause of the splat. Matt Brost's reply on the list suggests a potentially better fix using `lockdep_set_class` to assign distinct lock classes for chain and array fences, which would preserve the inline lock optimization while resolving the lockdep complaint.

---
Generated by Claude Code Patch Reviewer

Claude review: dma-buf: revert "use inline lock for the dma-fence-chain"

[Claude Code Review Bot]

Patch Review

The change itself is straightforward and correct:

> +	spin_lock_init(&chain->lock);
>  	rcu_assign_pointer(chain->prev, prev);

The spinlock is initialized before any use, which is correct. It is placed early in `dma_fence_chain_init`, well before the call to `dma_fence_init64` that stores the pointer.

> -	dma_fence_init64(&chain->base, &dma_fence_chain_ops, NULL,
> +	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>  			 context, seqno);

The lock pointer references `chain->lock` which is embedded in the chain object, so its lifetime matches the fence -- no use-after-free concern here.

> +	spinlock_t lock;
>  };

The field is placed after the `cb`/`work` union, outside the union, which is correct since the lock may be needed concurrently with either the callback or the irq work.

A few observations:

1. The commit message says "This causes a lockdep splat" but doesn't include the actual splat. It's standard practice in kernel patches fixing lockdep issues to include the splat output (or at least summarize the lock ordering conflict). This would help reviewers and future readers understand what the actual problem was.

2. The commit message's own admission that this is "not really the right fix, but changing this is more work than expected" is worth taking seriously. Matt Brost's reply proposes using `lockdep_set_class` with a static `lock_class_key` to give chain fences (and array fences) their own lockdep class. That approach would fix the lockdep splat without regressing on the memory/performance characteristics of the inline lock. Is there a reason that approach wouldn't work here, or is the plan to pursue it as a follow-up?

3. If the inline lock was introduced as an optimization (saving a spinlock per chain node), does reverting it have a measurable impact in workloads that create many chain nodes? The struct grows by `sizeof(spinlock_t)` per chain allocation.

No correctness bugs found in the patch itself.

---
Generated by Claude Code Patch Reviewer