On Wed, May 22, 2024 at 08:53:34AM +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? >Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of the boilerplate in the 'request' functions to some common 'request_internal' one.
On Mon, 27 May 2024 21:22:47 +0200 Danilo Krummrich <dakr 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? >> > > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of > the boilerplate in the 'request' functions to some common 'request_internal' one.You might need to add 'Invariants' comment on Firmware struct. BTW, what merge window are you aiming for? As I wrote before, I have a driver that needs the firmware abstractions (the minimum device abstractions is enough; Device::as_raw() and as_ref()). So the sooner, the better for me.
On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori wrote:> On Mon, 27 May 2024 21:22:47 +0200 > Danilo Krummrich <dakr 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? > >> > > > > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of > > the boilerplate in the 'request' functions to some common 'request_internal' one. > > You might need to add 'Invariants' comment on Firmware struct.Which ones do you think should be documented?> > BTW, what merge window are you aiming for? As I wrote before, I have a > driver that needs the firmware abstractions (the minimum device > abstractions is enough; Device::as_raw() and as_ref()). So the sooner, > the better for me.I'm not aiming this on a specific merge window. However, if you have a driver that needs the firmware abstractions, I would be surprised if there were any hesitations to already merge the minimum device abstractions [1] and this one (once reviewed) without the rest. At least there aren't any from my side. [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at redhat.com/>