[v3] rust: device: change the from_raw() function

[PATCH v3] rust: device: change the from_raw() function

Posted by Guilherme Giacomo Simoes 1 month, 3 weeks ago

The function Device::from_raw() increments a refcount by a call to
bindings::get_device(ptr). This can be confused because usually
from_raw() functions don't increment a refcount.
Hence, rename Device::from_raw() to avoid confuion with other "from_raw"
semantics.

The new name of function should be "get_device" to be consistent with
the function get_device() already exist in .c files.

This function body also changed, because the `into()` will convert the
`&'a Device` into `ARef<Device>` and also call `inc_ref` from the
`AlwaysRefCounted` trait implemented for Device.

Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
differences from v1 to v2:
 - remove the 0/1 patch
 - refactor get_device() function

differences from v2:
- fix the place of changelog.

The motivation from this change was will discussion in:
https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Inconsistency.20of.20.60from_raw.60.2E

I would like to thanks for Greg <gregkh@linuxfoundation.org>, Danilo
<dakr@kernel.org> and Alice <aliceryhl@google.com> for help me with this
patch.
---
 rust/kernel/device.rs   | 15 +++------------
 rust/kernel/firmware.rs |  2 +-
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 851018eef885..c8199ee079ef 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -51,18 +51,9 @@ impl Device {
     ///
     /// It must also be ensured that `bindings::device::release` can be called from any thread.
     /// While not officially documented, this should be the case for any `struct device`.
-    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
-        // SAFETY: By the safety requirements, ptr is valid.
-        // Initially increase the reference count by one to compensate for the final decrement once
-        // this newly created `ARef<Device>` instance is dropped.
-        unsafe { bindings::get_device(ptr) };
-
-        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
-        let ptr = ptr.cast::<Self>();
-
-        // SAFETY: `ptr` is valid by the safety requirements of this function. By the above call to
-        // `bindings::get_device` we also own a reference to the underlying `struct device`.
-        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) }
+    pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
+        // SAFETY: By the safety requirements ptr is valid
+        unsafe { Self::as_ref(ptr) }.into()
     }
 
     /// Obtain the raw `struct device *`.
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index dee5b4b18aec..13a374a5cdb7 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -44,7 +44,7 @@ fn request_nowarn() -> Self {
 ///
 /// # fn no_run() -> Result<(), Error> {
 /// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
-/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
+/// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) };
 ///
 /// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?;
 /// let blob = fw.data();
-- 
2.46.2

Re: [PATCH v3] rust: device: change the from_raw() function

Posted by Danilo Krummrich 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 05:56:03PM -0300, Guilherme Giacomo Simoes wrote:
> The function Device::from_raw() increments a refcount by a call to
> bindings::get_device(ptr). This can be confused because usually
> from_raw() functions don't increment a refcount.
> Hence, rename Device::from_raw() to avoid confuion with other "from_raw"
> semantics.
> 
> The new name of function should be "get_device" to be consistent with
> the function get_device() already exist in .c files.
> 
> This function body also changed, because the `into()` will convert the
> `&'a Device` into `ARef<Device>` and also call `inc_ref` from the
> `AlwaysRefCounted` trait implemented for Device.
> 
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

Acked-by: Danilo Krummrich <dakr@kernel.org>

> ---
> differences from v1 to v2:
>  - remove the 0/1 patch
>  - refactor get_device() function
> 
> differences from v2:
> - fix the place of changelog.
> 
> The motivation from this change was will discussion in:
> https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Inconsistency.20of.20.60from_raw.60.2E
> 
> I would like to thanks for Greg <gregkh@linuxfoundation.org>, Danilo
> <dakr@kernel.org> and Alice <aliceryhl@google.com> for help me with this
> patch.
> ---
>  rust/kernel/device.rs   | 15 +++------------
>  rust/kernel/firmware.rs |  2 +-
>  2 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 851018eef885..c8199ee079ef 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -51,18 +51,9 @@ impl Device {
>      ///
>      /// It must also be ensured that `bindings::device::release` can be called from any thread.
>      /// While not officially documented, this should be the case for any `struct device`.
> -    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
> -        // SAFETY: By the safety requirements, ptr is valid.
> -        // Initially increase the reference count by one to compensate for the final decrement once
> -        // this newly created `ARef<Device>` instance is dropped.
> -        unsafe { bindings::get_device(ptr) };
> -
> -        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
> -        let ptr = ptr.cast::<Self>();
> -
> -        // SAFETY: `ptr` is valid by the safety requirements of this function. By the above call to
> -        // `bindings::get_device` we also own a reference to the underlying `struct device`.
> -        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) }
> +    pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
> +        // SAFETY: By the safety requirements ptr is valid
> +        unsafe { Self::as_ref(ptr) }.into()
>      }
>  
>      /// Obtain the raw `struct device *`.
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index dee5b4b18aec..13a374a5cdb7 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -44,7 +44,7 @@ fn request_nowarn() -> Self {
>  ///
>  /// # fn no_run() -> Result<(), Error> {
>  /// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
> -/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
> +/// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) };
>  ///
>  /// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?;
>  /// let blob = fw.data();
> -- 
> 2.46.2
>

Re: [PATCH v3] rust: device: change the from_raw() function

Posted by Greg KH 1 month, 3 weeks ago

On Wed, Oct 02, 2024 at 03:58:29PM +0200, Danilo Krummrich wrote:
> On Tue, Oct 01, 2024 at 05:56:03PM -0300, Guilherme Giacomo Simoes wrote:
> > The function Device::from_raw() increments a refcount by a call to
> > bindings::get_device(ptr). This can be confused because usually
> > from_raw() functions don't increment a refcount.
> > Hence, rename Device::from_raw() to avoid confuion with other "from_raw"
> > semantics.
> > 
> > The new name of function should be "get_device" to be consistent with
> > the function get_device() already exist in .c files.
> > 
> > This function body also changed, because the `into()` will convert the
> > `&'a Device` into `ARef<Device>` and also call `inc_ref` from the
> > `AlwaysRefCounted` trait implemented for Device.
> > 
> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Thanks for the review, and Guilherme, for all of the revisions.  I'll
queue this up for this release soon so we don't get any code building on
the old api.

thanks,

greg k-h

Re: [PATCH v3] rust: device: change the from_raw() function

Posted by Boqun Feng 1 month, 3 weeks ago


On Thu, Oct 3, 2024, at 12:07 AM, Greg KH wrote:
> On Wed, Oct 02, 2024 at 03:58:29PM +0200, Danilo Krummrich wrote:
>> On Tue, Oct 01, 2024 at 05:56:03PM -0300, Guilherme Giacomo Simoes wrote:
>> > The function Device::from_raw() increments a refcount by a call to
>> > bindings::get_device(ptr). This can be confused because usually
>> > from_raw() functions don't increment a refcount.
>> > Hence, rename Device::from_raw() to avoid confuion with other "from_raw"
>> > semantics.
>> > 
>> > The new name of function should be "get_device" to be consistent with
>> > the function get_device() already exist in .c files.
>> > 
>> > This function body also changed, because the `into()` will convert the
>> > `&'a Device` into `ARef<Device>` and also call `inc_ref` from the
>> > `AlwaysRefCounted` trait implemented for Device.
>> > 

This work is because we have an issue, so this should really have a:

Closes: https://github.com/Rust-for-Linux/linux/issues/1088

tag.

Greg, could you add this when you queue this?

Guilherme, please add such a tag next time, so we can have things
tracked. Thanks!

Regards,
Boqun

>> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
>> 
>> Acked-by: Danilo Krummrich <dakr@kernel.org>
>
> Thanks for the review, and Guilherme, for all of the revisions.  I'll
> queue this up for this release soon so we don't get any code building on
> the old api.
>
> thanks,
>
> greg k-h

Re: [PATCH v3] rust: device: change the from_raw() function

Posted by Guilherme Giácomo Simões 1 month, 3 weeks ago

Boqun Feng <boqun.feng@gmail.com> wrote:
> This work is because we have an issue, so this should really have a:
>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1088
>
> tag.
>
> Greg, could you add this when you queue this?
>
> Guilherme, please add such a tag next time, so we can have things
> tracked. Thanks!
>
> Regards,
> Boqun
>
Understood. Next time I should add the closes tag.