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

Danilo Krummrich
2025-10-30T17:21:13Z

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