From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: rust: add ordered workqueue wrapper
Date: Sat, 14 Mar 2026 07:15:47 +1000
Message-ID: <review-patch3-20260313091646.16938-4-work@onurozkan.dev>
In-Reply-To: <20260313091646.16938-4-work@onurozkan.dev>
References: <20260313091646.16938-1-work@onurozkan.dev> <20260313091646.16938-4-work@onurozkan.dev>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Concerns about the helper and API design.**

The C helper:

```c
+rust_helper_alloc_ordered_workqueue(const char *name, unsigned int flags)
+{
+	return alloc_ordered_workqueue("%s", flags, name);
+}
```

This correctly avoids format string issues by using `"%s"` as the format. G=
ood.

The `OrderedQueue` type:

```rust
+pub struct OrderedQueue(NonNull<bindings::workqueue_struct>);
```

1. **`enqueue` uses `Queue::from_raw`**: The `enqueue` method does:

```rust
+    pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
+    where
+        W: RawWorkItem<ID> + Send + 'static,
+    {
+        // SAFETY: `self.0` is valid while `self` is alive.
+        unsafe { Queue::from_raw(self.0.as_ptr()) }.enqueue(w)
+    }
```

This creates a temporary `Queue` reference from the raw pointer just to cal=
l `enqueue`. This works but means `OrderedQueue` doesn't expose other `Queu=
e` methods (like `try_spawn`). This is fine for the current use case but yo=
u might consider providing an `as_queue(&self) -> &Queue` method or impleme=
nting `Deref<Target=3DQueue>` for more general use. That said, keeping the =
API minimal is also reasonable for now.

2. **Drop safety comment**: The drop impl says:

```rust
+        // - `OrderedQueue` does not expose delayed scheduling API.
```

This safety comment seems tangential =E2=80=94 the key invariant for `destr=
oy_workqueue` is that the workqueue was allocated by `alloc_ordered_workque=
ue` and is being destroyed exactly once, which is guaranteed by `NonNull` o=
wnership semantics.

3. **No `flush_workqueue` method**: Before destruction, `destroy_workqueue`=
 in C already drains pending work. This is fine, but callers using `disable=
_work_sync` (patch 2) before dropping individual work items should be aware=
 of the ordering =E2=80=94 which patch 4 handles correctly.

---

---
Generated by Claude Code Patch Reviewer