* [PATCH] dma-fence: Clarify external lock use case in dma_fence_init() docs
@ 2026-04-11 18:47 Maíra Canal
2026-04-11 22:49 ` Claude review: " Claude Code Review Bot
2026-04-11 22:49 ` Claude Code Review Bot
0 siblings, 2 replies; 3+ messages in thread
From: Maíra Canal @ 2026-04-11 18:47 UTC (permalink / raw)
To: Christian König, Sumit Semwal, Boris Brezillon,
Tvrtko Ursulin, Philipp Stanner
Cc: dri-devel, linaro-mm-sig, kernel-dev, Maíra Canal
The kerneldoc comment on dma_fence_init() and dma_fence_init64() describe
the legacy reason to pass an external lock as a need to prevent multiple
fences "from signaling out of order". However, this wording is a bit
misleading: a shared spinlock does not (and cannot) prevent the signaler
from signaling out of order. Signaling order is the driver's responsibility
regardless of whether the lock is shared or per-fence.
What a shared lock actually provides is serialization of signaling and
observation across fences in a given context, so that observers never
see a later fence signaled while an earlier one is not.
Reword both comments to describe this more accurately.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
Hi,
While reading the documentation, I found this particular paragraph quite
hard to understand. As I understand it, locks don't enforce order, only
serialization, but the paragraph seems to communicate the other way around.
Due to that, I had the impression that the current wording can be
misleading for driver developers.
I'm proposing a new wording to better describe the use case of the
external lock based on my understanding, but it would be great to hear
the feedback and suggestions from more experienced developers who might
have more insight about these legacy use cases.
Best regards,
- Maíra
drivers/dma-buf/dma-fence.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 1826ba73094c..bdc29d1c1b5c 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1102,8 +1102,10 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
* to check which fence is later by simply using dma_fence_later().
*
* It is strongly discouraged to provide an external lock because this couples
- * lock and fence life time. This is only allowed for legacy use cases when
- * multiple fences need to be prevented from signaling out of order.
+ * lock and fence lifetime. This is only allowed for legacy use cases that need
+ * a shared lock to serialize signaling and observation of fences within a
+ * context, so that observers never see a later fence signaled while an earlier
+ * one isn't.
*/
void
dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
@@ -1129,8 +1131,10 @@ EXPORT_SYMBOL(dma_fence_init);
* to check which fence is later by simply using dma_fence_later().
*
* It is strongly discouraged to provide an external lock because this couples
- * lock and fence life time. This is only allowed for legacy use cases when
- * multiple fences need to be prevented from signaling out of order.
+ * lock and fence lifetime. This is only allowed for legacy use cases that need
+ * a shared lock to serialize signaling and observation of fences within a
+ * context, so that observers never see a later fence signaled while an earlier
+ * one isn't.
*/
void
dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
--
2.53.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs
2026-04-11 18:47 [PATCH] dma-fence: Clarify external lock use case in dma_fence_init() docs Maíra Canal
@ 2026-04-11 22:49 ` Claude Code Review Bot
2026-04-11 22:49 ` Claude Code Review Bot
1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-11 22:49 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: dma-fence: Clarify external lock use case in dma_fence_init() docs
Author: =?UTF-8?q?Ma=C3=ADra=20Canal?= <mcanal@igalia.com>
Patches: 1
Reviewed: 2026-04-12T08:49:37.991345
---
This is a single-patch documentation-only change to the `dma_fence_init()` and `dma_fence_init64()` kerneldoc comments in `drivers/dma-buf/dma-fence.c`. The change is small, well-motivated, and technically accurate.
The existing wording says a shared lock prevents fences "from signaling out of order," which implies the lock somehow enforces ordering. As the author correctly notes, a spinlock cannot enforce ordering — it can only serialize. The driver is still responsible for signaling fences in the correct order. What the shared lock *does* provide is that an observer holding the lock will never see a state where a later fence is signaled while an earlier one is not (because the signaling operations are serialized under the same lock).
The new wording is a clear improvement. No functional code is changed.
**Verdict: Looks good.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 3+ messages in thread
* Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs
2026-04-11 18:47 [PATCH] dma-fence: Clarify external lock use case in dma_fence_init() docs Maíra Canal
2026-04-11 22:49 ` Claude review: " Claude Code Review Bot
@ 2026-04-11 22:49 ` Claude Code Review Bot
1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-11 22:49 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Assessment: Good patch, minor nit.**
The commit message explains the rationale well:
> a shared spinlock does not (and cannot) prevent the signaler from signaling out of order. Signaling order is the driver's responsibility regardless of whether the lock is shared or per-fence.
>
> What a shared lock actually provides is serialization of signaling and observation across fences in a given context, so that observers never see a later fence signaled while an earlier one is not.
This is correct. Looking at `dma_fence_signal()` (line 487), it acquires the fence lock, then calls `dma_fence_signal_timestamp_locked()`. If two fences share a lock, signaling one will block until the other's signal+callback processing completes, ensuring observers under the same lock see a consistent ordering.
The code change itself:
```c
- * lock and fence life time. This is only allowed for legacy use cases when
- * multiple fences need to be prevented from signaling out of order.
+ * lock and fence lifetime. This is only allowed for legacy use cases that need
+ * a shared lock to serialize signaling and observation of fences within a
+ * context, so that observers never see a later fence signaled while an earlier
+ * one isn't.
```
This is applied identically to both `dma_fence_init()` and `dma_fence_init64()`, which is correct since they share the same doc structure.
**Minor nit:** The fix of "life time" → "lifetime" is a nice bonus typo fix. It might be worth calling that out in the commit message so reviewers know it's intentional, though it's trivially obvious.
**One small wording suggestion:** The phrase "one isn't" at the end is slightly informal for kernel documentation. Consider "one is not" for consistency with the formal tone of the rest of the doc comment. This is purely stylistic and not a blocking concern.
**Reviewed-by worthy: Yes.** The patch is correct, well-explained, and improves clarity. The cover letter also shows good engagement with the community by inviting feedback on the interpretation.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-11 22:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-11 18:47 [PATCH] dma-fence: Clarify external lock use case in dma_fence_init() docs Maíra Canal
2026-04-11 22:49 ` Claude review: " Claude Code Review Bot
2026-04-11 22:49 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox