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

["Gary Guo" <gary@garyguo.net>]

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.

Best,
Gary

>
> It's not just cosmetic: it removes one call to an unsafe method
> (`NonNull::new_unchecked`), carries the fact that `this` is not NULL in
> the type system from the get-go (instead of relying on an explicit
> check), and makes the transition steps from the raw pointer to the
> `ARef` more explicit.