Thread (14 messages) 14 messages, 4 authors, 2023-10-03

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help