Re: [PATCH v3 2/2] rust: add firmware abstractions

[Andreas Hindborg <a.hindborg@samsung.com>]

Boqun Feng <boqun.feng@gmail.com> writes:

> On Mon, Jun 17, 2024 at 10:29:41PM +0200, Danilo Krummrich wrote:
>> Add an abstraction around the kernels firmware API to request firmware
>> images. The abstraction provides functions to access the firmware's size
>> and backing buffer.
>> 
>> The firmware is released once the abstraction instance is dropped.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

..

>> +/// # Examples
>> +///
>> +/// ```
>> +/// # use kernel::{c_str, device::Device, firmware::Firmware};
>> +///
>> +/// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
>> +/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
>> +///
>> +/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev).unwrap();
>> +/// let blob = fw.data();
>> +/// ```
>> +pub struct Firmware(NonNull<bindings::firmware>);
>> +
>
> I feel like eventually we need a very simple smart pointer type for
> these case, for example:
>
>     /// A smart pointer owns the underlying data.
>     pub struct Owned<T: Ownable> {
>         ptr: NonNull<T>,
>     }
>
>     impl<T: Ownable> Owned<T> {
>         /// # Safety
> 	/// `ptr` needs to be a valid pointer, and it should be the
> 	/// unique owner to the object, in other words, no one can touch
> 	/// or free the underlying data.
>         pub unsafe to_owned(ptr: *mut T) -> Self {
> 	    // SAFETY: Per function safety requirement.
> 	    Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> 	}
>
> 	/// other safe constructors are available if a initializer (impl
> 	/// Init) is provided
>     }
>
>     /// A Ownable type is a type that can be put into `Owned<T>`, and
>     /// when `Owned<T>` drops, `ptr_drop` will be called.
>     pub unsafe trait Ownable {
>         /// # Safety
> 	/// This could only be called in the `Owned::drop` function.
>         unsafe fn ptr_drop(ptr: *mut Self);
>     }
>
>     impl<T: Ownable> Drop for Owned<T> {
>         fn drop(&mut self) {
> 	    /// SAFETY: In Owned<T>::drop.
> 	    unsafe {
> 	        <T as Ownable>::ptr_drop(self.as_mut_ptr());
> 	    }
> 	}
>     }
>
> we can implement Deref and DerefMut easily on `Owned<T>`. And then we
> could define Firmware as
>
>     #[repr(transparent)]
>     pub struct Firmware(Opaque<bindings::firmware>);
>
> and
>
>     unsafe impl Ownable for Firmware {
>         unsafe fn ptr_drop(ptr: *mut Self) {
> 	    // SAFETY: Per function safety, this is called in
> 	    // Owned::drop(), so `ptr` is a unique pointer to object,
> 	    // it's safe to release the firmware.
>             unsafe { bindings::release_firmware(ptr.cast()); }
>         }
>     }
>
> and the request_*() will return a `Result<Owned<Self>>`. 
>
> Alice mentioned the need of this in page as well:
>
> 	https://lore.kernel.org/rust-for-linux/CAH5fLgjrt0Ohj1qBv=GrqZumBTMQ1jbsKakChmxmG2JYDJEM8w@mail.gmail.com		
>
> Just bring it up while we are (maybe not? ;-)) at it. Also I would like
> to hear whether this would work for Firmware in the longer-term ;-) But
> yes, I'm not that worried about merging it as it is if others are all
> OK.

Please see [1] for an attempt at this pattern.


Best regards,
Andreas Hindborg


[1] https://lore.kernel.org/r/20250618-unique-ref-v11-0-49eadcdc0aa6@pm.me