[PATCH 1/2] device: rust: change the name function

[PATCH 1/2] device: rust: change the name function

[Guilherme Giácomo Simões]

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

Re: [PATCH 1/2] device: rust: change the name function

[Daniel Almeida]

Hi Guilherme, welcome to the community!

The commit message needs some work IMHO. In particular, links may become stale,
so please do not link to anything.

It’s also written in first person and in a verbose way, which is not good.
Just describe what you did and why the change was necessary succinctly.

You can find some more tips at
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Lastly, I can’t see a diff anywhere. Make sure everything you send can
actually be applied and *at least* compile-check your changes. Submitting
patches that do not apply and/or do not compile can make people lose interest.

Send a v2 with the fixes above and we can start to actually discuss your code.

BR

— Daniel

Re: [PATCH 1/2] device: rust: change the name function

[Dirk Behme]

On 29.09.2024 17:35, Daniel Almeida wrote:
> links may become stale,
> so please do not link to anything.

This sounds a little too general to me ;)

I mean we link a lot, using the 'Link' tag even in patches.

For example the mailing list archive has the 'permalink' button and 
Zulip has the 'Copy link to message' button I think can be used.


> Lastly, I can’t see a diff anywhere.

Yes, I was confused by that, too. But I wonder if just the numbering is 
wrong? I.e. if [PATCH 1/2] was intended to be the cover letter 0/1 and 
the [PATCH 2/2] was intended to be the actual patch 1/1? Just a hint :)

Best regards

Dirk

Re: [PATCH 1/2] device: rust: change the name function

[Guilherme Giácomo Simões]

Dirk Behme <dirk.behme@de.bosch.com> writes:
>
>
> On 29.09.2024 17:35, Daniel Almeida wrote:
> > links may become stale,
> > so please do not link to anything.
>
> This sounds a little too general to me ;)
>
> I mean we link a lot, using the 'Link' tag even in patches.
>
> For example the mailing list archive has the 'permalink' button and
> Zulip has the 'Copy link to message' button I think can be used.
>
>
> > Lastly, I can’t see a diff anywhere.
>
> Yes, I was confused by that, too. But I wonder if just the numbering is
> wrong? I.e. if [PATCH 1/2] was intended to be the cover letter 0/1 and
> the [PATCH 2/2] was intended to be the actual patch 1/1? Just a hint :)
>
> Best regards
>
> Dirk

I can resend the patch with the message commit fixed and the correct numeration.
Thanks for your comment in my patch.

Re: [PATCH 1/2] device: rust: change the name function

[Miguel Ojeda]

On Mon, Sep 30, 2024 at 6:33 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> This sounds a little too general to me ;)
>
> I mean we link a lot, using the 'Link' tag even in patches.
>
> For example the mailing list archive has the 'permalink' button and
> Zulip has the 'Copy link to message' button I think can be used.

Yeah, it is fine using Links, as long as the commit message is
understandable, i.e. please keep the critical information in the
commit.

> Yes, I was confused by that, too. But I wonder if just the numbering is
> wrong? I.e. if [PATCH 1/2] was intended to be the cover letter 0/1 and
> the [PATCH 2/2] was intended to be the actual patch 1/1? Just a hint :)

Yeah, they also appear in two independent threads, and they shouldn't be.

Guilherme, please use `git format-patch` and `git send-email` (or `b4`
[1]), it will handle these details for you.

Thanks!

[1] https://b4.docs.kernel.org

Cheers,
Miguel