[PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[Miguel Ojeda]

This reverts commit 3267aea7cdd5d5c8851718b148cd25ff1a5374ac.

It was not meant to be backported before 7.1.y -- its tag said:

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

Cc: Chunfeng Song <springbreeze@stu.pku.edu.cn>
Cc: FUJITA Tomonori <fujita.tomonori@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/stable/SEWP216MB9770116738884C46F156A686FAF98D2@SEWP216MB977011.KORP216.PROD.OUTLOOK.COM/
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/net/phy.rs | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 1d17bf1ae87d..bf6272d87a7b 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -123,37 +123,39 @@ 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 {
-        let phydev = self.0.get().cast_const();
-        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
-        // `struct phy_device`, and there is no concurrent write to this field.
-        let link = unsafe { bindings::phy_device::link_raw(phydev) };
-        link == 1
+        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
     }
 
     /// Gets the current auto-negotiation configuration.
     ///
     /// It returns true if auto-negotiation is enabled.
-    #[inline]
     pub fn is_autoneg_enabled(&self) -> bool {
-        let phydev = self.0.get().cast_const();
-        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
-        // `struct phy_device`, and there is no concurrent write to this field.
-        let autoneg = unsafe { bindings::phy_device::autoneg_raw(phydev) };
-        autoneg == bindings::AUTONEG_ENABLE
+        // 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)
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
-    #[inline]
     pub fn is_autoneg_completed(&self) -> bool {
-        let phydev = self.0.get().cast_const();
-        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
-        // `struct phy_device`, and there is no concurrent write to this field.
-        let completed = unsafe { bindings::phy_device::autoneg_complete_raw(phydev) };
-        completed == 1
+        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
     }
 
     /// Sets the speed of the PHY.

base-commit: 1b357ecb321392158d507b04672ffee57bfa071d
-- 
2.55.0

[PATCH 6.18.y 2/2] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

commit 6fb0a9d9071f1ff0cc5cfc0782302d9c90d642cb upstream.

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
read_status() implementation, so it inherits the bug.
phy_attach_direct() sets is_genphy_driven only when it falls back to
the generic driver, and ax88796b has a real driver, so
is_genphy_driven stays 0. The broken is_autoneg_enabled() therefore
reads bit 13 as 0, compares it against AUTONEG_ENABLE (1), and always
returns false, so read_status() never reaches the
resolve_aneg_linkmode() call.

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.

The raw accessors are only emitted by bindgen 0.71 and later, and were
added at the Rust-for-Linux project's request, so this fix can only be
backported to stable branches whose minimum bindgen version is at least
that, hence the scope on the Cc: stable line below.

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 # Only 7.1.y and later (requires bindgen's raw pointer accessors).
Link: https://github.com/rust-lang/rust-bindgen/issues/2674
Signed-off-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>
Reviewed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Link: https://patch.msgid.link/20260910055110.167110-1-springbreeze@stu.pku.edu.cn
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
[ For 6.18.y, re-do the commit manually adjusting the bit numbers to
  make it work with the `bindgen` version available there as Chunfeng
  mentions in [1]. - Miguel ]
Link: https://lore.kernel.org/stable/SEWP216MB9770116738884C46F156A686FAF98D2@SEWP216MB977011.KORP216.PROD.OUTLOOK.COM/ [1]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
Chunfeng: I assume you meant a patch like this instead in [1], right?

 rust/kernel/net/phy.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index bf6272d87a7b..5963dbbd54af 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -130,7 +130,7 @@ pub fn is_link_up(&self) -> bool {
         // 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
+        bit_field.get(15, 1) == LINK_IS_UP
     }

     /// Gets the current auto-negotiation configuration.
@@ -142,7 +142,7 @@ pub fn is_autoneg_enabled(&self) -> bool {
         // 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)
+        bit_field.get(14, 1) == u64::from(bindings::AUTONEG_ENABLE)
     }

     /// Gets the current auto-negotiation state.
@@ -155,7 +155,7 @@ pub fn is_autoneg_completed(&self) -> bool {
         // 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
+        bit_field.get(16, 1) == AUTONEG_COMPLETED
     }

     /// Sets the speed of the PHY.
--
2.55.0

Re: [PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[Sasha Levin]

> This reverts commit 3267aea7cdd5d5c8851718b148cd25ff1a5374ac.

Greg asked for this revert and re-add in the 6.18.54-rc1 review, so
I'll leave it for him to queue.

One fix for when it's applied: the SHA above is the stable-rc one. The
commit in v6.18.54 is f22d349940cb ("rust: net: phy: fix off-by-one bit
positions in device status accessors").

-- 
Thanks,
Sasha

Re: [PATCH 6.18.y 2/2] rust: net: phy: fix off-by-one bit positions in device status accessors

[Miguel Ojeda]

On Mon, Sep 28, 2026 at 12:49 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Chunfeng: I assume you meant a patch like this instead in [1], right?

From the other thread:

Acked-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>

Cheers,
Miguel

Re: [PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[Miguel Ojeda]

On Tue, Sep 29, 2026 at 12:39 AM Sasha Levin <sashal@kernel.org> wrote:
>
> One fix for when it's applied: the SHA above is the stable-rc one. The
> commit in v6.18.54 is f22d349940cb ("rust: net: phy: fix off-by-one bit
> positions in device status accessors").

Ah, right, sorry about that. f22d349940cb is indeed the correct one.

By the way, adding Chunfeng's tag from the other review thread:

Acked-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>

Cheers,
Miguel

Re: [PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[Greg KH]

On Mon, Sep 28, 2026 at 12:48:54PM +0200, Miguel Ojeda wrote:
> This reverts commit 3267aea7cdd5d5c8851718b148cd25ff1a5374ac.

Odd, I don't see that commit id anywhere :(

Re: [PATCH 6.18.y 1/2] Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"

[Greg KH]

On Tue, Sep 29, 2026 at 07:14:40AM +0200, Miguel Ojeda wrote:
> On Tue, Sep 29, 2026 at 12:39 AM Sasha Levin <sashal@kernel.org> wrote:
> >
> > One fix for when it's applied: the SHA above is the stable-rc one. The
> > commit in v6.18.54 is f22d349940cb ("rust: net: phy: fix off-by-one bit
> > positions in device status accessors").
> 
> Ah, right, sorry about that. f22d349940cb is indeed the correct one.

Ah, will use that, thanks.

greg k-h