netdev - Re: [PATCH v1 1/3] rust: core abstractions for network PHY drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c0c00289-3bb3-4091-be78-8616e2ba90ee@lunn.ch>
Date: Mon, 2 Oct 2023 17:24:17 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org,
	miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH v1 1/3] rust: core abstractions for network PHY drivers

> +    /// 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