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: drm/tyr: Add generic slot manager
Date: Tue, 03 Mar 2026 12:48:19 +1000
Message-ID: <review-patch7-20260302232500.244489-8-deborah.brouwer@collabora.com>
In-Reply-To: <20260302232500.244489-8-deborah.brouwer@collabora.com>
References: <20260302232500.244489-1-deborah.brouwer@collabora.com> <20260302232500.244489-8-deborah.brouwer@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Author:** Boris Brezillon

This is the most architecturally significant patch. The `SlotManager` abstr=
action is clean and well-documented.

A few observations:

1. In `idle_slot()`, the seat is read and then immediately overwritten:

```rust
+    fn idle_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, M=
AX_SLOTS>) -> Result {
+        let slot =3D take(&mut self.slots[slot_idx]);
+
+        if let Slot::Active(slot_info) =3D slot {
+            self.slots[slot_idx] =3D Slot::Idle(SlotInfo {
+                slot_data: slot_info.slot_data,
+                seqno: slot_info.seqno,
+            })
+        };
+
+        *locked_seat.access_mut(self) =3D match locked_seat.access(self) {
```

The `access()` followed immediately by `access_mut()` on the same `LockedBy=
` works because `access()` returns a reference that is dropped before `acce=
ss_mut()` is called (due to the `match` expression borrowing temporarily). =
But this pattern is subtle. It effectively copies the `SeatInfo` fields. Th=
is is fine since `SeatInfo` is `Copy`-like (contains `u8` and `u64`), but m=
aking `SeatInfo` derive `Clone`/`Copy` and simplifying would be cleaner.

2. `use_seqno` starts at 1 and increments on each activation. With `u64`, o=
verflow is practically impossible (would take ~584 years at 1 billion activ=
ations/second), so no concern there.

3. The `Deref`/`DerefMut` implementations on `SlotManager` exposing the inn=
er `T` are convenient but somewhat unusual =E2=80=94 they allow calling `Sl=
otOperations` methods directly on the manager. This is intentional for the =
`AsSlotManager` use case where the manager needs to call `as_enable()` etc.=
 directly.

4. `record_active_slot` returns `Result` but can never fail =E2=80=94 it al=
ways returns `Ok(())`. Minor cleanup opportunity.

---
Generated by Claude Code Patch Reviewer