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

Chunfeng Song <[email protected]>
Newsgroups org.kernel.vger.rust-for-linux,org.kernel.vger.netdev,org.kernel.vger.stable
Message-ID <[email protected]>
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.

Hard-coded offsets will silently break again on the next layout
change, so use the bindgen-generated accessors (link(), autoneg(),
autoneg_complete()), which are always consistent with the C layout,
and 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: [email protected]
Signed-off-by: Chunfeng Song <[email protected]>
---
v2 -> v3:
  - Drop the links to previous versions from the commit message
    (Miguel Ojeda).
  - Keep the changelog here, after the separator, as is customary.

v1 -> v2:
  - Use the bindgen-generated accessors (link(), autoneg(),
    autoneg_complete()) instead of the hard-coded bit numbers, as
    suggested by Andrew Lunn, since the offsets drift whenever the
    layout changes.
  - Update the Fixes: tag to 2796ff1e3dca, the commit that inserted
    is_genphy_driven and shifted the fields (the offsets were correct
    when the abstraction was merged in v6.8).
  - Add the net tree prefix to the subject.

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

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 956cda573ddb..14dd1e6cb380 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -124,38 +124,30 @@ pub fn state(&self) -> DeviceState {
     ///
     /// It returns true if the link is up.
     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.
+        let phydev = self.0.get();
         // 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
+        unsafe { (*phydev).link() == 1 }
     }
 
     /// Gets the current auto-negotiation configuration.
     ///
     /// It returns true if auto-negotiation is enabled.
     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.
+        let phydev = self.0.get();
         // 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)
+        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
     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.
+        let phydev = self.0.get();
         // 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
+        unsafe { (*phydev).autoneg_complete() == 1 }
     }
 
     /// Sets the speed of the PHY.
-- 
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.