[PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Chunfeng Song]

The hand-written bitfield offsets in is_link_up(), is_autoneg_enabled()
and is_autoneg_completed() were correct when the abstraction was
merged: at that time autoneg, link, and autoneg_complete were at bits
13, 14, and 15 of struct phy_device's first bitfield unit. Commit
2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct phy_device")
later inserted is_genphy_driven just before autoneg, shifting the three
fields up by one, so the accessors now read:

  is_link_up()           reads bit 14 = autoneg
  is_autoneg_enabled()   reads bit 13 = is_genphy_driven
  is_autoneg_completed() reads bit 15 = link

The official ax88796b Rust driver uses all three accessors in its
link-change handling, so it inherits the bug. On genphy-driven devices
is_genphy_driven is always 1, which masks the broken
is_autoneg_enabled() check.

The ordinary bindgen accessors take &self. Calling them through
(*phydev).link() would create a shared reference to the complete
bindings::phy_device, which is not appropriate for an object wrapped in
Opaque.

Use the bindgen-generated raw accessors (link_raw(), autoneg_raw(),
and autoneg_complete_raw()) instead. They retain the bit positions and
endianness handling generated from the C layout without creating a Rust
reference to the complete phy_device. Drop the hand-written numbers
together with the TODO comment that marked them as a stopgap.

Found by a static equivalence audit (C2RustDrv, a C-to-Rust driver
migration tool) that compares hand-written bitfield offsets against
the bindgen layout of struct phy_device. Verified by building the
bindings and checking the generated accessors; no runtime testing was
possible without PHY hardware.

Fixes: 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct phy_device")
Cc: stable@vger.kernel.org
Signed-off-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>
---
v4:
  - Use the bindgen-generated raw accessors to avoid creating a shared
    reference to the complete C `phy_device`, as suggested by Jakub Kicinski.
  - Update the SAFETY comments to describe pointer validity and the
    callback-context preconditions.
  - Add `#[inline]` to the three small forwarding methods.

v3: https://lore.kernel.org/r/20260825111334.795732-1-springbreeze@stu.pku.edu.cn/
  - Use the bindgen-generated accessors instead of hard-coded bit numbers.
  - Update the Fixes tag and add the net tree subject prefix.

v2: https://lore.kernel.org/r/20260824111227.742645-1-springbreeze@stu.pku.edu.cn/
  - Use bindgen-generated accessors as suggested by Andrew Lunn.
  - Explain that the offsets drifted after `is_genphy_driven` was added.

v1: https://lore.kernel.org/r/20260823103644.342849-1-springbreeze@stu.pku.edu.cn/
 rust/kernel/net/phy.rs | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 956cda573ddb..3f1c1637f027 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -123,39 +123,42 @@ pub fn state(&self) -> DeviceState {
     /// Gets the current link state.
     ///
     /// It returns true if the link is up.
+    #[inline]
     pub fn is_link_up(&self) -> bool {
-        const LINK_IS_UP: u64 = 1;
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
-        // SAFETY: The struct invariant ensures that we may access
-        // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(14, 1) == LINK_IS_UP
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe { bindings::phy_device::link_raw(phydev) == 1 }
     }
 
     /// Gets the current auto-negotiation configuration.
     ///
     /// It returns true if auto-negotiation is enabled.
+    #[inline]
     pub fn is_autoneg_enabled(&self) -> bool {
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
-        // SAFETY: The struct invariant ensures that we may access
-        // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(13, 1) == u64::from(bindings::AUTONEG_ENABLE)
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe {
+            bindings::phy_device::autoneg_raw(phydev) == bindings::AUTONEG_ENABLE
+        }
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
+    #[inline]
     pub fn is_autoneg_completed(&self) -> bool {
-        const AUTONEG_COMPLETED: u64 = 1;
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
-        // SAFETY: The struct invariant ensures that we may access
-        // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(15, 1) == AUTONEG_COMPLETED
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe { bindings::phy_device::autoneg_complete_raw(phydev) == 1 }
     }
 
     /// Sets the speed of the PHY.

base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
-- 
2.43.0

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Gary Guo]

On Tue Sep 1, 2026 at 4:18 AM BST, Chunfeng Song wrote:
> The hand-written bitfield offsets in is_link_up(), is_autoneg_enabled()
> and is_autoneg_completed() were correct when the abstraction was
> merged: at that time autoneg, link, and autoneg_complete were at bits
> 13, 14, and 15 of struct phy_device's first bitfield unit. Commit
> 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct phy_device")
> later inserted is_genphy_driven just before autoneg, shifting the three
> fields up by one, so the accessors now read:
>
>   is_link_up()           reads bit 14 = autoneg
>   is_autoneg_enabled()   reads bit 13 = is_genphy_driven
>   is_autoneg_completed() reads bit 15 = link
>
> The official ax88796b Rust driver uses all three accessors in its
> link-change handling, so it inherits the bug. On genphy-driven devices
> is_genphy_driven is always 1, which masks the broken
> is_autoneg_enabled() check.
>
> The ordinary bindgen accessors take &self. Calling them through
> (*phydev).link() would create a shared reference to the complete
> bindings::phy_device, which is not appropriate for an object wrapped in
> Opaque.
>
> Use the bindgen-generated raw accessors (link_raw(), autoneg_raw(),
> and autoneg_complete_raw()) instead. They retain the bit positions and
> endianness handling generated from the C layout without creating a Rust
> reference to the complete phy_device. Drop the hand-written numbers
> together with the TODO comment that marked them as a stopgap.
>
> Found by a static equivalence audit (C2RustDrv, a C-to-Rust driver
> migration tool) that compares hand-written bitfield offsets against
> the bindgen layout of struct phy_device. Verified by building the
> bindings and checking the generated accessors; no runtime testing was
> possible without PHY hardware.

Might be worth mentioning that this is change is only possible since this
April's bindgen version bump.

This is a bindgen version that we requested
https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
bindgen 0.71.

This will affect backports.

>
> Fixes: 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct phy_device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>
> ---
> v4:
>   - Use the bindgen-generated raw accessors to avoid creating a shared
>     reference to the complete C `phy_device`, as suggested by Jakub Kicinski.
>   - Update the SAFETY comments to describe pointer validity and the
>     callback-context preconditions.
>   - Add `#[inline]` to the three small forwarding methods.
>
> v3: https://lore.kernel.org/r/20260825111334.795732-1-springbreeze@stu.pku.edu.cn/
>   - Use the bindgen-generated accessors instead of hard-coded bit numbers.
>   - Update the Fixes tag and add the net tree subject prefix.
>
> v2: https://lore.kernel.org/r/20260824111227.742645-1-springbreeze@stu.pku.edu.cn/
>   - Use bindgen-generated accessors as suggested by Andrew Lunn.
>   - Explain that the offsets drifted after `is_genphy_driven` was added.
>
> v1: https://lore.kernel.org/r/20260823103644.342849-1-springbreeze@stu.pku.edu.cn/
>  rust/kernel/net/phy.rs | 43 ++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 956cda573ddb..3f1c1637f027 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -123,39 +123,42 @@ pub fn state(&self) -> DeviceState {
>      /// Gets the current link state.
>      ///
>      /// It returns true if the link is up.
> +    #[inline]
>      pub fn is_link_up(&self) -> bool {
> -        const LINK_IS_UP: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> -        // SAFETY: The struct invariant ensures that we may access
> -        // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(14, 1) == LINK_IS_UP
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe { bindings::phy_device::link_raw(phydev) == 1 }
>      }
>  
>      /// Gets the current auto-negotiation configuration.
>      ///
>      /// It returns true if auto-negotiation is enabled.
> +    #[inline]
>      pub fn is_autoneg_enabled(&self) -> bool {
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> -        // SAFETY: The struct invariant ensures that we may access
> -        // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(13, 1) == u64::from(bindings::AUTONEG_ENABLE)
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe {
> +            bindings::phy_device::autoneg_raw(phydev) == bindings::AUTONEG_ENABLE

IMO it's little bit awkward to have the comparision be inside unsafe block.
Perhaps

    let autoneg = unsafe { bindings::phy_device::autoneg_raw(phydev) };
    autoneg == bindings::AUTONEG_ENABLE

?

Best,
Gary

> +        }
>      }
>  
>      /// Gets the current auto-negotiation state.
>      ///
>      /// It returns true if auto-negotiation is completed.
> +    #[inline]
>      pub fn is_autoneg_completed(&self) -> bool {
> -        const AUTONEG_COMPLETED: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> -        // SAFETY: The struct invariant ensures that we may access
> -        // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(15, 1) == AUTONEG_COMPLETED
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe { bindings::phy_device::autoneg_complete_raw(phydev) == 1 }
>      }
>  
>      /// Sets the speed of the PHY.
>
> base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:03 PM Gary Guo <gary@garyguo.net> wrote:
>
> Might be worth mentioning that this is change is only possible since this
> April's bindgen version bump.
>
> This is a bindgen version that we requested
> https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
> bindgen 0.71.
>
> This will affect backports.

Definitely, if the patch shouldn't be automatically backported, then
we should limit it with a `#` comment, e.g.

  Cc: stable@vger.kernel.org # Only 7.1.y and later (requires
bindgen's raw pointer accessors).

> IMO it's little bit awkward to have the comparision be inside unsafe block.
> Perhaps
>
>     let autoneg = unsafe { bindings::phy_device::autoneg_raw(phydev) };
>     autoneg == bindings::AUTONEG_ENABLE
>
> ?

Agreed, yeah.

Cheers,
Miguel

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:47 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Definitely, if the patch shouldn't be automatically backported, then
> we should limit it with a `#` comment, e.g.
>
>   Cc: stable@vger.kernel.org # Only 7.1.y and later (requires
> bindgen's raw pointer accessors).

And then we should have a custom version sent to the other versions, of course.

Cheers,
Miguel

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Chunfeng Song]

Thanks for the guidance, all applied in v5:

  - Cc: stable is now scoped to 7.1.y and later, and the commit message
    notes the bindgen 0.71 dependency:

      Cc: stable@vger.kernel.org # Only 7.1.y and later (requires bindgen's
      raw pointer accessors).

  - The comparison is moved out of the `unsafe` block so that only the
    raw accessor call stays inside, in all three methods.

For the older stable branches that contain 2796ff1e3dca but whose
minimum bindgen predates the raw accessors, I'll send a separate
backport that fixes the hand-written offsets directly, once this lands
in mainline.

Cheers,
Chunfeng

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 4:44 PM Chunfeng Song
<springbreeze@stu.pku.edu.cn> wrote:
>
> For the older stable branches that contain 2796ff1e3dca but whose
> minimum bindgen predates the raw accessors, I'll send a separate
> backport that fixes the hand-written offsets directly, once this lands
> in mainline.

That is very appreciated, thanks! :)

Cheers,
Miguel

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Andrew Lunn]

> Might be worth mentioning that this is change is only possible since this
> April's bindgen version bump.
> 
> This is a bindgen version that we requested
> https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
> bindgen 0.71.
> 
> This will affect backports.

Probably a FAQ, but what is the policy about toolchain versions for
stable kernels?

       Andrew

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Probably a FAQ, but what is the policy about toolchain versions for
> stable kernels?

As far as I understand it, we are supposed to avoid changes unless
they are truly needed.

So there can be exceptions, but I doubt we can easily change a minimum
version without breaking a lot of users (e.g. distributions or users
using those distributions that have a particular version provided).

But Cc'ing stable just in case.

Cheers,
Miguel

Re: [PATCH net v4] rust: net: phy: fix off-by-one bit positions in device status accessors

[Greg KH]

On Tue, Sep 01, 2026 at 02:51:12PM +0200, Miguel Ojeda wrote:
> On Tue, Sep 1, 2026 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Probably a FAQ, but what is the policy about toolchain versions for
> > stable kernels?
> 
> As far as I understand it, we are supposed to avoid changes unless
> they are truly needed.
> 
> So there can be exceptions, but I doubt we can easily change a minimum
> version without breaking a lot of users (e.g. distributions or users
> using those distributions that have a particular version provided).
> 
> But Cc'ing stable just in case.

Stable kernels should be able to build with newer toolchains, but we
shouldn't break them by forcing a toolchain update unless there is a
very good reason to do so.

thanks,

greg k-h