[PATCH v3 0/1] rust: device: rename "Device::from_raw()"

[PATCH v3 0/1] rust: device: rename "Device::from_raw()"

[Guilherme Giacomo Simoes]

Why did I make this change?
This function "Device::from_raw()" increments the refcount by this
command "bindings::get_device(prt)". This can be confused because the
function Arc::from_raw() from the standard library, doesn't increment
the refcount.

This discussion is in
https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Inconsistency.20of.20.60from_raw.60.2E

The options can be:
1) Rename the function for don't make confusing with the
Arc::from_raw().
2) Remove this function and use the unsafe { Device::as_ref(ptr)
}.into() when I need the get pointer for the device.

Proposed solution
I like the first option. Because, how was will commented by Boqun Feng ,
when the people write the "unsafe { Device::as_ref(ptr) }.into()" again,
again and again... inevitably anybody will create a help function for
this.

Then I think that we should rename this function for
Device::get_from_raw() or maybe Device::get_device() and I like more of
the second option because, this will be equal the get_device() function
that already exists in .c code.


How do I test this:
I create this simple file in sample/rust/device.rs
""""""""
use kernel::device::Device;
use kernel::prelude::*;
use kernel::types::ARef;

module! {
    type: DeviceTest,
    name: "device_test",
    author: "Test device",
    description: "A simple module for test device",
    license: "GPL",
}

struct DeviceTest;

impl kernel::Module for DeviceTest {
    fn init(_module: &'static ThisModule) -> Result<Self> {
        pr_info!("initial device test");
        let device = create_and_get_device();
        pr_info!("device created");

        Ok(DeviceTest)
    }
}

impl Drop for DeviceTest {
    fn drop(&mut self) {
        pr_info!("bye bye driver test");
    }
}

fn create_and_get_device() -> ARef<Device> {
    let device = unsafe { Device::get_device(core::ptr::null_mut()) };
    device
}
""""""""

I set this in Kconfig
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..7779969e7dd6 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -37,4 +37,9 @@ config SAMPLE_RUST_HOSTPROGS

          If unsure, say N.

+config SAMPLE_DEVICE_TEST
+       tristate "Device test"
+       help
+               This option is for device test
+
 endif # SAMPLES_RUST


and in Makefile
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea4..85a8b30100e7 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,5 +2,6 @@

 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)              += rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)                        += rust_print.o
+obj-$(CONFIG_SAMPLE_DEVICE_TEST)                       += device.o

 subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)         += hostprogs


Then I enable this in menu config... compile the kernel e run this in a
qemu:
qemu-system-x86_64 -kernel bzImage -initrd initramfs.img -m 2G -machine
q35 -device ich9-ahci,id=sata -drive
id=disk,file=rootfs.img,if=none,format=raw -device
ide-hd,drive=disk,bus=sata.0 -append "root=/dev/sda console=ttyS0"
-nographic -monitor telnet:127.0.0.1:5555,server,nowai

the expected print is showing
[    2.786174] device_test: initial device test
[    2.786541] device_test: device created


Guilherme Giacomo Simoes (1):
  rust: device: rename "Device::from_raw()"

 rust/kernel/device.rs   | 2 +-
 rust/kernel/firmware.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 

Changes from v2:
 - Refactored commit message style

[PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Guilherme Giacomo Simoes]

This function increments the refcount by a call to
"bindings::get_device(ptr)". This can be confused because, the function
Arch::from_raw() from standard library, don't increments the refcount.
Hence, rename "Device::from_raw()" to avoid confusion with other
"from_raw" semantics.

Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 rust/kernel/device.rs   | 2 +-
 rust/kernel/firmware.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 851018eef885..ecffaff041e0 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..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 1/1] rust: device: rename "Device::from_raw()"

[Greg KH]

On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> This function increments the refcount by a call to
> "bindings::get_device(ptr)". This can be confused because, the function
> Arch::from_raw() from standard library, don't increments the refcount.
> Hence, rename "Device::from_raw()" to avoid confusion with other
> "from_raw" semantics.
> 
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> ---
>  rust/kernel/device.rs   | 2 +-
>  rust/kernel/firmware.rs | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Guilherme Giácomo Simões]

Greg KH <gregkh@linuxfoundation.org> writes:
>
> On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> > This function increments the refcount by a call to
> > "bindings::get_device(ptr)". This can be confused because, the function
> > Arch::from_raw() from standard library, don't increments the refcount.
> > Hence, rename "Device::from_raw()" to avoid confusion with other
> > "from_raw" semantics.
> >
> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> > ---
> >  rust/kernel/device.rs   | 2 +-
> >  rust/kernel/firmware.rs | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
>
> Hi,
>
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
>
> You are receiving this message because of the following common error(s)
> as indicated below:
>
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/process/submitting-patches.rst for what
>   needs to be done here to properly describe this.
>
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
>
> thanks,
>
> greg k-h's patch email bot

Yeah, I was think that only in 0/1 I should explain the changes ..
I'm was wrong.   I'll put the changelog in 1/1 too.

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Greg KH]

On Mon, Sep 30, 2024 at 11:57:25AM -0300, Guilherme Giácomo Simões wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> >
> > On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> > > This function increments the refcount by a call to
> > > "bindings::get_device(ptr)". This can be confused because, the function
> > > Arch::from_raw() from standard library, don't increments the refcount.
> > > Hence, rename "Device::from_raw()" to avoid confusion with other
> > > "from_raw" semantics.
> > >
> > > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> > > ---
> > >  rust/kernel/device.rs   | 2 +-
> > >  rust/kernel/firmware.rs | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> >
> > Hi,
> >
> > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > a patch that has triggered this response.  He used to manually respond
> > to these common problems, but in order to save his sanity (he kept
> > writing the same thing over and over, yet to different people), I was
> > created.  Hopefully you will not take offence and will fix the problem
> > in your patch and resubmit it so that it can be accepted into the Linux
> > kernel tree.
> >
> > You are receiving this message because of the following common error(s)
> > as indicated below:
> >
> > - This looks like a new version of a previously submitted patch, but you
> >   did not list below the --- line any changes from the previous version.
> >   Please read the section entitled "The canonical patch format" in the
> >   kernel file, Documentation/process/submitting-patches.rst for what
> >   needs to be done here to properly describe this.
> >
> > If you wish to discuss this problem further, or you have questions about
> > how to resolve this issue, please feel free to respond to this email and
> > Greg will reply once he has dug out from the pending patches received
> > from other developers.
> >
> > thanks,
> >
> > greg k-h's patch email bot
> 
> Yeah, I was think that only in 0/1 I should explain the changes ..
> I'm was wrong.   I'll put the changelog in 1/1 too.

Was it in the 0/1 email?  I didn't see it there either.

And for patches where there is only one commit, you almost never need a
0/1 email, just put the needed information in the single patch you send
out.

thanks,

greg k-h

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Danilo Krummrich]

On Mon, Sep 30, 2024 at 11:57:25AM -0300, Guilherme Giácomo Simões wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> >
> > On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> > > This function increments the refcount by a call to
> > > "bindings::get_device(ptr)". This can be confused because, the function
> > > Arch::from_raw() from standard library, don't increments the refcount.
> > > Hence, rename "Device::from_raw()" to avoid confusion with other
> > > "from_raw" semantics.
> > >
> > > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> > > ---
> > >  rust/kernel/device.rs   | 2 +-
> > >  rust/kernel/firmware.rs | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> >
> > Hi,
> >
> > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > a patch that has triggered this response.  He used to manually respond
> > to these common problems, but in order to save his sanity (he kept
> > writing the same thing over and over, yet to different people), I was
> > created.  Hopefully you will not take offence and will fix the problem
> > in your patch and resubmit it so that it can be accepted into the Linux
> > kernel tree.
> >
> > You are receiving this message because of the following common error(s)
> > as indicated below:
> >
> > - This looks like a new version of a previously submitted patch, but you
> >   did not list below the --- line any changes from the previous version.
> >   Please read the section entitled "The canonical patch format" in the
> >   kernel file, Documentation/process/submitting-patches.rst for what
> >   needs to be done here to properly describe this.
> >
> > If you wish to discuss this problem further, or you have questions about
> > how to resolve this issue, please feel free to respond to this email and
> > Greg will reply once he has dug out from the pending patches received
> > from other developers.
> >
> > thanks,
> >
> > greg k-h's patch email bot
> 
> Yeah, I was think that only in 0/1 I should explain the changes ..
> I'm was wrong.   I'll put the changelog in 1/1 too.
> 

It's fine to have the changelog in the cover letter. I don't know under which
exact conditions Greg's bot triggers though. Normally,

For a single patch of this size and complexity a cover letter isn't needed
though.

But don't worry, no need to resend. :)

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Danilo Krummrich]

On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> This function increments the refcount by a call to
> "bindings::get_device(ptr)". This can be confused because, the function
> Arch::from_raw() from standard library, don't increments the refcount.

`Arc::from_raw`

Again the note, it's both the semantics used in the Rust stdlib and in the
kernel.

Let's wait for what Greg says, if he wants to fix the small typo when
applying the patch.

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

> Hence, rename "Device::from_raw()" to avoid confusion with other
> "from_raw" semantics.
> 
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> ---
>  rust/kernel/device.rs   | 2 +-
>  rust/kernel/firmware.rs | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 851018eef885..ecffaff041e0 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..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 1/1] rust: device: rename "Device::from_raw()"

[Alice Ryhl]

On Mon, Sep 30, 2024 at 4:58 PM Guilherme Giácomo Simões
<trintaeoitogc@gmail.com> wrote:
>
> Greg KH <gregkh@linuxfoundation.org> writes:
> >
> > On Mon, Sep 30, 2024 at 11:43:27AM -0300, Guilherme Giacomo Simoes wrote:
> > > This function increments the refcount by a call to
> > > "bindings::get_device(ptr)". This can be confused because, the function
> > > Arch::from_raw() from standard library, don't increments the refcount.
> > > Hence, rename "Device::from_raw()" to avoid confusion with other
> > > "from_raw" semantics.
> > >
> > > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> > > ---
> > >  rust/kernel/device.rs   | 2 +-
> > >  rust/kernel/firmware.rs | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> >
> > Hi,
> >
> > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > a patch that has triggered this response.  He used to manually respond
> > to these common problems, but in order to save his sanity (he kept
> > writing the same thing over and over, yet to different people), I was
> > created.  Hopefully you will not take offence and will fix the problem
> > in your patch and resubmit it so that it can be accepted into the Linux
> > kernel tree.
> >
> > You are receiving this message because of the following common error(s)
> > as indicated below:
> >
> > - This looks like a new version of a previously submitted patch, but you
> >   did not list below the --- line any changes from the previous version.
> >   Please read the section entitled "The canonical patch format" in the
> >   kernel file, Documentation/process/submitting-patches.rst for what
> >   needs to be done here to properly describe this.
> >
> > If you wish to discuss this problem further, or you have questions about
> > how to resolve this issue, please feel free to respond to this email and
> > Greg will reply once he has dug out from the pending patches received
> > from other developers.
> >
> > thanks,
> >
> > greg k-h's patch email bot
>
> Yeah, I was think that only in 0/1 I should explain the changes ..
> I'm was wrong.   I'll put the changelog in 1/1 too.

You can use one of my patches as an example. E.g.:
https://lore.kernel.org/all/20240930-static-mutex-v4-1-c59555413127@google.com/

Here, the commit message itself has:
1. Motivation for why we should add global lock support. (To replace a
hack I had to use in the Binder driver.)
2. Explanation for why I implemented it in a certain way. (Why
separate initialization step?)

Then, below the --- line and not part of the commit message, I have:
1. Information about which patches it depends on.
2. A changelog and links to previous versions.

Anything below the --- line will not be part of the commit history
when your change is merged. So you should think about what people
would want to see when they look at your patch in the commit history.
They care about why the change was made, and why it was implemented
that way. What other things need to be merged first are not relevant
to people who see the final version after it has been merged.

Similarly, the changelog is important for reviewers so they can
compare with the previous version, but for people who just see the
final version, they don't care about which bugs you had in previous
versions of the patch. Of course, if you change the implementation
approach, then they might care about why you chose that approach over
some other approach, but that explanation should be in the commit
message (and the changelog should just say you changed the approach).

Alice

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Guilherme Giácomo Simões]

Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Was it in the 0/1 email?  I didn't see it there either.
>
> And for patches where there is only one commit, you almost never need a
> 0/1 email, just put the needed information in the single patch you send
> out.
>
> thanks,
>
> greg k-h

The 0/1 email is for explanation the motivations, attach a link for
discussion or issue and too for explain how to test this.

But if you think that is not necessary, I can send a v4 without 0/1

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Guilherme Giácomo Simões]

Alice Ryhl <aliceryhl@google.com> wrote:
>
> You can use one of my patches as an example. E.g.:
> https://lore.kernel.org/all/20240930-static-mutex-v4-1-c59555413127@google.com/
>
> Here, the commit message itself has:
> 1. Motivation for why we should add global lock support. (To replace a
> hack I had to use in the Binder driver.)
> 2. Explanation for why I implemented it in a certain way. (Why
> separate initialization step?)
>
> Then, below the --- line and not part of the commit message, I have:
> 1. Information about which patches it depends on.
> 2. A changelog and links to previous versions.
>
> Anything below the --- line will not be part of the commit history
> when your change is merged. So you should think about what people
> would want to see when they look at your patch in the commit history.
> They care about why the change was made, and why it was implemented
> that way. What other things need to be merged first are not relevant
> to people who see the final version after it has been merged.
>
> Similarly, the changelog is important for reviewers so they can
> compare with the previous version, but for people who just see the
> final version, they don't care about which bugs you had in previous
> versions of the patch. Of course, if you change the implementation
> approach, then they might care about why you chose that approach over
> some other approach, but that explanation should be in the commit
> message (and the changelog should just say you changed the approach).
>
> Alice

This really make sense. I will resend a v4 version of this patch,
without 0/1.

Tanks for this Mrs. Ryhl.

Re: [PATCH v3 1/1] rust: device: rename "Device::from_raw()"

[Miguel Ojeda]

On Mon, Sep 30, 2024 at 6:40 PM Guilherme Giácomo Simões
<trintaeoitogc@gmail.com> wrote:
>
> The 0/1 email is for explanation the motivations, attach a link for
> discussion or issue and too for explain how to test this.

Yeah, that is fine. What Greg is saying is that, when it is a single
patch, one can put that in the patch itself, below the `---` line. You
can add explanations there for each patch, and when a patch series
only has 1 patch, then you can use that space as the cover letter,
thus avoiding a 0/1 email.

Cheers,
Miguel