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

["Guilherme Giácomo Simões" <trintaeoitogc@gmail.com>]

Danilo Krummrich <dakr@kernel.org> writes:
>
> On Mon, Sep 30, 2024 at 09:39:56AM -0300, Guilherme Giacomo Simoes wrote:
> > This function increment the refcount by this command "bindings::get_device(prt)".
>
> 'increments', 'bindings::get_device(ptr)'
>
> I'd also say 'by a call to' instead of 'by this command'.
>
> > This can be confuse becuase, the function Arc::from_raw() from standard library, don't increment the refcount.
>
> Neither the stdlib one, nor the kernel one.
>
> > Then, this function "Device::from_raw()" will be renamed for don't make confusing in the future.
>
> Prefer the imperative, e.g. "Hence, rename `Device::from_raw` to avoid
> confusion with other `from_raw` semantics."
>
> >
> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
>
> Please make sure to run ./scripts/checkpatch.pl before submitting patches.
>
> I see the following two warnings:
>
> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
> #6:
> This function increment the refcount by this command "bindings::get_device(prt)".
>
> WARNING: 'becuase' may be misspelled - perhaps 'because'?
> #7:
> This can be confuse becuase, the function Arc::from_raw() from standard library, don't increment the refcount.
>
> Please also use a more descriptive commit summary, e.g.
> "rust: device: rename `Device::from_raw`".
>

Thanks for your suggestions...  I will make this changes.

> > ---
> >  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> {
>
> As a follow-up, it probably makes sense to also change the function body to
> just: `unsafe { Self::as_ref(ptr) }.into()`.

But if we change the function body that is suggested for you, the
function will not have your own refcount. If I don't wrong, the
Device::as_ref() expects the caller to have its own reference counter.
And if we change the behavior of function, your name don't need to be
changed, because your behavior will be equal the from_raw() from
standard library.

>
> >          // 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
> >