-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Device::from_raw()
in favor of using Device::as_ref()
+ into()
.
#1088
Comments
Perhaps @panosfol would like to take on this introductory patch for the kernel. |
Yes sounds good! |
I understand this "problmen" ... The Arc::from_raw() function from standart library don't increment the refcount. But the Device::from_raw() increement your own refcount byt the line The options can be:
I like more of the first option. Because, how was will commented by @fbq , when the people wrtite the Then I think that we should rename this function for Then my patch will be anything like this: diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 851018eef885..5dfbe7c0824c 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -51,7 +51,7 @@ 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> {
+ pub unsafe fn get_device(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.
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index dee5b4b18aec..23a0a758fc72 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(); What do you think ?? |
@fbq the patch will be applied. |
Thanks for the hard work. As I already mentioned in the email reply, the patch should have a "Closes" tag, with that we can track that when the patch gets merged. |
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> Closes: Rust-for-Linux#1088 Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Link: https://lore.kernel.org/r/20241001205603.106278-1-trintaeoitogc@gmail.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
First, there is an inconsistency between
Device::from_raw()
andARef::from_raw()
on whether the refcount gets increased or not. That meansDevice::from_raw()
should be renamed. Moreover, as suggested here,Device::from_raw()
can be avoid by usingunsafe { Device::as_ref(ptr) }.into()
. Things need to be done:Device::from_raw()
.Device::as_ref()
and probablyARef
as wellThis requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes, to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and so on. Please see https://docs.kernel.org/process/submitting-patches.html and https://rust-for-linux.com/contributing for details.
Please take this issue only if you are new to the kernel development process and you would like to use it as a test to submit your first patch to the kernel. Please do not take it if you do not plan to make other contributions to the kernel.
The text was updated successfully, but these errors were encountered: