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

Zhi Wang
2025-10-30T17:19:07Z

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