Message-ID: <1f61dda0-1e5e-4cdb-991b-1107439ecc99@proton.me>
Date: Tue, 24 Oct 2023 16:23:20 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org
Cc: rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers

On 24.10.23 02:58, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>

[...]

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..2d821c2475e1
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,708 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{
> +    bindings,
> +    error::*,
> +    prelude::{vtable, Pin},
> +    str::CStr,
> +    types::Opaque,
> +};
> +use core::marker::PhantomData;
> +
> +/// PHY state machine states.
> +///
> +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state).

Please use `rustfmt`, this line is 109 characters long.

Also it might make sense to use a relative link, since then it also
works offline (though you have to build the C docs).

> +/// Some of PHY drivers access to the state of PHY's software state machine.
> +#[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,
> +}
> +
> +/// A mode of Ethernet communication.
> +///
> +/// PHY drivers get duplex information from hardware and update the current state.
> +pub enum DuplexMode {
> +    /// PHY is in full-duplex mode.
> +    Full,
> +    /// PHY is in half-duplex mode.
> +    Half,
> +    /// PHY is in unknown duplex mode.
> +    Unknown,
> +}
> +
> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's [`struct phy_device`](https://docs.kernel.org/networking/kapi.html#c.phy_device).
> +/// A [`Device`] instance is created when a callback in [`Driver``] is executed. A PHY driver executes
> +/// [`Driver`]'s methods during the callback.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique
> +// for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock held,
> +// thus guaranteeing that [`Driver::resume`] has exclusive access to the instance. [`Driver::resume`] and
> +// [`Driver::suspend`] also are called where only one thread can access to the instance.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function must only be called from the callbacks in `phy_driver`.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
> +        // the duration of `'a`.
> +        unsafe { &mut *ptr }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn phy_id(&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 }
> +    }
> +
> +    /// Gets the state of the PHY.
> +    pub fn state(&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 };
> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
> +        // when it becomes possible.
> +        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
> +        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,
> +            _ => DeviceState::Error,
> +        }
> +    }
> +
> +    /// Gets the current link state. It returns true if the link is up.
> +    pub fn get_link(&self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.link() == LINK_IS_UP
> +    }

Can we please change this name? I think Tomo is waiting for Andrew to
give his OK. All the other getter functions already follow the Rust
naming convention, so this one should as well. I think using
`is_link_up` would be ideal, since `link()` reads a bit weird in code:

     if dev.link() {
         // ...
     }

vs

     if dev.is_link_up() {
         // ...
     }

> +
> +    /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled.

Move the second sentence into a new line, it should not be part of the
one-line summary.

> +    pub fn is_autoneg_enabled(&self) -> bool {
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg() == bindings::AUTONEG_ENABLE
> +    }
> +
> +    /// Gets the current auto-negotiation state. It returns true if auto-negotiation is completed.
Same here.

--
Cheers,
Benno

> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
[...]

Powered by blists - more mailing lists