On Wed, 2024-05-22 at 08:53 +0900, FUJITA Tomonori wrote:> Hi, > Thanks for working on the firmware API! > > On Mon, 20 May 2024 19:24:19 +0200 > Danilo Krummrich <dakr at redhat.com> wrote: > > > Add an abstraction around the kernels firmware API to request > > firmware > > images. The abstraction provides functions to access the firmware > > buffer and / or copy it to a new buffer allocated with a given > > allocator > > backend. > > > > The firmware is released once the abstraction is dropped. > > > > Signed-off-by: Danilo Krummrich <dakr at redhat.com> > > --- > > ? rust/bindings/bindings_helper.h |? 1 + > > ? rust/kernel/firmware.rs???????? | 74 > > +++++++++++++++++++++++++++++++++ > > ? rust/kernel/lib.rs????????????? |? 1 + > > ? 3 files changed, 76 insertions(+) > > ? create mode 100644 rust/kernel/firmware.rs > > > > diff --git a/rust/bindings/bindings_helper.h > > b/rust/bindings/bindings_helper.h > > index b245db8d5a87..e4ffc47da5ec 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -14,6 +14,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/pci.h> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > > new file mode 100644 > > index 000000000000..700504fb3c9c > > --- /dev/null > > +++ b/rust/kernel/firmware.rs > > @@ -0,0 +1,74 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Firmware abstraction > > +//! > > +//! C header: > > [`include/linux/firmware.h`](../../../../include/linux/firmware.h") > > + > > +use crate::{bindings, device::Device, error::Error, error::Result, > > str::CStr, types::Opaque}; > > + > > +/// Abstraction around a C firmware struct. > > +/// > > +/// 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]`. Alternatively, the firmware can be copied to a new > > buffer using `Firmware::copy`. The > > +/// firmware is released once [`Firmware`] is dropped. > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let fw = Firmware::request("path/to/firmware.bin", > > dev.as_ref())?; > > +/// driver_load_firmware(fw.data()); > > +/// ``` > > +pub struct Firmware(Opaque<*const bindings::firmware>); > > Wrapping a raw pointer is not the intended use of Qpaque type? >What is the intended use? As I see it, all uses wrapp some binding::* ? but a rawpointer to a binding shouldn't be wrapped by it? Maybe we can add something to the docu in kernel/types.rs: /// Stores an opaque value. /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] pub struct Opaque<T> { P.
Hi, On Wed, 22 May 2024 09:37:30 +0200 Philipp Stanner <pstanner at redhat.com> wrote:>> > +/// Abstraction around a C firmware struct. >> > +/// >> > +/// 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]`. Alternatively, the firmware can be copied to a new >> > buffer using `Firmware::copy`. The >> > +/// firmware is released once [`Firmware`] is dropped. >> > +/// >> > +/// # Examples >> > +/// >> > +/// ``` >> > +/// let fw = Firmware::request("path/to/firmware.bin", >> > dev.as_ref())?; >> > +/// driver_load_firmware(fw.data()); >> > +/// ``` >> > +pub struct Firmware(Opaque<*const bindings::firmware>); >> >> Wrapping a raw pointer is not the intended use of Qpaque type? >> > > What is the intended use? > As I see it, all uses wrapp some binding::* ? but a rawpointer to a > binding shouldn't be wrapped by it?If I understand correctly, it's for embedding C's struct in Rust's struct (as all the usage in the tree do). It gives the tricks for initialization and a pointer to the embedded object. The C's firmware API gives a pointer to an initialized object so no reason to use Opaque. With such C API, Rust's struct simply uses raw pointers in old rust branch. For example, https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28 struct Cdev(*mut bindings::cdev); Another choice that I know is NonNull: https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51 at google.com/ pub struct Page { page: NonNull<bindings::page>, }
On Thu, May 23, 2024 at 08:15:13AM +0900, FUJITA Tomonori wrote:> Hi, > > On Wed, 22 May 2024 09:37:30 +0200 > Philipp Stanner <pstanner at redhat.com> wrote: > > >> > +/// Abstraction around a C firmware struct. > >> > +/// > >> > +/// 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]`. Alternatively, the firmware can be copied to a new > >> > buffer using `Firmware::copy`. The > >> > +/// firmware is released once [`Firmware`] is dropped. > >> > +/// > >> > +/// # Examples > >> > +/// > >> > +/// ``` > >> > +/// let fw = Firmware::request("path/to/firmware.bin", > >> > dev.as_ref())?; > >> > +/// driver_load_firmware(fw.data()); > >> > +/// ``` > >> > +pub struct Firmware(Opaque<*const bindings::firmware>); > >> > >> Wrapping a raw pointer is not the intended use of Qpaque type? > >> > > > > What is the intended use? > > As I see it, all uses wrapp some binding::* ? but a rawpointer to a > > binding shouldn't be wrapped by it? >Thank you Tomo for calling this out! And yes, using `Opaque` on a pointer is weird. A `Opaque<T>` is `UnsafeCell<MayUninit<T>>`, `UnsafeCell` means the inner `T` may be accessed by C code at anytime, and `MayUninit` means that the inner `T` may not be properly initialized by the C code. So as the doc says: This is meant to be used with FFI objects that are never interpreted by Rust code. that is, Rust code should never create a `&T` or `&mut T`, it should only be accessed with `Opaque::get()` or its friends (i.e. accessing it via a raw pointer), much like a black box to Rust code in some sense. Hence putting `Opaque` on a raw pointer is not what you want to do.> If I understand correctly, it's for embedding C's struct in Rust's > struct (as all the usage in the tree do). It gives the tricks for > initialization and a pointer to the embedded object. > > The C's firmware API gives a pointer to an initialized object so no > reason to use Opaque. > > With such C API, Rust's struct simply uses raw pointers in old rust > branch. For example, > > https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28 > > struct Cdev(*mut bindings::cdev); > > > Another choice that I know is NonNull: > > https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51 at google.com/ > > pub struct Page { > page: NonNull<bindings::page>, > }Both are reasonable for temporary use, although, we could generify the "wrapping on pointer which owns the underlying object" in the future. Regards, Boqun