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

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Oct 30, 2025 at 5:22 PM CET, 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.

Indeed, the safety comment should mention this. And if it would it could not
justify it with the current code, since the function takes a &Device instead of
the required &Device<Bound> argument.

>> +        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?
>
>> +            }
>> +            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?

Yeah, such Registration structures of (class) devices should be wrapped into a
Devres container (i.e. Devres<fwctl::Registration>)to be able to provide this
guarantee. See also my other reply to this patch [1].

While not a class device, the auxiliary bus with its auxiliary::Registration
[2], is a good example.

Alternatively (or additionally), it can also be implemented in a way that the
driver does not get control over a Registration object at all, but once created
it is not accessible anymore and automatically dropped on parent device unbind.
This approach is used by cpufreq [3].

It always depends on whether a driver might want to drop the Registration
manually before device unbind.

[1] https://lore.kernel.org/lkml/DDVT5YA564C6.3HN9WCMQX49PC@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/auxiliary.rs?id=b0b7301b004301afe920b3d08caa6171dd3f4011#n304
[3] https://rust.docs.kernel.org/kernel/cpufreq/struct.Registration.html#method.new_foreign_owned