[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zvsypwsxi4hMAfOT@boqun-archlinux>
Date: Mon, 30 Sep 2024 16:22:15 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Guilherme Giacomo Simoes <trintaeoitogc@...il.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
benno.lossin@...ton.me, aliceryhl@...gle.com, mcgrof@...nel.org,
russ.weight@...ux.dev, dakr@...hat.com, a.hindborg@...nel.org,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: device: change the from_raw() function
On Mon, Sep 30, 2024 at 04:46:34PM -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@...il.com>
This looks really good right now, thanks!
Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> ---
^^^ note that this is the '---' that Alice mentioned, next time you
could put information that is helpful but you don't want to include in
the commit log here.
Regards,
Boqun
> 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();
> --
>
> 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@...uxfoundation.org>, Danilo
> <dakr@...nel.org> and Alice <aliceryhl@...gle.com> for help me with this
> patch.
>
> 2.46.2
>
Powered by blists - more mailing lists