rust: cast to the proper type

[PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 4 months ago

Use the ffi type rather than the resolved underlying type.

Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/net/phy.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 32ea43ece646..905e6534c083 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -163,17 +163,17 @@ pub fn set_speed(&mut self, speed: u32) {
         let phydev = self.0.get();
         // SAFETY: The struct invariant ensures that we may access
         // this field without additional synchronization.
-        unsafe { (*phydev).speed = speed as i32 };
+        unsafe { (*phydev).speed = speed as crate::ffi::c_int };
     }
 
     /// Sets duplex mode.
     pub fn set_duplex(&mut self, mode: DuplexMode) {
         let phydev = self.0.get();
         let v = match mode {
-            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
-            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
-            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
-        };
+            DuplexMode::Full => bindings::DUPLEX_FULL,
+            DuplexMode::Half => bindings::DUPLEX_HALF,
+            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
+        } as crate::ffi::c_int;
         // SAFETY: The struct invariant ensures that we may access
         // this field without additional synchronization.
         unsafe { (*phydev).duplex = v };

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250611-correct-type-cast-1de8876ddfc1

Best regards,
--  
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH] rust: cast to the proper type

Posted by Alice Ryhl 3 months, 2 weeks ago

On Wed, Jun 11, 2025 at 06:28:47AM -0400, Tamir Duberstein wrote:
> Use the ffi type rather than the resolved underlying type.
> 
> Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Please use unqualified imports.

>  rust/kernel/net/phy.rs | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 32ea43ece646..905e6534c083 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -163,17 +163,17 @@ pub fn set_speed(&mut self, speed: u32) {
>          let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        unsafe { (*phydev).speed = speed as i32 };
> +        unsafe { (*phydev).speed = speed as crate::ffi::c_int };

unsafe { (*phydev).speed = speed as c_int };

>      }
>  
>      /// Sets duplex mode.
>      pub fn set_duplex(&mut self, mode: DuplexMode) {
>          let phydev = self.0.get();
>          let v = match mode {
> -            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
> -            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
> -            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
> -        };
> +            DuplexMode::Full => bindings::DUPLEX_FULL,
> +            DuplexMode::Half => bindings::DUPLEX_HALF,
> +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> +        } as crate::ffi::c_int;

I would keep the imports on each line.

let v = match mode {
    DuplexMode::Full => bindings::DUPLEX_FULL as c_int,
    DuplexMode::Half => bindings::DUPLEX_HALF as c_int,
    DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as c_int,
};

Alice

>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
>          unsafe { (*phydev).duplex = v };
> 
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250611-correct-type-cast-1de8876ddfc1
> 
> Best regards,
> --  
> Tamir Duberstein <tamird@gmail.com>
>

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 4:39 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Jun 11, 2025 at 06:28:47AM -0400, Tamir Duberstein wrote:
> > Use the ffi type rather than the resolved underlying type.
> >
> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> Please use unqualified imports.

OK, I will change this to two patches in v2; the first will change all
ffi references in this file to unqualified, the second will be this
with unqualified references.

>
> >  rust/kernel/net/phy.rs | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> > index 32ea43ece646..905e6534c083 100644
> > --- a/rust/kernel/net/phy.rs
> > +++ b/rust/kernel/net/phy.rs
> > @@ -163,17 +163,17 @@ pub fn set_speed(&mut self, speed: u32) {
> >          let phydev = self.0.get();
> >          // SAFETY: The struct invariant ensures that we may access
> >          // this field without additional synchronization.
> > -        unsafe { (*phydev).speed = speed as i32 };
> > +        unsafe { (*phydev).speed = speed as crate::ffi::c_int };
>
> unsafe { (*phydev).speed = speed as c_int };
>
> >      }
> >
> >      /// Sets duplex mode.
> >      pub fn set_duplex(&mut self, mode: DuplexMode) {
> >          let phydev = self.0.get();
> >          let v = match mode {
> > -            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
> > -            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
> > -            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
> > -        };
> > +            DuplexMode::Full => bindings::DUPLEX_FULL,
> > +            DuplexMode::Half => bindings::DUPLEX_HALF,
> > +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> > +        } as crate::ffi::c_int;
>
> I would keep the imports on each line.
>
> let v = match mode {
>     DuplexMode::Full => bindings::DUPLEX_FULL as c_int,
>     DuplexMode::Half => bindings::DUPLEX_HALF as c_int,
>     DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as c_int,
> };

Could you help me understand why that's better?

Re: [PATCH] rust: cast to the proper type

Posted by Alice Ryhl 4 months ago

On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Use the ffi type rather than the resolved underlying type.
>
> Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")

Does this need to be backported? If not, I wouldn't include a Fixes tag.

> +            DuplexMode::Full => bindings::DUPLEX_FULL,
> +            DuplexMode::Half => bindings::DUPLEX_HALF,
> +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> +        } as crate::ffi::c_int;

This file imports the prelude, so this can just be c_int without the
crate::ffI:: path.

Alice

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 4 months ago

On Wed, Jun 11, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Use the ffi type rather than the resolved underlying type.
> >
> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
>
> Does this need to be backported? If not, I wouldn't include a Fixes tag.

I'm fine with omitting it. I wanted to leave a breadcrumb to the
commit that introduced the current code.

>
> > +            DuplexMode::Full => bindings::DUPLEX_FULL,
> > +            DuplexMode::Half => bindings::DUPLEX_HALF,
> > +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> > +        } as crate::ffi::c_int;
>
> This file imports the prelude, so this can just be c_int without the
> crate::ffI:: path.

This has come up a few times now; should we consider denying
unused_qualifications?

https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNUSED_QUALIFICATIONS.html

Re: [PATCH] rust: cast to the proper type

Posted by FUJITA Tomonori 4 months ago

On Wed, 11 Jun 2025 09:30:46 -0400
Tamir Duberstein <tamird@gmail.com> wrote:

> On Wed, Jun 11, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >
>> > Use the ffi type rather than the resolved underlying type.
>> >
>> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
>>
>> Does this need to be backported? If not, I wouldn't include a Fixes tag.
> 
> I'm fine with omitting it. I wanted to leave a breadcrumb to the
> commit that introduced the current code.

I also don't think this tag is necessary because this is not a bug
fix. And since this tag points to the file's initial commit, I don't
think it's particularly useful.

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 4 months ago

On Wed, Jun 11, 2025 at 8:48 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 11 Jun 2025 09:30:46 -0400
> Tamir Duberstein <tamird@gmail.com> wrote:
>
> > On Wed, Jun 11, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >> >
> >> > Use the ffi type rather than the resolved underlying type.
> >> >
> >> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> >>
> >> Does this need to be backported? If not, I wouldn't include a Fixes tag.
> >
> > I'm fine with omitting it. I wanted to leave a breadcrumb to the
> > commit that introduced the current code.
>
> I also don't think this tag is necessary because this is not a bug
> fix. And since this tag points to the file's initial commit, I don't
> think it's particularly useful.

Would you be OK stripping the tag on apply, or would you like me to send v2?

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 3 months, 3 weeks ago

On Wed, Jun 11, 2025 at 8:54 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Wed, Jun 11, 2025 at 8:48 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > On Wed, 11 Jun 2025 09:30:46 -0400
> > Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > > On Wed, Jun 11, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >>
> > >> On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
> > >> >
> > >> > Use the ffi type rather than the resolved underlying type.
> > >> >
> > >> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> > >>
> > >> Does this need to be backported? If not, I wouldn't include a Fixes tag.
> > >
> > > I'm fine with omitting it. I wanted to leave a breadcrumb to the
> > > commit that introduced the current code.
> >
> > I also don't think this tag is necessary because this is not a bug
> > fix. And since this tag points to the file's initial commit, I don't
> > think it's particularly useful.
>
> Would you be OK stripping the tag on apply, or would you like me to send v2?

Hi Tomo, gentle ping here. Does this look reasonable to you, with the
Fixes tag stripped on apply?

Re: [PATCH] rust: cast to the proper type

Posted by FUJITA Tomonori 3 months, 3 weeks ago

On Thu, 19 Jun 2025 11:29:56 -0400
Tamir Duberstein <tamird@gmail.com> wrote:

>> > >> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
>> > >>
>> > >> Does this need to be backported? If not, I wouldn't include a Fixes tag.
>> > >
>> > > I'm fine with omitting it. I wanted to leave a breadcrumb to the
>> > > commit that introduced the current code.
>> >
>> > I also don't think this tag is necessary because this is not a bug
>> > fix. And since this tag points to the file's initial commit, I don't
>> > think it's particularly useful.
>>
>> Would you be OK stripping the tag on apply, or would you like me to send v2?
> 
> Hi Tomo, gentle ping here. Does this look reasonable to you, with the
> Fixes tag stripped on apply?

Yeah, if you drop the Fixes tag, it's fine by me.

Thanks,

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 3 months, 3 weeks ago

On Thu, Jun 19, 2025 at 6:55 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 19 Jun 2025 11:29:56 -0400
> Tamir Duberstein <tamird@gmail.com> wrote:
>
> >> > >> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> >> > >>
> >> > >> Does this need to be backported? If not, I wouldn't include a Fixes tag.
> >> > >
> >> > > I'm fine with omitting it. I wanted to leave a breadcrumb to the
> >> > > commit that introduced the current code.
> >> >
> >> > I also don't think this tag is necessary because this is not a bug
> >> > fix. And since this tag points to the file's initial commit, I don't
> >> > think it's particularly useful.
> >>
> >> Would you be OK stripping the tag on apply, or would you like me to send v2?
> >
> > Hi Tomo, gentle ping here. Does this look reasonable to you, with the
> > Fixes tag stripped on apply?
>
> Yeah, if you drop the Fixes tag, it's fine by me.

Thanks. Would you mind adding your Acked-by?

Re: [PATCH] rust: cast to the proper type

Posted by FUJITA Tomonori 3 months, 3 weeks ago

On Thu, 19 Jun 2025 19:24:14 -0400
Tamir Duberstein <tamird@gmail.com> wrote:

>> >> > >> > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
>> >> > >>
>> >> > >> Does this need to be backported? If not, I wouldn't include a Fixes tag.
>> >> > >
>> >> > > I'm fine with omitting it. I wanted to leave a breadcrumb to the
>> >> > > commit that introduced the current code.
>> >> >
>> >> > I also don't think this tag is necessary because this is not a bug
>> >> > fix. And since this tag points to the file's initial commit, I don't
>> >> > think it's particularly useful.
>> >>
>> >> Would you be OK stripping the tag on apply, or would you like me to send v2?
>> >
>> > Hi Tomo, gentle ping here. Does this look reasonable to you, with the
>> > Fixes tag stripped on apply?
>>
>> Yeah, if you drop the Fixes tag, it's fine by me.
> 
> Thanks. Would you mind adding your Acked-by?

With the tag dropped,

Acked-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Re: [PATCH] rust: cast to the proper type

Posted by Miguel Ojeda 3 months, 3 weeks ago

On Fri, Jun 20, 2025 at 3:05 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> With the tag dropped,
>
> Acked-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

I guess this will go via netdev, but if you want me to pick it up,
please let me know.

Cheers,
Miguel

Re: [PATCH] rust: cast to the proper type

Posted by Tamir Duberstein 4 months ago

On Wed, Jun 11, 2025 at 9:30 AM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Wed, Jun 11, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Wed, Jun 11, 2025 at 12:28 PM Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > Use the ffi type rather than the resolved underlying type.
> > >
> > > Fixes: f20fd5449ada ("rust: core abstractions for network PHY drivers")
> >
> > Does this need to be backported? If not, I wouldn't include a Fixes tag.
>
> I'm fine with omitting it. I wanted to leave a breadcrumb to the
> commit that introduced the current code.
>
> >
> > > +            DuplexMode::Full => bindings::DUPLEX_FULL,
> > > +            DuplexMode::Half => bindings::DUPLEX_HALF,
> > > +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> > > +        } as crate::ffi::c_int;
> >
> > This file imports the prelude, so this can just be c_int without the
> > crate::ffI:: path.
>
> This has come up a few times now; should we consider denying
> unused_qualifications?
>
> https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNUSED_QUALIFICATIONS.html

I should point out also that every reference to crate::ffi::c_int in
this file is fully qualified, so I think this should be a separate
change.

Re: [PATCH] rust: cast to the proper type

Posted by FUJITA Tomonori 4 months ago

On Wed, 11 Jun 2025 09:41:08 -0400
Tamir Duberstein <tamird@gmail.com> wrote:

>> > > +            DuplexMode::Full => bindings::DUPLEX_FULL,
>> > > +            DuplexMode::Half => bindings::DUPLEX_HALF,
>> > > +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
>> > > +        } as crate::ffi::c_int;
>> >
>> > This file imports the prelude, so this can just be c_int without the
>> > crate::ffI:: path.
>>
>> This has come up a few times now; should we consider denying
>> unused_qualifications?
>>
>> https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNUSED_QUALIFICATIONS.html
> 
> I should point out also that every reference to crate::ffi::c_int in
> this file is fully qualified, so I think this should be a separate
> change.

I can take care of this file.

Thanks,