Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"
PATCH 1/2 Revert "rust: net: phy: fix off-by-one bit positions in device status accessors"
This reverts commit 3267aea7cdd5d5c8851718b148cd25ff1a5374ac.
It was not meant to be backported before 7.1.y -- its tag said:
--- rust/kernel/net/phy.rs | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)
rust/kernel/net/phy.rs +20 -18
@@ -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
Signature · 1 line
2.55.0
PATCH 2/2 rust: net: phy: fix off-by-one bit positions in device status accessors
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.
[ 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 ]
---
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(-)
rust/kernel/net/phy.rs +3 -3
@@ -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 2/2] rust: net: phy: fix off-by-one bit positions in device status accessors
Quoted 2 lines · Miguel Ojeda
Chunfeng: I assume you meant a patch like this instead in [1], right?
From the other thread:
Cheers,
Miguel
Sasha Levin
maintainerQuoted 1 line
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").
Signature · 2 lines
Thanks, Sasha
Miguel Ojeda
Acked-by Chunfeng SongQuoted 4 lines · Sasha Levin
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:
Cheers,
Miguel
Quoted 7 lines · Miguel Ojeda
Quoted 4 lines · Sasha Levin
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.
f22d349940cbis indeed the correct one.
Ah, will use that, thanks.
greg k-h
Quoted 1 line · Miguel Ojeda
This reverts commit
3267aea7cdd5d5c8851718b148cd25ff1a5374ac.
Odd, I don't see that commit id anywhere :(