Hi,
as agreed in [1] this is the separate series for the device and firmware
abstractions to unblock the inclusion of Fujita's PHY driver.
Originally, those patches were part of the patch series [2][3].
Changes in v2
=============
- use the 'srctree/' notation
- expand the existing documentation and make it more unambiguous
- use `NonNull` in `Firmware`
- generalize the `Firmware` request functions
- add missing safety comments to `Firmware`
[1] https://lore.kernel.org/rust-for-linux/2024060745-palatable-dragging-32d1@gregkh/
[2] https://lore.kernel.org/rust-for-linux/[email protected]/
[3] https://lore.kernel.org/rust-for-linux/[email protected]/
Danilo Krummrich (2):
rust: add abstraction for struct device
rust: add firmware abstractions
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 1 +
rust/kernel/device.rs | 97 +++++++++++++++++++++++++++++
rust/kernel/firmware.rs | 107 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
5 files changed, 208 insertions(+)
create mode 100644 rust/kernel/device.rs
create mode 100644 rust/kernel/firmware.rs
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
--
2.45.1
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 <[email protected]>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/firmware.rs | 107 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
3 files changed, 109 insertions(+)
create mode 100644 rust/kernel/firmware.rs
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..7ff4c325f670
--- /dev/null
+++ b/rust/kernel/firmware.rs
@@ -0,0 +1,107 @@
+// 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::firmware::Firmware;
+///
+/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
+/// driver_load_firmware(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)
+ }
+
+ /// Send a request for a firmware with platform-fw fallback. See also
+ /// `bindings::firmware_request_platform`.
+ pub fn request_platform(name: &CStr, dev: &Device) -> Result<Self> {
+ Self::request_internal(name, dev, bindings::firmware_request_platform)
+ }
+
+ /// Send a request for a firmware directly without usermode helper. See also
+ /// `bindings::request_firmware_direct`.
+ pub fn request_direct(name: &CStr, dev: &Device) -> Result<Self> {
+ Self::request_internal(name, dev, bindings::request_firmware_direct)
+ }
+
+ 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 as by the type invariant.
+ unsafe { (*self.as_raw()).size }
+ }
+
+ /// Returns the requested firmware as `&[u8]`.
+ pub fn data(&self) -> &[u8] {
+ // SAFETY: Safe as by the type invariant. Additionally, `bindings::firmware` guarantees, if
+ // sucessfully 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 as 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..5307aa45bb8d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -30,6 +30,7 @@
mod build_assert;
pub mod device;
pub mod error;
+pub mod firmware;
pub mod init;
pub mod ioctl;
#[cfg(CONFIG_KUNIT)]
--
2.45.1
On Mon, Jun 10, 2024 at 08:02:28PM +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 <[email protected]>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/firmware.rs | 107 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 3 files changed, 109 insertions(+)
> create mode 100644 rust/kernel/firmware.rs
>
> 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..7ff4c325f670
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,107 @@
> +// 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::firmware::Firmware;
> +///
> +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> +/// driver_load_firmware(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)
> + }
How does this handle when CONFIG_FW_LOADER is not enabled? Why are you
building these bindings if that option is not checked?
> +
> + /// 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)
> + }
> +
> + /// Send a request for a firmware with platform-fw fallback. See also
> + /// `bindings::firmware_request_platform`.
> + pub fn request_platform(name: &CStr, dev: &Device) -> Result<Self> {
> + Self::request_internal(name, dev, bindings::firmware_request_platform)
> + }
> +
> + /// Send a request for a firmware directly without usermode helper. See also
> + /// `bindings::request_firmware_direct`.
> + pub fn request_direct(name: &CStr, dev: &Device) -> Result<Self> {
> + Self::request_internal(name, dev, bindings::request_firmware_direct)
> + }
Why just these variants? Why not just add the ones that people actually
need instead of a random assortment like you choose here :)
thanks,
greg k-h
On Tue, Jun 11, 2024 at 08:31:46AM +0200, Greg KH wrote:
> On Mon, Jun 10, 2024 at 08:02:28PM +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 <[email protected]>
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/firmware.rs | 107 ++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 3 files changed, 109 insertions(+)
> > create mode 100644 rust/kernel/firmware.rs
> >
> > 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..7ff4c325f670
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,107 @@
> > +// 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::firmware::Firmware;
> > +///
> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> > +/// driver_load_firmware(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)
> > + }
>
> How does this handle when CONFIG_FW_LOADER is not enabled? Why are you
> building these bindings if that option is not checked?
Good catch, gonna fix it.
>
> > +
> > + /// 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)
> > + }
> > +
> > + /// Send a request for a firmware with platform-fw fallback. See also
> > + /// `bindings::firmware_request_platform`.
> > + pub fn request_platform(name: &CStr, dev: &Device) -> Result<Self> {
> > + Self::request_internal(name, dev, bindings::firmware_request_platform)
> > + }
> > +
> > + /// Send a request for a firmware directly without usermode helper. See also
> > + /// `bindings::request_firmware_direct`.
> > + pub fn request_direct(name: &CStr, dev: &Device) -> Result<Self> {
> > + Self::request_internal(name, dev, bindings::request_firmware_direct)
> > + }
>
> Why just these variants? Why not just add the ones that people actually
> need instead of a random assortment like you choose here :)
Indeed seems a bit random, not entirely though. I chose `request_firmware` and
`firmware_request_nowarn` since those are the ones we need in Nova, maybe we can
switch some calls to `request_firmware_into_buf` in the future, once we got the
allocator API in place.
I added `firmware_request_platform` and `request_firmware_direct` as well, since
they share the same function signature as the ones mentioned above and hence all
four of them share the same implementation through `Firmware::request_internal`,
just passing a different function pointer.
If you prefer I can drop the latter for now though.
- Danilo
>
> thanks,
>
> greg k-h
>
On Tue, Jun 11, 2024 at 03:34:22PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 11, 2024 at 08:31:46AM +0200, Greg KH wrote:
> > On Mon, Jun 10, 2024 at 08:02:28PM +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 <[email protected]>
> > > ---
> > > rust/bindings/bindings_helper.h | 1 +
> > > rust/kernel/firmware.rs | 107 ++++++++++++++++++++++++++++++++
> > > rust/kernel/lib.rs | 1 +
> > > 3 files changed, 109 insertions(+)
> > > create mode 100644 rust/kernel/firmware.rs
> > >
> > > 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..7ff4c325f670
> > > --- /dev/null
> > > +++ b/rust/kernel/firmware.rs
> > > @@ -0,0 +1,107 @@
> > > +// 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::firmware::Firmware;
> > > +///
> > > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> > > +/// driver_load_firmware(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)
> > > + }
> >
> > How does this handle when CONFIG_FW_LOADER is not enabled? Why are you
> > building these bindings if that option is not checked?
>
> Good catch, gonna fix it.
>
> >
> > > +
> > > + /// 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)
> > > + }
> > > +
> > > + /// Send a request for a firmware with platform-fw fallback. See also
> > > + /// `bindings::firmware_request_platform`.
> > > + pub fn request_platform(name: &CStr, dev: &Device) -> Result<Self> {
> > > + Self::request_internal(name, dev, bindings::firmware_request_platform)
> > > + }
> > > +
> > > + /// Send a request for a firmware directly without usermode helper. See also
> > > + /// `bindings::request_firmware_direct`.
> > > + pub fn request_direct(name: &CStr, dev: &Device) -> Result<Self> {
> > > + Self::request_internal(name, dev, bindings::request_firmware_direct)
> > > + }
> >
> > Why just these variants? Why not just add the ones that people actually
> > need instead of a random assortment like you choose here :)
>
> Indeed seems a bit random, not entirely though. I chose `request_firmware` and
> `firmware_request_nowarn` since those are the ones we need in Nova, maybe we can
> switch some calls to `request_firmware_into_buf` in the future, once we got the
> allocator API in place.
>
> I added `firmware_request_platform` and `request_firmware_direct` as well, since
> they share the same function signature as the ones mentioned above and hence all
> four of them share the same implementation through `Firmware::request_internal`,
> just passing a different function pointer.
>
> If you prefer I can drop the latter for now though.
Yes, please only add bindings that you "know" will be used. We can
always add new ones later.
thanks,
greg k-h