Re: [PATCH v1 1/3] rust: core abstractions for network PHY drivers
From: Andrew Lunn <andrew@lunn.ch>
Date: 2023-10-02 15:24:21
Also in:
rust-for-linux
+ /// Gets the id of the PHY.
+ pub fn id(&mut self) -> u32 {
+ let phydev = self.0.get();
+ // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+ unsafe { (*phydev).phy_id }
+ }I somewhat agree with GregKH here. It will be easier to review and maintain if the naming of well known things stay the same in the C and Rust world. So phy_id. However....
+ /// Gets the state of the PHY.
+ pub fn state(&mut self) -> DeviceState {
+ let phydev = self.0.get();
+ // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+ let state = unsafe { (*phydev).state };
+ match state {
+ bindings::phy_state::PHY_DOWN => DeviceState::Down,
+ bindings::phy_state::PHY_READY => DeviceState::Ready,
+ bindings::phy_state::PHY_HALTED => DeviceState::Halted,
+ bindings::phy_state::PHY_ERROR => DeviceState::Error,
+ bindings::phy_state::PHY_UP => DeviceState::Up,
+ bindings::phy_state::PHY_RUNNING => DeviceState::Running,
+ bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
+ bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
+ }
+ }
+
+ /// Returns true if the link is up.
+ pub fn get_link(&mut self) -> bool {
+ const LINK_IS_UP: u32 = 1;
+ let phydev = self.0.get();
+ // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+ unsafe { (*phydev).link() == LINK_IS_UP }
+ }Naming is hard. This one is trickier and shows a difference between C and Rust. C just does phydev->link and treats it as a boolean, setter/getters are not needed. But Rust does seem to need setter/getters, and it is a lot less clear what link() does. get_link() is a bit more obvious. has_link() would also work. But as GregKH said, get_foo() and put_foo() are often used to represent getting a reference on an object and releasing it. I am however of the opinion that many driver writers don't understand locking, so it is best to hide all the locking in the core. I would not actually expect to see a PHY driver need to take a reference on anything. Since we forced into a world of getter/setter, the previous one probably should be get_phy_id() and we want consistent set_ and get_ prefixes for plain accesses to members without further interpretation.
+
+ /// Returns true if auto-negotiation is enabled.
+ pub fn is_autoneg_enabled(&mut self) -> bool {
+ let phydev = self.0.get();
+ // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+ unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
+ }
Should this maybe be get_autoneg_enabled()? I don't know.
Andrew