Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Zhi Wang]

On 30.10.2025 18.22, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> +    /// Allocate and register a new fwctl device under the given parent device.
>> +    pub fn new(parent: &device::Device) -> Result<Self> {
>> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> +        // and initializes its embedded `struct device`.
>> +        let dev = unsafe {
>> +            bindings::_fwctl_alloc_device(
>> +                parent.as_raw(),
>> +                ops,
>> +                core::mem::size_of::<bindings::fwctl_device>(),
>> +            )
>> +        };
>> +
>> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
> 
> This is a Bound device, not just any device.
> 

Will do this in the next re-spin. It needs extra abstraction around fwctl_device.

>> +        if ret != 0 {
>> +            // SAFETY: If registration fails, release the allocated fwctl_device().
>> +            unsafe {
>> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
> 
> ?? Don't open code fwctl_put() - it should be called directly?

fwctl_put() is a inline function and it seem opened by the compiler when bindings are
generated. 

$ grep -R fwctl_put *
$ pwd
/home/inno/drm-rust/rust/bindings
$ grep -R put_device *
bindings_generated.rs:    pub fn put_device(dev: *mut device);

Hehe. I am open to options.

Z.
> 
>> +            }
>> +            return Err(Error::from_errno(ret));
>> +        }
>> +
>> +        Ok(Self {
>> +            fwctl_dev: dev,
>> +            _marker: PhantomData,
>> +        })
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
>> +        self.fwctl_dev.as_ptr()
>> +    }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
> 
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
> 
>> +        unsafe {
>> +            bindings::fwctl_unregister(self.as_raw());
>> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
> 
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
> 
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?
> 
> Jason