Re: [RFC PATCH 7/8] rust: add firmware abstractions

[Boqun Feng <boqun.feng@gmail.com>]

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