For consistency, wrap the firmware as an `Owned` smart pointer in the
constructor.

Cc: Danilo Krummrich <dakr@redhat.com>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index dee5b4b18aec..6da834b37455 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -4,8 +4,8 @@
 //!
 //! 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;
+use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
+            types::{Opaque, Owned, Ownable}};
 
 /// # Invariants
 ///
@@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
 /// # Ok(())
 /// # }
 /// ```
-pub struct Firmware(NonNull<bindings::firmware>);
+ #[repr(transparent)]
+pub struct Firmware(Opaque<bindings::firmware>);
 
 impl Firmware {
-    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
+    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
         let mut fw: *mut bindings::firmware = core::ptr::null_mut();
         let pfw: *mut *mut bindings::firmware = &mut fw;
 
@@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
         if ret != 0 {
             return Err(Error::from_errno(ret));
         }
-
+        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
+        let ptr = fw.cast::<Self>();
         // 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) }))
+        Ok(unsafe { Owned::to_owned(ptr) })
     }
 
     /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
-    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
+    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
         Self::request_internal(name, dev, FwFunc::request())
     }
 
     /// Send a request for an optional firmware module. See also
     /// `bindings::firmware_request_nowarn`.
-    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
+    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
         Self::request_internal(name, dev, FwFunc::request_nowarn())
     }
 
     fn as_raw(&self) -> *mut bindings::firmware {
-        self.0.as_ptr()
+        self.0.get()
     }
 
     /// Returns the size of the requested firmware in bytes.
@@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
     }
 }
 
-impl Drop for Firmware {
-    fn drop(&mut self) {
-        // SAFETY: `self.as_raw()` is valid by the type invariant.
-        unsafe { bindings::release_firmware(self.as_raw()) };
+unsafe impl Ownable for Firmware {
+    unsafe fn ptr_drop(ptr: *mut Self) {
+        // SAFETY:
+        // - By the type invariants, we have ownership of the ptr and can free it.
+        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
+        //   unique pointer to object, it's safe to release the firmware.
+        unsafe { bindings::release_firmware(ptr.cast()) };
     }
 }
 
-- 
2.43.0

On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> For consistency, wrap the firmware as an `Owned` smart pointer in the
> constructor.
> 
> Cc: Danilo Krummrich <dakr@redhat.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index dee5b4b18aec..6da834b37455 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -4,8 +4,8 @@
>  //!
>  //! 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;
> +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> +            types::{Opaque, Owned, Ownable}};
>  
>  /// # Invariants
>  ///
> @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
>  /// # Ok(())
>  /// # }
>  /// ```
> -pub struct Firmware(NonNull<bindings::firmware>);
> + #[repr(transparent)]
> +pub struct Firmware(Opaque<bindings::firmware>);
>  
>  impl Firmware {
> -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {

I think it's fine to implement this for consistency, but I'm not sure I like
that drivers have to refer to it as `Owned<Firmware>`.

Anyway, if we keep it this way the patch also needs the following change.

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index 6da834b37455..1db854eb2422 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {

 // 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 {}
+unsafe impl Send for Owned<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 {}
+unsafe impl Sync for Owned<Firmware> {}

>          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
>          let pfw: *mut *mut bindings::firmware = &mut fw;
>  
> @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
>          if ret != 0 {
>              return Err(Error::from_errno(ret));
>          }
> -
> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> +        let ptr = fw.cast::<Self>();
>          // 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) }))
> +        Ok(unsafe { Owned::to_owned(ptr) })
>      }
>  
>      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
>          Self::request_internal(name, dev, FwFunc::request())
>      }
>  
>      /// Send a request for an optional firmware module. See also
>      /// `bindings::firmware_request_nowarn`.
> -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
>          Self::request_internal(name, dev, FwFunc::request_nowarn())
>      }
>  
>      fn as_raw(&self) -> *mut bindings::firmware {
> -        self.0.as_ptr()
> +        self.0.get()
>      }
>  
>      /// Returns the size of the requested firmware in bytes.
> @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
>      }
>  }
>  
> -impl Drop for Firmware {
> -    fn drop(&mut self) {
> -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> -        unsafe { bindings::release_firmware(self.as_raw()) };
> +unsafe impl Ownable for Firmware {
> +    unsafe fn ptr_drop(ptr: *mut Self) {
> +        // SAFETY:
> +        // - By the type invariants, we have ownership of the ptr and can free it.
> +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> +        //   unique pointer to object, it's safe to release the firmware.
> +        unsafe { bindings::release_firmware(ptr.cast()) };
>      }
>  }
>  
> -- 
> 2.43.0
>

On Wed, Oct 23, 2024 at 11:35:15AM +0200, Danilo Krummrich wrote:
> On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > constructor.
> > 
> > Cc: Danilo Krummrich <dakr@redhat.com>
> > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..6da834b37455 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -4,8 +4,8 @@
> >  //!
> >  //! 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;
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > +            types::{Opaque, Owned, Ownable}};
> >  
> >  /// # Invariants
> >  ///
> > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> >  /// # Ok(())
> >  /// # }
> >  /// ```
> > -pub struct Firmware(NonNull<bindings::firmware>);
> > + #[repr(transparent)]
> > +pub struct Firmware(Opaque<bindings::firmware>);
> >  
> >  impl Firmware {
> > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> 
> I think it's fine to implement this for consistency, but I'm not sure I like
> that drivers have to refer to it as `Owned<Firmware>`.
> 

May I ask why not? ;-)

Ideally, we should not wrap a pointer to particular type, instead we
should wrap the type and then combine it with a meaningful pointer type,
e.g. Box<>, ARef<>, Owned<> ... in this way, we de-couple how the
lifetime of object is maintained (described by the pointer type) and
what operations are available on the object (described by the wrapper
type).

If later on, a firmware object creation is doable in pure Rust code for
some condition, we can then have a function that returns a
`KBox<Firmware>` (assume using kmalloc for the object), and it will just
work (tm).

Regards,
Boqun

> Anyway, if we keep it this way the patch also needs the following change.
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6da834b37455..1db854eb2422 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> 
>  // 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 {}
> +unsafe impl Send for Owned<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 {}
> +unsafe impl Sync for Owned<Firmware> {}
> 
> >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> >          let pfw: *mut *mut bindings::firmware = &mut fw;
> >  
> > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> >          if ret != 0 {
> >              return Err(Error::from_errno(ret));
> >          }
> > -
> > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > +        let ptr = fw.cast::<Self>();
> >          // 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) }))
> > +        Ok(unsafe { Owned::to_owned(ptr) })
> >      }
> >  
> >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request())
> >      }
> >  
> >      /// Send a request for an optional firmware module. See also
> >      /// `bindings::firmware_request_nowarn`.
> > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> >      }
> >  
> >      fn as_raw(&self) -> *mut bindings::firmware {
> > -        self.0.as_ptr()
> > +        self.0.get()
> >      }
> >  
> >      /// Returns the size of the requested firmware in bytes.
> > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> >      }
> >  }
> >  
> > -impl Drop for Firmware {
> > -    fn drop(&mut self) {
> > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > +unsafe impl Ownable for Firmware {
> > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > +        // SAFETY:
> > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > +        //   unique pointer to object, it's safe to release the firmware.
> > +        unsafe { bindings::release_firmware(ptr.cast()) };
> >      }
> >  }
> >  
> > -- 
> > 2.43.0
> > 
>

On Sun, Oct 27, 2024 at 03:20:03PM -0700, Boqun Feng wrote:
> On Wed, Oct 23, 2024 at 11:35:15AM +0200, Danilo Krummrich wrote:
> > On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > > constructor.
> > > 
> > > Cc: Danilo Krummrich <dakr@redhat.com>
> > > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > > ---
> > >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > > index dee5b4b18aec..6da834b37455 100644
> > > --- a/rust/kernel/firmware.rs
> > > +++ b/rust/kernel/firmware.rs
> > > @@ -4,8 +4,8 @@
> > >  //!
> > >  //! 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;
> > > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > > +            types::{Opaque, Owned, Ownable}};
> > >  
> > >  /// # Invariants
> > >  ///
> > > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> > >  /// # Ok(())
> > >  /// # }
> > >  /// ```
> > > -pub struct Firmware(NonNull<bindings::firmware>);
> > > + #[repr(transparent)]
> > > +pub struct Firmware(Opaque<bindings::firmware>);
> > >  
> > >  impl Firmware {
> > > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> > 
> > I think it's fine to implement this for consistency, but I'm not sure I like
> > that drivers have to refer to it as `Owned<Firmware>`.
> > 
> 
> May I ask why not? ;-)

I think it's because with this an instance of `Firmware` is never a valid thing
to have, which is a bit weird at a first glance.

But I fully agree with the existance of the `Owned` type and the rationale
below.

> 
> Ideally, we should not wrap a pointer to particular type, instead we
> should wrap the type and then combine it with a meaningful pointer type,
> e.g. Box<>, ARef<>, Owned<> ... in this way, we de-couple how the
> lifetime of object is maintained (described by the pointer type) and
> what operations are available on the object (described by the wrapper
> type).
> 
> If later on, a firmware object creation is doable in pure Rust code for
> some condition, we can then have a function that returns a
> `KBox<Firmware>` (assume using kmalloc for the object), and it will just
> work (tm).
> 
> Regards,
> Boqun
> 
> > Anyway, if we keep it this way the patch also needs the following change.
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index 6da834b37455..1db854eb2422 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> > 
> >  // 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 {}
> > +unsafe impl Send for Owned<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 {}
> > +unsafe impl Sync for Owned<Firmware> {}
> > 
> > >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> > >          let pfw: *mut *mut bindings::firmware = &mut fw;
> > >  
> > > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > >          if ret != 0 {
> > >              return Err(Error::from_errno(ret));
> > >          }
> > > -
> > > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > > +        let ptr = fw.cast::<Self>();
> > >          // 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) }))
> > > +        Ok(unsafe { Owned::to_owned(ptr) })
> > >      }
> > >  
> > >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > >          Self::request_internal(name, dev, FwFunc::request())
> > >      }
> > >  
> > >      /// Send a request for an optional firmware module. See also
> > >      /// `bindings::firmware_request_nowarn`.
> > > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> > >      }
> > >  
> > >      fn as_raw(&self) -> *mut bindings::firmware {
> > > -        self.0.as_ptr()
> > > +        self.0.get()
> > >      }
> > >  
> > >      /// Returns the size of the requested firmware in bytes.
> > > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> > >      }
> > >  }
> > >  
> > > -impl Drop for Firmware {
> > > -    fn drop(&mut self) {
> > > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > > +unsafe impl Ownable for Firmware {
> > > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > > +        // SAFETY:
> > > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > > +        //   unique pointer to object, it's safe to release the firmware.
> > > +        unsafe { bindings::release_firmware(ptr.cast()) };
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.43.0
> > > 
> > 
>

On Wed, Oct 23, 2024 at 11:35:20AM +0200, Danilo Krummrich wrote:
> On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > constructor.
> > 
> > Cc: Danilo Krummrich <dakr@redhat.com>
> > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..6da834b37455 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -4,8 +4,8 @@
> >  //!
> >  //! 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;
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > +            types::{Opaque, Owned, Ownable}};
> >  
> >  /// # Invariants
> >  ///
> > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> >  /// # Ok(())
> >  /// # }
> >  /// ```
> > -pub struct Firmware(NonNull<bindings::firmware>);
> > + #[repr(transparent)]
> > +pub struct Firmware(Opaque<bindings::firmware>);
> >  
> >  impl Firmware {
> > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> 
> I think it's fine to implement this for consistency, but I'm not sure I like
> that drivers have to refer to it as `Owned<Firmware>`.
> 
> Anyway, if we keep it this way the patch also needs the following change.
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6da834b37455..1db854eb2422 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> 
>  // 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 {}
> +unsafe impl Send for Owned<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 {}
> +unsafe impl Sync for Owned<Firmware> {}

Actually, I think `Owned` should implement `Send` and `Sync` like this instead.

   unsafe impl<T> Sync for Owned<T> where T: Sync + Ownable {}
   unsafe impl<T> Send for Owned<T> where T: Send + Ownable {}

> 
> >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> >          let pfw: *mut *mut bindings::firmware = &mut fw;
> >  
> > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> >          if ret != 0 {
> >              return Err(Error::from_errno(ret));
> >          }
> > -
> > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > +        let ptr = fw.cast::<Self>();
> >          // 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) }))
> > +        Ok(unsafe { Owned::to_owned(ptr) })
> >      }
> >  
> >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request())
> >      }
> >  
> >      /// Send a request for an optional firmware module. See also
> >      /// `bindings::firmware_request_nowarn`.
> > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> >      }
> >  
> >      fn as_raw(&self) -> *mut bindings::firmware {
> > -        self.0.as_ptr()
> > +        self.0.get()
> >      }
> >  
> >      /// Returns the size of the requested firmware in bytes.
> > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> >      }
> >  }
> >  
> > -impl Drop for Firmware {
> > -    fn drop(&mut self) {
> > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > +unsafe impl Ownable for Firmware {
> > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > +        // SAFETY:
> > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > +        //   unique pointer to object, it's safe to release the firmware.
> > +        unsafe { bindings::release_firmware(ptr.cast()) };
> >      }
> >  }
> >  
> > -- 
> > 2.43.0
> >