Re: [RFC PATCH v3 1/3] rust: core abstractions for network PHY drivers

[Trevor Gross <tmgross@umich.edu>]

On Fri, Sep 29, 2023 at 4:39 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> [...]
> >> +    /// 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,
> >
> > Again, this feels like a maintance nightmare.
>
> As I wrote above, it's guaranteed that C and Rust code never
> diverge. However, as you said, suley it's troublesome to maintain this.
>
> We could directly use enum generated from C's enum at compile
> time. Then we can remove this conversion. The names are not pretty
> from Rust's view though.
>
> Any concerns from Rust side?
> [...]
>
>  /// Corresponds to the kernel's `enum phy_state`.
> -#[derive(PartialEq)]
> -pub enum DeviceState {
> -    /// PHY device and driver are not ready for anything.
> -    Down,
> -    /// PHY is ready to send and receive packets.
> -    Ready,
> -    /// PHY is up, but no polling or interrupts are done.
> -    Halted,
> -    /// PHY is up, but is in an error state.
> -    Error,
> -    /// PHY and attached device are ready to do work.
> -    Up,
> -    /// PHY is currently running.
> -    Running,
> -    /// PHY is up, but not currently plugged in.
> -    NoLink,
> -    /// PHY is performing a cable test.
> -    CableTest,
> -}
> +///
> +///  PHY device and driver are not ready for anything.
> +///  PHY_DOWN
> +///  PHY is ready to send and receive packets.
> +///  PHY_READY
> +///  PHY is up, but no polling or interrupts are done.
> +///  PHY_HALTED
> +///  PHY is up, but is in an error state.
> +///  PHY_ERROR
> +///  PHY and attached device are ready to do work.
> +///  PHY_UP
> +///  PHY is currently running.
> +///  PHY_RUNNING
> +///  PHY is up, but not currently plugged in.
> +///  PHY_NOLINK
> +///  PHY is performing a cable test.
> +///  PHY_CABLETEST
> +pub use bindings::phy_state as DeviceState;

I think there are three reasons we may prefer to recreate an enum from
C in Rust:

1. Size, C enums are typically an int but rust enums are u8 until they
   need to be bigger
2. Name consistency, rust is almost always CamelCase for enum variant
   names but the ones from bindings don't
3. Docs, if the enum is user-facing then there are benefits to documenting
   the variants rather than the entire enum (cleaner doc output, ide hints)

None of those are super strong on their own but together seems like
good enough reason to leave it as you have it. I think the most
important thing is making sure it is maintainable, incompatible
changes on the C side should fail rust compilation - which is the goal
of this conversation.