Message-Id: <20231004.084644.50784533959398755.fujita.tomonori@gmail.com>
Date: Wed, 04 Oct 2023 08:46:44 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, 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

On Mon, 2 Oct 2023 17:24:17 +0200
Andrew Lunn <andrew@...n.ch> wrote:

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

phy_id() is fine by me.

The complete type name is `net::phy::Device` so I guess that the
method names usually don't start with `phy`. But we maintain both C
and Rust so I think that we need a balance between them.


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

get/set_something names aren't commonly used in Rust, I guess. Some examples
follows in the standard library.

https://doc.rust-lang.org/stable/std/net/struct.TcpStream.html

there are set_linger(), set_nodelay(), set_read_timeout(),
set_write_timeout(). correspondingly, linger(), nodelay(),
read_timeout(), write_timeout() are provided.

https://doc.rust-lang.org/stable/std/io/struct.Cursor.html

There are set_position() and position().

As I wrote above, I don't think that we need to follow Rust naming
practices strictly, as long as there are patterns in Rust bindings.


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

I think that we can leave this name alone since tis_something() names
are used for OS related functions in Rust.

Powered by blists - more mailing lists