Re: [PATCH v6 1/1] rust: introduce abstractions for fwctl

[Alexandre Courbot]

On Mon Jul 6, 2026 at 9:00 PM JST, Gary Guo wrote:
> On Mon Jul 6, 2026 at 5:19 AM BST, Alexandre Courbot wrote:
>> Hi Zhi,
>>
>> This is looking pretty good to me overall, a few remarks/questions
>> below. None of these should require big changes.
>>
>> On Tue Jun 30, 2026 at 12:01 AM JST, Zhi Wang wrote:
>> <...>
>>> +/// A fwctl device.
>>> +///
>>> +/// `#[repr(C)]` with the `fwctl_device` at offset 0, matching the C `fwctl_alloc_device()` layout
>>> +/// convention. Contains a pointer to the [`Registration`]'s data, set at registration time and
>>> +/// cleared on unregistration.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - `dev` is embedded at offset 0 and is initialised by fwctl.
>>> +/// - The fwctl refcount owns the allocation lifetime.
>>> +/// - `registration_data` is either `NonNull::dangling()` (before registration / after
>>> +///   unregistration) or points to valid data owned by the [`Registration`].
>>> +#[repr(C)]
>>> +pub struct Device<T: Operations> {
>>> +    dev: Opaque<bindings::fwctl_device>,
>>> +    registration_data: UnsafeCell<NonNull<T::RegistrationData<'static>>>,
>>> +}
>>> +
>>> +impl<T: Operations> Device<T> {
>>> +    /// Allocate a new fwctl device.
>>> +    ///
>>> +    /// Returns an [`ARef`] that can be passed to [`Registration::new()`]
>>> +    /// to make the device visible to userspace.
>>> +    pub fn new(parent: &device::Device<device::Bound>) -> Result<ARef<Self>> {
>>> +        const_assert!(
>>> +            core::mem::offset_of!(Self, dev) == 0,
>>> +            "struct fwctl_device must be at offset 0"
>>> +        );
>>> +
>>> +        let ops = core::ptr::from_ref::<bindings::fwctl_ops>(&VTable::<T>::VTABLE).cast_mut();
>>> +
>>> +        // SAFETY: `ops` is static, `parent` is bound, and `size` covers the full `Device<T>`.
>>> +        let raw = unsafe {
>>> +            bindings::_fwctl_alloc_device(parent.as_raw(), ops, core::mem::size_of::<Self>())
>>> +        };
>>> +
>>> +        if raw.is_null() {
>>> +            return Err(ENOMEM);
>>> +        }
>>> +
>>> +        let this = raw.cast::<Self>();
>>> +
>>> +        // INVARIANT: Set `registration_data` to dangling (no registration yet).
>>> +        // SAFETY: `this` points to the allocation just returned by fwctl.
>>> +        unsafe {
>>> +            core::ptr::addr_of_mut!((*this).registration_data)
>>> +                .write(UnsafeCell::new(NonNull::dangling()));
>>> +        };
>>> +
>>> +        // SAFETY: `raw` owns the initial reference.
>>> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(this)) })
>>
>> How about replacing from `if raw.is_null() ...` with something more
>> functional-style:
>>
>>     NonNull::new(raw.cast::<Self>())
>>         .map(|this| {
>>             // INVARIANT: Set `registration_data` to dangling (no registration yet).
>>             // SAFETY: `this` points to the allocation just returned by fwctl.
>>             unsafe {
>>                 (&raw mut (*this.as_ptr()).registration_data)
>>                     .write(UnsafeCell::new(NonNull::dangling()))
>>             };
>>             // SAFETY: `this` owns the initial reference.
>>             unsafe { ARef::from_raw(this) }
>>         })
>>         .ok_or(ENOMEM)
>
> I would rather do
>
>     let this = NonNull::new(raw.cast::<Self>()).ok_or(ENOMEM);
>     // use this
>
> for a more linear style. Map shouldn't be needed.

That works too, the main thing being using `NonNull` early on to avoid
the `new_unchecked`.