Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs

Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs

[Claude Code Review Bot]

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

Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs

[Claude Code Review Bot]

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

[PATCH v3] dma-fence: Clarify external lock use case in dma_fence_init() docs

[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.

Reword both comments to better describe the legacy use cases where a
shared lock was needed.

Signed-off-by: Maíra Canal <mcanal@igalia.com>

---

v1 -> v2: https://lore.kernel.org/dri-devel/20260411185756.1887119-4-mcanal@igalia.com/

- Be more explicit about not allowing new users to use an external lock.
- De-duplicate the explanation in dma_fence_init64() by pointing to the
  dma_fence_init() documentation.

v2 -> v3: https://lore.kernel.org/dri-devel/20260419134943.54833-2-mcanal@igalia.com/T/

- Apply Christian's suggestion with small readability improvements.

---
 drivers/dma-buf/dma-fence.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index b3bfa6943a8e..c7ea1e75d38a 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1102,9 +1102,12 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
  * context and seqno are used for easy comparison between fences, allowing
  * 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.
+ * External locks are a relic of legacy use cases that needed a shared lock
+ * to serialize signaling when no out-of-order signaling was possible through
+ * &dma_fence_ops.signaled. Drivers have abandoned this concept since the
+ * introduction of the callback, but the external lock is still around. New
+ * users MUST NOT use external locks, as they force the issuer to outlive all
+ * fences that reference the lock.
  */
 void
 dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
@@ -1129,9 +1132,8 @@ EXPORT_SYMBOL(dma_fence_init);
  * Context and seqno are used for easy comparison between fences, allowing
  * 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.
+ * New users MUST NOT use external locks. Check the documentation in
+ * dma_fence_init() to understand the motives behind the legacy use cases.
  */
 void
 dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
-- 
2.54.0

Re: [PATCH v3] dma-fence: Clarify external lock use case in dma_fence_init() docs

[Christian König]

On 5/31/26 14:49, Maíra Canal wrote:
> 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.
> 
> Reword both comments to better describe the legacy use cases where a
> shared lock was needed.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

I'm going to push this to drm-misc-next.

Thanks,
Christian.

> 
> ---
> 
> v1 -> v2: https://lore.kernel.org/dri-devel/20260411185756.1887119-4-mcanal@igalia.com/
> 
> - Be more explicit about not allowing new users to use an external lock.
> - De-duplicate the explanation in dma_fence_init64() by pointing to the
>   dma_fence_init() documentation.
> 
> v2 -> v3: https://lore.kernel.org/dri-devel/20260419134943.54833-2-mcanal@igalia.com/T/
> 
> - Apply Christian's suggestion with small readability improvements.
> 
> ---
>  drivers/dma-buf/dma-fence.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index b3bfa6943a8e..c7ea1e75d38a 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -1102,9 +1102,12 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>   * context and seqno are used for easy comparison between fences, allowing
>   * 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.
> + * External locks are a relic of legacy use cases that needed a shared lock
> + * to serialize signaling when no out-of-order signaling was possible through
> + * &dma_fence_ops.signaled. Drivers have abandoned this concept since the
> + * introduction of the callback, but the external lock is still around. New
> + * users MUST NOT use external locks, as they force the issuer to outlive all
> + * fences that reference the lock.
>   */
>  void
>  dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
> @@ -1129,9 +1132,8 @@ EXPORT_SYMBOL(dma_fence_init);
>   * Context and seqno are used for easy comparison between fences, allowing
>   * 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.
> + * New users MUST NOT use external locks. Check the documentation in
> + * dma_fence_init() to understand the motives behind the legacy use cases.
>   */
>  void
>  dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,

Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs

[Claude Code Review Bot]

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: 2
Reviewed: 2026-06-04T14:48:33.627035

---

This is a single documentation-only patch (v3) that improves the kerneldoc comments for `dma_fence_init()` and `dma_fence_init64()` in `drivers/dma-buf/dma-fence.c`. The motivation is clear and correct: the existing wording ("prevent multiple fences from signaling out of order") is misleading because a shared spinlock cannot actually enforce signaling order — that's the driver's responsibility. The patch rewrites the comment to explain the *actual* legacy rationale (serializing signaling when no out-of-order signaling was possible via `dma_fence_ops.signaled`) and adds a strong "MUST NOT" prohibition for new users.

The patch is well-scoped, addresses only the misleading documentation, and the de-duplication approach for `dma_fence_init64()` (pointing to `dma_fence_init()` docs rather than repeating the explanation) is sensible.

**Verdict: Looks good.** No functional changes, documentation improvement is accurate and clearly worded. Minor observations below.

---
Generated by Claude Code Patch Reviewer

Claude review: dma-fence: Clarify external lock use case in dma_fence_init() docs

[Claude Code Review Bot]

Patch Review

**Correctness of the new wording:**

The replacement text for `dma_fence_init()`:

```
+ * External locks are a relic of legacy use cases that needed a shared lock
+ * to serialize signaling when no out-of-order signaling was possible through
+ * &dma_fence_ops.signaled. Drivers have abandoned this concept since the
+ * introduction of the callback, but the external lock is still around. New
+ * users MUST NOT use external locks, as they force the issuer to outlive all
+ * fences that reference the lock.
```

This is accurate. The old comment implied the shared lock *prevented* out-of-order signaling, but in reality signaling order is the driver's responsibility. The new text correctly identifies the actual purpose (serializing signaling) and the actual problem with external locks (lifetime coupling — the issuer must outlive all fences). The reference to `&dma_fence_ops.signaled` as the mechanism that made external locks unnecessary is technically sound, as the `.signaled` callback allows the fence framework to peek at fence state without holding the shared lock.

**De-duplication in `dma_fence_init64()`:**

```
+ * New users MUST NOT use external locks. Check the documentation in
+ * dma_fence_init() to understand the motives behind the legacy use cases.
```

This is a reasonable approach. One very minor nit: kerneldoc cross-references typically use `&function()` or are more explicit. The plain text "dma_fence_init()" will render fine in the generated docs, but using `dma_fence_init()` as-is is conventional enough and readable. Not worth a respin.

**Minor observation:** The phrase "when no out-of-order signaling was possible through `&dma_fence_ops.signaled`" could be read two ways — either "when the `.signaled` callback couldn't handle out-of-order signaling" or "when out-of-order signaling was not possible [and they relied on] `.signaled`". In context, the intended meaning (the former) is clear enough, especially for the audience of dma-fence developers.

**No issues found.** This is a clean documentation improvement with correct technical content. 

Reviewed-by recommendation: **Accept as-is.**

---
Generated by Claude Code Patch Reviewer