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/>
On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:> 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/No, the device abstractions are NOT ready for merging just yet, sorry, if for no other reason than a non-RFC has never been posted :) greg k-h
On Tue, 28 May 2024 14:19:24 +0200 Danilo Krummrich <dakr at redhat.com> wrote:> 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?Something like the comment for struct Page looks fine to me. But the Rust reviewers might have a different opinion. /// The pointer is valid, and has ownership over the page.