* [PATCH 2/2] dma-buf: Assign separate lockdep class to array lock
2026-02-24 17:55 [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock Matthew Brost
@ 2026-02-24 17:55 ` Matthew Brost
2026-02-25 8:26 ` [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock Christian König
2026-03-02 15:28 ` Boris Brezillon
2 siblings, 0 replies; 8+ messages in thread
From: Matthew Brost @ 2026-02-24 17:55 UTC (permalink / raw)
To: intel-xe, dri-devel
Cc: Christian König, Tvrtko Ursulin, Philipp Stanner,
Boris Brezillon
dma_fence_array_enable_signaling() runs while holding the array
inline_lock and may add callbacks to underlying fences, which takes
their inline_lock.
Since both locks share the same lockdep class, this valid nesting
triggers a recursive locking warning. Assign a distinct lockdep class
to the array inline_lock so lockdep can correctly model the hierarchy.
Fixes: 5943243914b9 ("dma-buf: use inline lock for the dma-fence-array")
Cc: Christian König <christian.koenig@amd.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
drivers/dma-buf/dma-fence-array.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index cd970eceaefb..9cfac1ca68b5 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_LOCKDEP)
+ static struct lock_class_key dma_fence_array_lock_key;
+#endif
+
WARN_ON(!num_fences || !fences);
array->num_fences = num_fences;
@@ -208,6 +212,20 @@ 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_LOCKDEP)
+ /*
+ * dma_fence_array_enable_signaling() is invoked while holding
+ * array->base.inline_lock and may call dma_fence_add_callback()
+ * on the underlying fences, which takes their inline_lock.
+ *
+ * Since both locks share the same lockdep class, this legitimate
+ * nesting confuses lockdep and triggers a recursive locking
+ * warning. Assign a separate lockdep class to the array lock
+ * to model this hierarchy correctly.
+ */
+ 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;
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-02-24 17:55 [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock Matthew Brost
2026-02-24 17:55 ` [PATCH 2/2] dma-buf: Assign separate lockdep class to array lock Matthew Brost
@ 2026-02-25 8:26 ` Christian König
2026-03-02 15:28 ` Boris Brezillon
2 siblings, 0 replies; 8+ messages in thread
From: Christian König @ 2026-02-25 8:26 UTC (permalink / raw)
To: Matthew Brost, intel-xe, dri-devel
Cc: Tvrtko Ursulin, Philipp Stanner, Boris Brezillon
On 2/24/26 18:55, Matthew Brost wrote:
> dma_fence_chain_enable_signaling() runs while holding the chain
> inline_lock and may add callbacks to underlying fences, which takes
> their inline_lock.
Just for the record, we have the same problem for dma_fence_chain_is_signaled().
Maybe mention that in the commit message as well for completeness.
>
> Since both locks share the same lockdep class, this valid nesting
> triggers a recursive locking warning. Assign a distinct lockdep class
> to the chain inline_lock so lockdep can correctly model the hierarchy.
Thanks a lot! I was just about to figure out how to do this.
>
> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the dma-fence-chain")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com> for both patches.
Regards,
Christian.
> ---
> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a707792b6025..4c2a9f2ce126 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_LOCKDEP)
> + 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,20 @@ 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_LOCKDEP)
> + /*
> + * dma_fence_chain_enable_signaling() is invoked while holding
> + * chain->base.inline_lock and may call dma_fence_add_callback()
> + * on the underlying fences, which takes their inline_lock.
> + *
> + * Since both locks share the same lockdep class, this legitimate
> + * nesting confuses lockdep and triggers a recursive locking
> + * warning. Assign a separate lockdep class to the chain lock
> + * to model this hierarchy correctly.
> + */
> + 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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-02-24 17:55 [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock Matthew Brost
2026-02-24 17:55 ` [PATCH 2/2] dma-buf: Assign separate lockdep class to array lock Matthew Brost
2026-02-25 8:26 ` [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock Christian König
@ 2026-03-02 15:28 ` Boris Brezillon
2026-03-02 15:42 ` Christian König
2 siblings, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2026-03-02 15:28 UTC (permalink / raw)
To: Matthew Brost
Cc: intel-xe, dri-devel, Christian König, Tvrtko Ursulin,
Philipp Stanner
On Tue, 24 Feb 2026 09:55:43 -0800
Matthew Brost <matthew.brost@intel.com> wrote:
> dma_fence_chain_enable_signaling() runs while holding the chain
> inline_lock and may add callbacks to underlying fences, which takes
> their inline_lock.
>
> Since both locks share the same lockdep class, this valid nesting
> triggers a recursive locking warning. Assign a distinct lockdep class
> to the chain inline_lock so lockdep can correctly model the hierarchy.
>
> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the dma-fence-chain")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a707792b6025..4c2a9f2ce126 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_LOCKDEP)
> + 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,20 @@ 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_LOCKDEP)
> + /*
> + * dma_fence_chain_enable_signaling() is invoked while holding
> + * chain->base.inline_lock and may call dma_fence_add_callback()
> + * on the underlying fences, which takes their inline_lock.
> + *
> + * Since both locks share the same lockdep class, this legitimate
> + * nesting confuses lockdep and triggers a recursive locking
> + * warning. Assign a separate lockdep class to the chain lock
> + * to model this hierarchy correctly.
> + */
> + lockdep_set_class(&chain->base.inline_lock, &dma_fence_chain_lock_key);
> +#endif
If we're going to recommend the use of this inline_lock for all new
dma_fence_ops implementers, as the commit message seems to imply
> Shared spinlocks have the problem that implementations need to guarantee
> that the lock lives at least as long all fences referencing them.
>
> Using a per-fence spinlock allows completely decoupling spinlock
> producer and consumer life times, simplifying the handling in most use
> cases.
maybe we should have the lock_class_key at the dma_buf_ops level and
have this lockdep_set_class() automated in __dma_fence_init().
> +
> /*
> * Chaining dma_fence_chain container together is only allowed through
> * the prev fence and not through the contained fence.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-03-02 15:28 ` Boris Brezillon
@ 2026-03-02 15:42 ` Christian König
2026-03-02 16:39 ` Boris Brezillon
0 siblings, 1 reply; 8+ messages in thread
From: Christian König @ 2026-03-02 15:42 UTC (permalink / raw)
To: Boris Brezillon, Matthew Brost
Cc: intel-xe, dri-devel, Tvrtko Ursulin, Philipp Stanner
On 3/2/26 16:28, Boris Brezillon wrote:
> On Tue, 24 Feb 2026 09:55:43 -0800
> Matthew Brost <matthew.brost@intel.com> wrote:
>
>> dma_fence_chain_enable_signaling() runs while holding the chain
>> inline_lock and may add callbacks to underlying fences, which takes
>> their inline_lock.
>>
>> Since both locks share the same lockdep class, this valid nesting
>> triggers a recursive locking warning. Assign a distinct lockdep class
>> to the chain inline_lock so lockdep can correctly model the hierarchy.
>>
>> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the dma-fence-chain")
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>> index a707792b6025..4c2a9f2ce126 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_LOCKDEP)
>> + 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,20 @@ 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_LOCKDEP)
>> + /*
>> + * dma_fence_chain_enable_signaling() is invoked while holding
>> + * chain->base.inline_lock and may call dma_fence_add_callback()
>> + * on the underlying fences, which takes their inline_lock.
>> + *
>> + * Since both locks share the same lockdep class, this legitimate
>> + * nesting confuses lockdep and triggers a recursive locking
>> + * warning. Assign a separate lockdep class to the chain lock
>> + * to model this hierarchy correctly.
>> + */
>> + lockdep_set_class(&chain->base.inline_lock, &dma_fence_chain_lock_key);
>> +#endif
>
> If we're going to recommend the use of this inline_lock for all new
> dma_fence_ops implementers, as the commit message seems to imply
>
>> Shared spinlocks have the problem that implementations need to guarantee
>> that the lock lives at least as long all fences referencing them.
>>
>> Using a per-fence spinlock allows completely decoupling spinlock
>> producer and consumer life times, simplifying the handling in most use
>> cases.
>
> maybe we should have the lock_class_key at the dma_buf_ops level and
> have this lockdep_set_class() automated in __dma_fence_init().
The dma_fence_chain() and dma_fence_array() containers are the only ones who are allowed to nest the lock with other dma_fences. E.g. we have WARN_ON()s in place which fire when you try to stitch together something which won't work.
So everybody else should get a lockdep warning when they try to do nasty things like this because you really can't guarantee lock order between different dma_fence implementations.
Regards,
Christian.
>
>> +
>> /*
>> * Chaining dma_fence_chain container together is only allowed through
>> * the prev fence and not through the contained fence.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-03-02 15:42 ` Christian König
@ 2026-03-02 16:39 ` Boris Brezillon
2026-03-02 20:05 ` Matthew Brost
0 siblings, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2026-03-02 16:39 UTC (permalink / raw)
To: Christian König
Cc: Matthew Brost, intel-xe, dri-devel, Tvrtko Ursulin,
Philipp Stanner
On Mon, 2 Mar 2026 16:42:28 +0100
Christian König <christian.koenig@amd.com> wrote:
> On 3/2/26 16:28, Boris Brezillon wrote:
> > On Tue, 24 Feb 2026 09:55:43 -0800
> > Matthew Brost <matthew.brost@intel.com> wrote:
> >
> >> dma_fence_chain_enable_signaling() runs while holding the chain
> >> inline_lock and may add callbacks to underlying fences, which takes
> >> their inline_lock.
> >>
> >> Since both locks share the same lockdep class, this valid nesting
> >> triggers a recursive locking warning. Assign a distinct lockdep
> >> class to the chain inline_lock so lockdep can correctly model the
> >> hierarchy.
> >>
> >> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the
> >> dma-fence-chain") Cc: Christian König <christian.koenig@amd.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >> Cc: Philipp Stanner <phasta@kernel.org>
> >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> >> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> >> ---
> >> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
> >> 1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/dma-buf/dma-fence-chain.c
> >> b/drivers/dma-buf/dma-fence-chain.c index
> >> a707792b6025..4c2a9f2ce126 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_LOCKDEP)
> >> + 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,20 @@ 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_LOCKDEP)
> >> + /*
> >> + * dma_fence_chain_enable_signaling() is invoked while
> >> holding
> >> + * chain->base.inline_lock and may call
> >> dma_fence_add_callback()
> >> + * on the underlying fences, which takes their
> >> inline_lock.
> >> + *
> >> + * Since both locks share the same lockdep class, this
> >> legitimate
> >> + * nesting confuses lockdep and triggers a recursive
> >> locking
> >> + * warning. Assign a separate lockdep class to the chain
> >> lock
> >> + * to model this hierarchy correctly.
> >> + */
> >> + lockdep_set_class(&chain->base.inline_lock,
> >> &dma_fence_chain_lock_key); +#endif
> >
> > If we're going to recommend the use of this inline_lock for all new
> > dma_fence_ops implementers, as the commit message seems to imply
> >
> >> Shared spinlocks have the problem that implementations need to
> >> guarantee that the lock lives at least as long all fences
> >> referencing them.
> >>
> >> Using a per-fence spinlock allows completely decoupling spinlock
> >> producer and consumer life times, simplifying the handling in most
> >> use cases.
> >
> > maybe we should have the lock_class_key at the dma_buf_ops level and
> > have this lockdep_set_class() automated in __dma_fence_init().
>
> The dma_fence_chain() and dma_fence_array() containers are the only
> ones who are allowed to nest the lock with other dma_fences. E.g. we
> have WARN_ON()s in place which fire when you try to stitch together
> something which won't work.
>
> So everybody else should get a lockdep warning when they try to do
> nasty things like this because you really can't guarantee lock order
> between different dma_fence implementations.
Okay, that makes sense.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-03-02 16:39 ` Boris Brezillon
@ 2026-03-02 20:05 ` Matthew Brost
2026-03-03 8:36 ` Boris Brezillon
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Brost @ 2026-03-02 20:05 UTC (permalink / raw)
To: Boris Brezillon
Cc: Christian König, intel-xe, dri-devel, Tvrtko Ursulin,
Philipp Stanner
On Mon, Mar 02, 2026 at 05:39:59PM +0100, Boris Brezillon wrote:
> On Mon, 2 Mar 2026 16:42:28 +0100
> Christian König <christian.koenig@amd.com> wrote:
>
> > On 3/2/26 16:28, Boris Brezillon wrote:
> > > On Tue, 24 Feb 2026 09:55:43 -0800
> > > Matthew Brost <matthew.brost@intel.com> wrote:
> > >
> > >> dma_fence_chain_enable_signaling() runs while holding the chain
> > >> inline_lock and may add callbacks to underlying fences, which takes
> > >> their inline_lock.
> > >>
> > >> Since both locks share the same lockdep class, this valid nesting
> > >> triggers a recursive locking warning. Assign a distinct lockdep
> > >> class to the chain inline_lock so lockdep can correctly model the
> > >> hierarchy.
> > >>
> > >> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the
> > >> dma-fence-chain") Cc: Christian König <christian.koenig@amd.com>
> > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > >> Cc: Philipp Stanner <phasta@kernel.org>
> > >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > >> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > >> ---
> > >> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
> > >> 1 file changed, 17 insertions(+)
> > >>
> > >> diff --git a/drivers/dma-buf/dma-fence-chain.c
> > >> b/drivers/dma-buf/dma-fence-chain.c index
> > >> a707792b6025..4c2a9f2ce126 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_LOCKDEP)
> > >> + 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,20 @@ 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_LOCKDEP)
> > >> + /*
> > >> + * dma_fence_chain_enable_signaling() is invoked while
> > >> holding
> > >> + * chain->base.inline_lock and may call
> > >> dma_fence_add_callback()
> > >> + * on the underlying fences, which takes their
> > >> inline_lock.
> > >> + *
> > >> + * Since both locks share the same lockdep class, this
> > >> legitimate
> > >> + * nesting confuses lockdep and triggers a recursive
> > >> locking
> > >> + * warning. Assign a separate lockdep class to the chain
> > >> lock
> > >> + * to model this hierarchy correctly.
> > >> + */
> > >> + lockdep_set_class(&chain->base.inline_lock,
> > >> &dma_fence_chain_lock_key); +#endif
> > >
> > > If we're going to recommend the use of this inline_lock for all new
> > > dma_fence_ops implementers, as the commit message seems to imply
> > >
> > >> Shared spinlocks have the problem that implementations need to
> > >> guarantee that the lock lives at least as long all fences
> > >> referencing them.
> > >>
> > >> Using a per-fence spinlock allows completely decoupling spinlock
> > >> producer and consumer life times, simplifying the handling in most
> > >> use cases.
> > >
> > > maybe we should have the lock_class_key at the dma_buf_ops level and
> > > have this lockdep_set_class() automated in __dma_fence_init().
> >
> > The dma_fence_chain() and dma_fence_array() containers are the only
> > ones who are allowed to nest the lock with other dma_fences. E.g. we
> > have WARN_ON()s in place which fire when you try to stitch together
> > something which won't work.
> >
> > So everybody else should get a lockdep warning when they try to do
> > nasty things like this because you really can't guarantee lock order
> > between different dma_fence implementations.
>
> Okay, that makes sense.
Yes, I agree with Christian's reasoning - chains / arrays is the only
case where nesting should be allowed. Also if we assigned a key for
every inline lock we'd quickly exhaust the number of lockdep keys.
Matt
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] dma-buf: Assign separate lockdep class to chain lock
2026-03-02 20:05 ` Matthew Brost
@ 2026-03-03 8:36 ` Boris Brezillon
0 siblings, 0 replies; 8+ messages in thread
From: Boris Brezillon @ 2026-03-03 8:36 UTC (permalink / raw)
To: Matthew Brost
Cc: Christian König, intel-xe, dri-devel, Tvrtko Ursulin,
Philipp Stanner
On Mon, 2 Mar 2026 12:05:46 -0800
Matthew Brost <matthew.brost@intel.com> wrote:
> On Mon, Mar 02, 2026 at 05:39:59PM +0100, Boris Brezillon wrote:
> > On Mon, 2 Mar 2026 16:42:28 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >
> > > On 3/2/26 16:28, Boris Brezillon wrote:
> > > > On Tue, 24 Feb 2026 09:55:43 -0800
> > > > Matthew Brost <matthew.brost@intel.com> wrote:
> > > >
> > > >> dma_fence_chain_enable_signaling() runs while holding the chain
> > > >> inline_lock and may add callbacks to underlying fences, which takes
> > > >> their inline_lock.
> > > >>
> > > >> Since both locks share the same lockdep class, this valid nesting
> > > >> triggers a recursive locking warning. Assign a distinct lockdep
> > > >> class to the chain inline_lock so lockdep can correctly model the
> > > >> hierarchy.
> > > >>
> > > >> Fixes: a408c0ca0c41 ("dma-buf: use inline lock for the
> > > >> dma-fence-chain") Cc: Christian König <christian.koenig@amd.com>
> > > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > >> Cc: Philipp Stanner <phasta@kernel.org>
> > > >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > > >> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > >> ---
> > > >> drivers/dma-buf/dma-fence-chain.c | 17 +++++++++++++++++
> > > >> 1 file changed, 17 insertions(+)
> > > >>
> > > >> diff --git a/drivers/dma-buf/dma-fence-chain.c
> > > >> b/drivers/dma-buf/dma-fence-chain.c index
> > > >> a707792b6025..4c2a9f2ce126 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_LOCKDEP)
> > > >> + 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,20 @@ 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_LOCKDEP)
> > > >> + /*
> > > >> + * dma_fence_chain_enable_signaling() is invoked while
> > > >> holding
> > > >> + * chain->base.inline_lock and may call
> > > >> dma_fence_add_callback()
> > > >> + * on the underlying fences, which takes their
> > > >> inline_lock.
> > > >> + *
> > > >> + * Since both locks share the same lockdep class, this
> > > >> legitimate
> > > >> + * nesting confuses lockdep and triggers a recursive
> > > >> locking
> > > >> + * warning. Assign a separate lockdep class to the chain
> > > >> lock
> > > >> + * to model this hierarchy correctly.
> > > >> + */
> > > >> + lockdep_set_class(&chain->base.inline_lock,
> > > >> &dma_fence_chain_lock_key); +#endif
> > > >
> > > > If we're going to recommend the use of this inline_lock for all new
> > > > dma_fence_ops implementers, as the commit message seems to imply
> > > >
> > > >> Shared spinlocks have the problem that implementations need to
> > > >> guarantee that the lock lives at least as long all fences
> > > >> referencing them.
> > > >>
> > > >> Using a per-fence spinlock allows completely decoupling spinlock
> > > >> producer and consumer life times, simplifying the handling in most
> > > >> use cases.
> > > >
> > > > maybe we should have the lock_class_key at the dma_buf_ops level and
> > > > have this lockdep_set_class() automated in __dma_fence_init().
> > >
> > > The dma_fence_chain() and dma_fence_array() containers are the only
> > > ones who are allowed to nest the lock with other dma_fences. E.g. we
> > > have WARN_ON()s in place which fire when you try to stitch together
> > > something which won't work.
> > >
> > > So everybody else should get a lockdep warning when they try to do
> > > nasty things like this because you really can't guarantee lock order
> > > between different dma_fence implementations.
> >
> > Okay, that makes sense.
>
> Yes, I agree with Christian's reasoning - chains / arrays is the only
> case where nesting should be allowed. Also if we assigned a key for
> every inline lock we'd quickly exhaust the number of lockdep keys.
The suggestion was to have a key per-dma_buf_ops implementation, not
per-lock ;-). Anyway, Christian's argument already convinced me, so
this is moot.
^ permalink raw reply [flat|nested] 8+ messages in thread