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>
---
 drivers/base/firmware_loader/Kconfig |  7 ++
 rust/bindings/bindings_helper.h      |  1 +
 rust/kernel/firmware.rs              | 98 ++++++++++++++++++++++++++++
 rust/kernel/lib.rs                   |  2 +
 4 files changed, 108 insertions(+)
 create mode 100644 rust/kernel/firmware.rs

diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
index 5ca00e02fe82..a03701674265 100644
--- a/drivers/base/firmware_loader/Kconfig
+++ b/drivers/base/firmware_loader/Kconfig
@@ -37,6 +37,13 @@ config FW_LOADER_DEBUG
 	  SHA256 checksums to the kernel log for each firmware file that is
 	  loaded.
 
+config RUST_FW_LOADER_ABSTRACTIONS
+	bool "Rust Firmware Loader abstractions"
+	depends on RUST
+	depends on FW_LOADER=y
+	help
+	  This enables the Rust abstractions for the firmware loader API.
+
 if FW_LOADER
 
 config FW_LOADER_PAGED_BUF
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ddb5644d4fd9..18a3f05115cb 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -9,6 +9,7 @@
 #include <kunit/test.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
+#include <linux/firmware.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
new file mode 100644
index 000000000000..05a4f84cfd42
--- /dev/null
+++ b/rust/kernel/firmware.rs
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Firmware abstraction
+//!
+//! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h")
+
+use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
+use core::ptr::NonNull;
+
+// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
+// `firmware_request_platform`, `bindings::request_firmware_direct`
+type FwFunc =
+    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32;
+
+/// Abstraction around a C `struct firmware`.
+///
+/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
+/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
+/// `&[u8]`. The firmware is released once [`Firmware`] is dropped.
+///
+/// # Invariants
+///
+/// The pointer is valid, and has ownership over the instance of `struct firmware`.
+///
+/// # 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>);
+
+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
+        // 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

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>

I don't speak Rust, so if we're gonna add this, I'd just ask you also
become a firmware loader maintainer, willing to spend time to help
review both C and Rust code. Is that too much to ask?

  Luis

Hi Luis,

On 6/18/24 21:47, Luis Chamberlain wrote:
> 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>
> 
> I don't speak Rust, so if we're gonna add this, I'd just ask you also
> become a firmware loader maintainer, willing to spend time to help
> review both C and Rust code. Is that too much to ask?

That is fine, I'm happy to help out. I think Greg applied the patch already.
But I can send a separate one adding the corresponding entry later on.

- Danilo

> 
>    Luis
>

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>
> ---
>  drivers/base/firmware_loader/Kconfig |  7 ++
>  rust/bindings/bindings_helper.h      |  1 +
>  rust/kernel/firmware.rs              | 98 ++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                   |  2 +
>  4 files changed, 108 insertions(+)
>  create mode 100644 rust/kernel/firmware.rs
> 
> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> index 5ca00e02fe82..a03701674265 100644
> --- a/drivers/base/firmware_loader/Kconfig
> +++ b/drivers/base/firmware_loader/Kconfig
> @@ -37,6 +37,13 @@ config FW_LOADER_DEBUG
>  	  SHA256 checksums to the kernel log for each firmware file that is
>  	  loaded.
>  
> +config RUST_FW_LOADER_ABSTRACTIONS
> +	bool "Rust Firmware Loader abstractions"
> +	depends on RUST
> +	depends on FW_LOADER=y
> +	help
> +	  This enables the Rust abstractions for the firmware loader API.
> +
>  if FW_LOADER
>  
>  config FW_LOADER_PAGED_BUF
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index ddb5644d4fd9..18a3f05115cb 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
>  #include <kunit/test.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> +#include <linux/firmware.h>
>  #include <linux/jiffies.h>
>  #include <linux/mdio.h>
>  #include <linux/phy.h>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> new file mode 100644
> index 000000000000..05a4f84cfd42
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Firmware abstraction
> +//!
> +//! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h")
> +
> +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> +use core::ptr::NonNull;
> +
> +// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> +// `firmware_request_platform`, `bindings::request_firmware_direct`
> +type FwFunc =
> +    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32;
> +
> +/// Abstraction around a C `struct firmware`.
> +///
> +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> +/// `&[u8]`. The firmware is released once [`Firmware`] is dropped.
> +///
> +/// # Invariants
> +///
> +/// The pointer is valid, and has ownership over the instance of `struct firmware`.
> +///
> +/// # 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.

> +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.

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
>

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

On Mon, Jun 17, 2024 at 03:05:32PM -0700, Boqun Feng wrote:
> 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>
> > ---
> >  drivers/base/firmware_loader/Kconfig |  7 ++
> >  rust/bindings/bindings_helper.h      |  1 +
> >  rust/kernel/firmware.rs              | 98 ++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs                   |  2 +
> >  4 files changed, 108 insertions(+)
> >  create mode 100644 rust/kernel/firmware.rs
> > 
> > diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> > index 5ca00e02fe82..a03701674265 100644
> > --- a/drivers/base/firmware_loader/Kconfig
> > +++ b/drivers/base/firmware_loader/Kconfig
> > @@ -37,6 +37,13 @@ config FW_LOADER_DEBUG
> >  	  SHA256 checksums to the kernel log for each firmware file that is
> >  	  loaded.
> >  
> > +config RUST_FW_LOADER_ABSTRACTIONS
> > +	bool "Rust Firmware Loader abstractions"
> > +	depends on RUST
> > +	depends on FW_LOADER=y
> > +	help
> > +	  This enables the Rust abstractions for the firmware loader API.
> > +
> >  if FW_LOADER
> >  
> >  config FW_LOADER_PAGED_BUF
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index ddb5644d4fd9..18a3f05115cb 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -9,6 +9,7 @@
> >  #include <kunit/test.h>
> >  #include <linux/errname.h>
> >  #include <linux/ethtool.h>
> > +#include <linux/firmware.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/mdio.h>
> >  #include <linux/phy.h>
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > new file mode 100644
> > index 000000000000..05a4f84cfd42
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Firmware abstraction
> > +//!
> > +//! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h")
> > +
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> > +use core::ptr::NonNull;
> > +
> > +// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> > +// `firmware_request_platform`, `bindings::request_firmware_direct`
> > +type FwFunc =
> > +    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32;
> > +
> > +/// Abstraction around a C `struct firmware`.
> > +///
> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> > +/// `&[u8]`. The firmware is released once [`Firmware`] is dropped.
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer is valid, and has ownership over the instance of `struct firmware`.
> > +///
> > +/// # 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		

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?

> 
> 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.

> 
> > +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.

> 
> 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
> > 
>

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
> > > 
> > 
>