Re: [PATCH v7 2/7] drm/tyr: add a generic slot manager

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware and software slot states diverge when `manager.activate()` fails after a successful lazy eviction.
- [Medium] Truncation of `slot_idx` to `u8` causes permanent leakage of hardware slots when `MAX_SLOTS > 256`.
- [Medium] Re-activating an already valid seat with new `slot_data` updates the software tracking but fails to reprogram the hardware, leading to mismatched eviction.
--

--- Patch [2]: [PATCH v7 2/7] drm/tyr: add a generic slot manager ---
commit aac93e6de5af8d0538293cb60a9c909446516880
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/tyr: add a generic slot manager
    
Introduce a generic slot manager to dynamically allocate limited hardware
slots to software "seats". It can be used for both address space (AS) and
command stream group (CSG) slots.
    
The slot manager initially assigns seats to its free slots. It will
continue to reuse the same slot for a seat, as long as another seat does
not start to use the slot in the interim.
    
When contention arises because all of the slots are allocated, the slot
manager will lazily evict and reuse slots that have become idle (if any).
    
The seat state is protected using the LockedBy pattern with the same lock
that guards the SlotManager. This ensures the seat state stays consistent
across slot operations.
    
Hardware specific behaviour is controlled through the SlotManager's
specific manager type that implements the `SlotOperations` trait.

> diff --git a/drivers/gpu/drm/tyr/slot.rs b/drivers/gpu/drm/tyr/slot.rs
> new file mode 100644
> index 0000000000000..5c266fcf30eab
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/slot.rs
> @@ -0,0 +1,378 @@
[ ... ]
> +pub(crate) struct SeatInfo {
> +    /// Slot used by this seat.
> +    ///
> +    /// This index is only valid if the slot pointed to by this index
> +    /// has its `SlotInfo::seqno` match `SeatInfo::seqno`. Otherwise,
> +    /// it means the object has been evicted from the hardware slot,
> +    /// and a new slot needs to be acquired to make this object
> +    /// resident again.
> +    slot: u8,
> +
> +    /// Sequence number encoding the last time this seat was active.
> +    /// We also use it to check if a slot is still bound to a seat.
> +    seqno: u64,
> +}
[ ... ]
> +impl<T: SlotOperations, const MAX_SLOTS: usize> SlotManager<T, MAX_SLOTS> {
> +    /// Creates a specific instance of a slot manager.
> +    pub(crate) fn new(manager: T, slot_count: usize) -> Result<Self> {
> +        if slot_count == 0 {
> +            pr_err!("Invalid slot count: 0");
> +            return Err(EINVAL);
> +        }
> +        if slot_count > MAX_SLOTS {
> +            pr_err!(
> +                "Slot count: {} cannot exceed MAX_SLOTS: {}",
> +                slot_count,
> +                MAX_SLOTS
> +            );
> +            return Err(EINVAL);
> +        }
> +        Ok(Self {
> +            manager,
> +            slot_count,
> +            slots: [const { SlotState::Free }; MAX_SLOTS],
> +            use_seqno: 1,
> +        })
> +    }
> +
> +    /// Records a slot as active for the given seat.
> +    fn record_active_slot(
> +        &mut self,
> +        slot_idx: usize,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        let cur_seqno = self.use_seqno;
> +
> +        *locked_seat.access_mut(self) = Seat::Active(SeatInfo {
> +            slot: slot_idx as u8,

[Severity: Medium]
Since the SlotManager::new() constructor accepts any usize for slot_count,
what happens if a driver instantiates this generic manager with > 256 slots?

In SeatInfo, the slot is stored as a u8:

    slot: u8,

When record_active_slot() assigns a slot, it unconditionally casts the index:

    slot: slot_idx as u8,

If an index >= 256 is used, wouldn't it silently truncate? During subsequent
operations, check_seat() would read the truncated index, fail the sequence
number validation against the wrong slot, and forcefully transition the seat
to NoSeat. Would the actual high-index hardware slot remain stuck in
SlotState::Active forever, causing a resource leak?

> +            seqno: cur_seqno,
> +        });
> +
> +        self.slots[slot_idx] = SlotState::Active(SlotInfo {
> +            slot_data,
> +            seqno: cur_seqno,
> +        });
> +
> +        self.use_seqno += 1;
> +        Ok(())
> +    }
[ ... ]
> +    /// Finds a slot for the given seat. A free slot is preferred, but if none
> +    /// are available, the oldest idle slot is evicted and reused. Otherwise, if
> +    /// there are no free or idle slots, return [`EBUSY`].
> +    fn allocate_slot(
> +        &mut self,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        let slots = &self.slots[..self.slot_count];
> +
> +        let mut idle_slot_idx = None;
> +        let mut idle_slot_seqno: u64 = 0;
> +
> +        for (slot_idx, slot) in slots.iter().enumerate() {
> +            match slot {
> +                SlotState::Free => {
> +                    return self.activate_slot(slot_idx, locked_seat, slot_data);
> +                }
> +                SlotState::Idle(slot_info) => {
> +                    if idle_slot_idx.is_none() || slot_info.seqno < idle_slot_seqno {
> +                        idle_slot_idx = Some(slot_idx);
> +                        idle_slot_seqno = slot_info.seqno;
> +                    }
> +                }
> +                SlotState::Active(_) => (),
> +            }
> +        }
> +
> +        match idle_slot_idx {
> +            Some(slot_idx) => {
> +                // Lazily evict idle slot just before it is reused.
> +                if let SlotState::Idle(slot_info) = &self.slots[slot_idx] {
> +                    self.manager.evict(slot_idx, &slot_info.slot_data)?;
> +                }
> +                self.activate_slot(slot_idx, locked_seat, slot_data)

[Severity: High]
What happens to the software state if self.manager.evict() succeeds, but
self.activate_slot() fails (e.g. due to a hardware timeout)?

If self.activate_slot() returns an error, the function returns early.
Crucially, the software state in self.slots[slot_idx] is not cleared to
SlotState::Free and remains SlotState::Idle(old_slot_info).

When the previous owner of that slot later calls activate(), won't
check_seat() see matching sequence numbers and leave the seat valid?

Could the seat then seamlessly transition to Active without calling
manager.activate(), resulting in the software thinking it is active while
the hardware is actually evicted?

> +            }
> +            None => Err(EBUSY),
> +        }
> +    }
[ ... ]
> +    /// Activate a resource on any available/reclaimable slot.
> +    pub(crate) fn activate(
> +        &mut self,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        self.check_seat(locked_seat);
> +        match locked_seat.access(self) {
> +            // If a seat still has a valid slot, just reuse the slot and refresh the bookkeeping.
> +            Seat::Active(seat_info) | Seat::Idle(seat_info) => {
> +                self.record_active_slot(seat_info.slot as usize, locked_seat, slot_data)

[Severity: Medium]
Can slot_data change between activations of the same seat?

If a seat is already Active or Idle, activate() calls record_active_slot()
directly. This updates the software tracking in self.slots[slot_idx] with
the newly provided slot_data, but skips calling self.manager.activate().

If the new slot_data differs from the data originally used to program the
hardware, won't the hardware continue operating with the old configuration?
Furthermore, when the slot is eventually evicted, would self.manager.evict()
be invoked with the newly updated slot_data, potentially passing incorrect
parameters to the hardware unprogramming routine?

> +            }
> +            _ => self.allocate_slot(locked_seat, slot_data),
> +        }
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-fw-boot-b4-v7-0-afd8dc1494dd@collabora.com?part=2