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              | 101 +++++++++++++++++++++++++++
 rust/kernel/lib.rs                   |   2 +
 4 files changed, 111 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..b55ea1b45368
--- /dev/null
+++ b/rust/kernel/firmware.rs
@@ -0,0 +1,101 @@
+// 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`.
+///
+/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
+/// is dropped.
+///
+/// # 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 Tue, 18 Jun 2024 17:48:35 +0200
Danilo Krummrich <dakr@redhat.com> 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              | 101 +++++++++++++++++++++++++++
>  rust/kernel/lib.rs                   |   2 +
>  4 files changed, 111 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..b55ea1b45368
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,101 @@
> +// 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`.
> +///
> +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
> +/// is dropped.
> +///
> +/// # 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()) };

This is line is unsound if this function is called with an arbitrary
FwFunc, therefore the safety comment should also mention that `func`
cannot be an arbitrary function and it must be one of
`request_firmware`, `firmware_request_nowarn`,
`firmware_request_platform`, `request_firmware_direct`, and this is
true because the function is not public and all users in the file
satisfy this safety precondition.


> +        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)]

On 6/20/24 15:36, Gary Guo wrote:
> On Tue, 18 Jun 2024 17:48:35 +0200
> Danilo Krummrich <dakr@redhat.com> 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              | 101 +++++++++++++++++++++++++++
>>   rust/kernel/lib.rs                   |   2 +
>>   4 files changed, 111 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..b55ea1b45368
>> --- /dev/null
>> +++ b/rust/kernel/firmware.rs
>> @@ -0,0 +1,101 @@
>> +// 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`.
>> +///
>> +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
>> +/// is dropped.
>> +///
>> +/// # 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()) };
> 
> This is line is unsound if this function is called with an arbitrary
> FwFunc, therefore the safety comment should also mention that `func`
> cannot be an arbitrary function and it must be one of
> `request_firmware`, `firmware_request_nowarn`,
> `firmware_request_platform`, `request_firmware_direct`, and this is

This is documented in the type definition of `FwFunc`. We can link to this invariant though
and explicitly mark it as such. Does that make sense?

- Danilo

> true because the function is not public and all users in the file
> satisfy this safety precondition.
> 
> 
>> +        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)]
>

On Thu, 20 Jun 2024 15:43:52 +0200
Danilo Krummrich <dakr@redhat.com> wrote:

> On 6/20/24 15:36, Gary Guo wrote:
> > On Tue, 18 Jun 2024 17:48:35 +0200
> > Danilo Krummrich <dakr@redhat.com> 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              | 101 +++++++++++++++++++++++++++
> >>   rust/kernel/lib.rs                   |   2 +
> >>   4 files changed, 111 insertions(+)
> >>   create mode 100644 rust/kernel/firmware.rs
> >>
> >> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> >> new file mode 100644
> >> index 000000000000..b55ea1b45368
> >> --- /dev/null
> >> +++ b/rust/kernel/firmware.rs
> >> @@ -0,0 +1,101 @@
> >> +// 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`.
> >> +///
> >> +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
> >> +/// is dropped.
> >> +///
> >> +/// # 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()) };  
> > 
> > This is line is unsound if this function is called with an arbitrary
> > FwFunc, therefore the safety comment should also mention that `func`
> > cannot be an arbitrary function and it must be one of
> > `request_firmware`, `firmware_request_nowarn`,
> > `firmware_request_platform`, `request_firmware_direct`, and this is  
> 
> This is documented in the type definition of `FwFunc`. We can link to this invariant though
> and explicitly mark it as such. Does that make sense?

You can't really attach an invariant to `FwFunc` because it's just a
type alias, although linking to the comment in `FwFunc` and mentioning
that all users are within the module is good to me.

Some other options are:
* New type over FwFunc and attach invariant
* Make this function unsafe and have this as a safety precondition

Both would make the safety comment making local reasoning rather than
file-level reasoning. Although I don't think either is necessary since
this is a small file. But we need to be explicit file-level reasoning
is used here.

Best,
Gary

> - Danilo
> 
> > true because the function is not public and all users in the file
> > satisfy this safety precondition.
> > 
> >   
> >> +        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 {}

On 18.06.24 17:48, 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              | 101 +++++++++++++++++++++++++++
>  rust/kernel/lib.rs                   |   2 +
>  4 files changed, 111 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..b55ea1b45368
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,101 @@
> +// 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`.
> +///
> +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
> +/// is dropped.

This can simply be "The `firmware`'s backing buffer is not modified."
Since I interpret "Once requested" as "Once created" and you are allowed
to break invariants as long as nobody can observe that.

> +///
> +/// # 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

I would not write "Safe by ...", since it is important to know what is
guaranteed by what. Instead I would write "self.as_raw() is valid by the
type invariant.".

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

Ditto.

---
Cheers,
Benno

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

Greg,

Benno's comments provide some nice hints to further improve the safety comments.
Since I was notified that those patches hit your tree already, how do you want
to proceed?

- Danilo

On Wed, Jun 19, 2024 at 08:58:02AM +0000, Benno Lossin wrote:
> On 18.06.24 17:48, 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              | 101 +++++++++++++++++++++++++++
> >  rust/kernel/lib.rs                   |   2 +
> >  4 files changed, 111 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..b55ea1b45368
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,101 @@
> > +// 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`.
> > +///
> > +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
> > +/// is dropped.
> 
> This can simply be "The `firmware`'s backing buffer is not modified."
> Since I interpret "Once requested" as "Once created" and you are allowed
> to break invariants as long as nobody can observe that.
> 
> > +///
> > +/// # 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
> 
> I would not write "Safe by ...", since it is important to know what is
> guaranteed by what. Instead I would write "self.as_raw() is valid by the
> type invariant.".
> 
> > +        // 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.
> 
> Ditto.
> 
> ---
> Cheers,
> Benno
> 
> > +        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 Wed, Jun 19, 2024 at 11:44:23AM +0200, Danilo Krummrich wrote:
> Greg,
> 
> Benno's comments provide some nice hints to further improve the safety comments.
> Since I was notified that those patches hit your tree already, how do you want
> to proceed?

Please start by not top-posting :)

Anyway, patches on top of what is in my tree is fine, these are just
comment updates, not any real broken issue to prevent the existing stuff
from existing.

thanks,

greg k-h

On Wed, Jun 19, 2024 at 12:43:55PM +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 11:44:23AM +0200, Danilo Krummrich wrote:
> > Greg,
> > 
> > Benno's comments provide some nice hints to further improve the safety comments.
> > Since I was notified that those patches hit your tree already, how do you want
> > to proceed?
> 
> Please start by not top-posting :)

Well, I guess it kinda made sense in this case, since I wasn't replying to any
of the comments specifically.

> 
> Anyway, patches on top of what is in my tree is fine, these are just
> comment updates, not any real broken issue to prevent the existing stuff
> from existing.

Ok, I'll send you another series for this including the changes for the
MAINTAINERS file.

- Danilo

> 
> thanks,
> 
> greg k-h
>

On Tue, Jun 18, 2024 at 05:48:35PM +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>

Acked-by: Boqun Feng <boqun.feng@gmail.com>

Thanks!

Regards,
Boqun

> ---
>  drivers/base/firmware_loader/Kconfig |   7 ++
>  rust/bindings/bindings_helper.h      |   1 +
>  rust/kernel/firmware.rs              | 101 +++++++++++++++++++++++++++
>  rust/kernel/lib.rs                   |   2 +
>  4 files changed, 111 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..b55ea1b45368
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,101 @@
> +// 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`.
> +///
> +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware`
> +/// is dropped.
> +///
> +/// # 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
>