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

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Jun 18, 2024 at 12:50:45AM +0200, Danilo Krummrich wrote:
[...]
> >     /// 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		
> 
> I think in the `Page` case this is useful to create `Page` references from
> previously allocated memory.
> 
> In the case of `Firmware`, I agree it makes sense to use it once we have it,
> but other than for consistency, is there any advantage?
> 

To help people build future abstraction easier (and make review easier
as well). But I'm also waiting for a third use case, yes, I usually
wait for 3 cases to begin thinking about generalization ;-)

> > 
> > 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.
> 
> I think there's not too much to add here in the future, once we got an allocator
> API (I should get back to that soon), I want to add a method that copies the
> data to a new buffer allocated with a given allocator. And maybe we want to
> support a few other request_firmware_* functions in the future, but none of that
> should require the above abstraction.
> 

Thank you!

> > 
> > > +impl Firmware {
> > > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > +        let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> > > +        let pfw: *mut *mut bindings::firmware = &mut fw;
> > > +
> > > +        // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer.
> > > +        // `name` and `dev` are valid as by their type invariants.
> > > +        let ret = unsafe { func(pfw as _, name.as_char_ptr(), dev.as_raw()) };
> > > +        if ret != 0 {
> > > +            return Err(Error::from_errno(ret));
> > > +        }
> > > +
> > > +        // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
> > > +        // valid pointer to `bindings::firmware`.
> > > +        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> > > +    }
> > > +
> > > +    /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > > +    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > > +        Self::request_internal(name, dev, bindings::request_firmware)
> > > +    }
> > > +
> > > +    /// Send a request for an optional firmware module. See also
> > > +    /// `bindings::firmware_request_nowarn`.
> > > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > > +        Self::request_internal(name, dev, bindings::firmware_request_nowarn)
> > > +    }
> > > +
> > > +    fn as_raw(&self) -> *mut bindings::firmware {
> > > +        self.0.as_ptr()
> > > +    }
> > > +
> > > +    /// Returns the size of the requested firmware in bytes.
> > > +    pub fn size(&self) -> usize {
> > > +        // SAFETY: Safe by the type invariant.
> > > +        unsafe { (*self.as_raw()).size }
> > > +    }
> > > +
> > > +    /// Returns the requested firmware as `&[u8]`.
> > > +    pub fn data(&self) -> &[u8] {
> > > +        // SAFETY: Safe by the type invariant. Additionally, `bindings::firmware` guarantees, if
> > 
> > Does this "Safe by the type invariant" also covers the following safe
> > requirement of `from_raw_parts`?
> > 
> > 	The memory referenced by the returned slice must not be mutated for the duration of lifetime 'a, except inside an UnsafeCell.
> > 
> > in that `&[u8]` has the same lifetime as `&self`, and as long as
> > `&self` exists, no function can touch the inner `data`? If so, I
> > probably want to call this out.
> 
> Yes, nothing should ever modify the firmware buffer after it has been requested
> successfully. I can add this to the type invariant.
> 

Oh, you have an even easier (stronger) type invariant. Yes, please add
it and use it here. Thanks!

Regards,
Boqun

> > 
> > Regards,
> > Boqun
> > 
> > > +        // successfully requested, that `bindings::firmware::data` has a size of
> > > +        // `bindings::firmware::size` bytes.
> > > +        unsafe { core::slice::from_raw_parts((*self.as_raw()).data, self.size()) }
> > > +    }
> > > +}
> > > +
> > > +impl Drop for Firmware {
> > > +    fn drop(&mut self) {
> > > +        // SAFETY: Safe by the type invariant.
> > > +        unsafe { bindings::release_firmware(self.as_raw()) };
> > > +    }
> > > +}
> > > +
> > > +// SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
> > > +// any thread.
> > > +unsafe impl Send for Firmware {}
> > > +
> > > +// SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
> > > +// be used from any thread.
> > > +unsafe impl Sync for Firmware {}
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index dd1207f1a873..7707cb013ce9 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -30,6 +30,8 @@
> > >  mod build_assert;
> > >  pub mod device;
> > >  pub mod error;
> > > +#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> > > +pub mod firmware;
> > >  pub mod init;
> > >  pub mod ioctl;
> > >  #[cfg(CONFIG_KUNIT)]
> > > -- 
> > > 2.45.1
> > > 
> > 
>