[RFC PATCH v3 0/3] Rust abstractions for network PHY drivers

[RFC PATCH v3 0/3] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

This patchset adds Rust abstractions for network PHY drivers. It
doesn't fully cover the C APIs for PHY drivers yet but I think that
it's already useful. I implement two PHY drivers (Asix AX88772A PHYs
and Realtek Generic FE-GE). Seems they work well with real hardware.

Thanks for lots of feedback on v2!

There is no major changes from v2; cleanups and comment improvement.

v1:
https://lwn.net/ml/rust-for-linux/20230913133609.1668758-1-fujita.tomonori@gmail.com/
v2:
https://lwn.net/ml/rust-for-linux/20230924064902.1339662-1-fujita.tomonori@gmail.com/


FUJITA Tomonori (3):
  rust: core abstractions for network PHY drivers
  MAINTAINERS: add Rust PHY abstractions to the ETHERNET PHY LIBRARY
  net: phy: add Rust Asix PHY driver

 MAINTAINERS                      |   2 +
 drivers/net/phy/Kconfig          |   7 +
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 ++++++
 rust/Makefile                    |   1 +
 rust/bindings/bindings_helper.h  |   3 +
 rust/kernel/lib.rs               |   3 +
 rust/kernel/net.rs               |   6 +
 rust/kernel/net/phy.rs           | 706 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 10 files changed, 863 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

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

[FUJITA Tomonori]

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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/Makefile                   |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 706 ++++++++++++++++++++++++++++++++
 5 files changed, 719 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

diff --git a/rust/Makefile b/rust/Makefile
index 87958e864be0..f67e55945b36 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
       cmd_bindgen = \
 	$(BINDGEN) $< $(bindgen_target_flags) \
 		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
+		--rustified-enum phy_state\
 		--no-debug '.*' \
 		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
 		$(bindgen_target_cflags) $(bindgen_target_extra)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c91a3c24f607..ec4ee09a34ad 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,9 @@
 
 #include <kunit/test.h>
 #include <linux/errname.h>
+#include <linux/ethtool.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e8811700239a..0588422e273c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -14,6 +14,7 @@
 #![no_std]
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
+#![feature(const_maybe_uninit_zeroed)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(receiver_trait)]
@@ -36,6 +37,8 @@
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..b49b052969e5
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_PHYLIB)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..335dfd7c9ddf
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,706 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
+use core::marker::PhantomData;
+
+/// 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,
+}
+
+/// Represents duplex mode.
+pub enum DuplexMode {
+    /// Full-duplex mode
+    Half,
+    /// Half-duplex mode
+    Full,
+    /// Unknown
+    Unknown,
+}
+
+/// Wraps the kernel's `struct phy_device`.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
+    /// may read or write to the `phy_device` object.
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// 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 }
+    }
+
+    /// 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 }
+    }
+
+    /// 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 }
+    }
+
+    /// Returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&mut self) -> bool {
+        const AUTONEG_COMPLETED: u32 = 1;
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).autoneg_complete() == AUTONEG_COMPLETED }
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&mut self, speed: u32) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).speed = speed as i32 };
+    }
+
+    /// Sets duplex mode.
+    pub fn set_duplex(&mut self, mode: DuplexMode) {
+        let phydev = self.0.get();
+        let v = match mode {
+            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
+            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
+            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
+        };
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).duplex = v };
+    }
+
+    /// Reads a given C22 PHY register.
+    pub fn read(&mut self, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Writes a given C22 PHY register.
+    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
+        })
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Resolves the advertisements into PHY settings.
+    pub fn resolve_aneg_linkmode(&mut self) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
+    }
+
+    /// Executes software reset the PHY via BMCR_RESET bit.
+    pub fn genphy_soft_reset(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // so an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_init_hw(phydev) })
+    }
+
+    /// Starts auto-negotiation.
+    pub fn start_aneg(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_start_aneg(phydev) })
+    }
+
+    /// Resumes the PHY via BMCR_PDOWN bit.
+    pub fn genphy_resume(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_resume(phydev) })
+    }
+
+    /// Suspends the PHY via BMCR_PDOWN bit.
+    pub fn genphy_suspend(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_suspend(phydev) })
+    }
+
+    /// Checks the link status and updates current link state.
+    pub fn genphy_read_status(&mut self) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Updates the link status.
+    pub fn genphy_update_link(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_update_link(phydev) })
+    }
+
+    /// Reads Link partner ability.
+    pub fn genphy_read_lpa(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
+    }
+
+    /// Reads PHY abilities.
+    pub fn genphy_read_abilities(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
+    }
+}
+
+/// Defines certain other features this PHY supports (like interrupts).
+pub mod flags {
+    /// PHY is internal.
+    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
+    /// PHY needs to be reset after the refclk is enabled.
+    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
+    /// Polling is used to detect PHY status changes.
+    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
+    /// Don't suspend.
+    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
+}
+
+/// An adapter for the registration of a PHY driver.
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
+            Ok(ret.into())
+        })
+    }
+
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum as u8, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+}
+
+/// Creates the kernel's `phy_driver` instance.
+///
+/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
+pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
+    Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr() as *mut i8,
+        flags: T::FLAGS,
+        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
+        soft_reset: if T::HAS_SOFT_RESET {
+            Some(Adapter::<T>::soft_reset_callback)
+        } else {
+            None
+        },
+        get_features: if T::HAS_GET_FEATURES {
+            Some(Adapter::<T>::get_features_callback)
+        } else {
+            None
+        },
+        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
+            Some(Adapter::<T>::match_phy_device_callback)
+        } else {
+            None
+        },
+        suspend: if T::HAS_SUSPEND {
+            Some(Adapter::<T>::suspend_callback)
+        } else {
+            None
+        },
+        resume: if T::HAS_RESUME {
+            Some(Adapter::<T>::resume_callback)
+        } else {
+            None
+        },
+        config_aneg: if T::HAS_CONFIG_ANEG {
+            Some(Adapter::<T>::config_aneg_callback)
+        } else {
+            None
+        },
+        read_status: if T::HAS_READ_STATUS {
+            Some(Adapter::<T>::read_status_callback)
+        } else {
+            None
+        },
+        read_mmd: if T::HAS_READ_MMD {
+            Some(Adapter::<T>::read_mmd_callback)
+        } else {
+            None
+        },
+        write_mmd: if T::HAS_WRITE_MMD {
+            Some(Adapter::<T>::write_mmd_callback)
+        } else {
+            None
+        },
+        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
+            Some(Adapter::<T>::link_change_notify_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+    })
+}
+
+/// Corresponds to functions in `struct phy_driver`.
+///
+/// This is used to register a PHY driver.
+#[vtable]
+pub trait Driver {
+    /// Defines certain other features this PHY supports.
+    const FLAGS: u32 = 0;
+    /// The friendly name of this PHY type.
+    const NAME: &'static CStr;
+    /// This driver only works for PHYs with IDs which match this field.
+    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
+
+    /// Issues a PHY software reset.
+    fn soft_reset(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Probes the hardware to determine what abilities it has.
+    fn get_features(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Returns true if this is a suitable driver for the given phydev.
+    /// If not implemented, matching is based on [`PHY_DEVICE_ID`].
+    fn match_phy_device(_dev: &mut Device) -> bool {
+        false
+    }
+
+    /// Configures the advertisement and resets auto-negotiation
+    /// if auto-negotiation is enabled.
+    fn config_aneg(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Determines the negotiated speed and duplex.
+    fn read_status(_dev: &mut Device) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Suspends the hardware, saving state if needed.
+    fn suspend(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Resumes the hardware, restoring state if needed.
+    fn resume(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD read function for reading a MMD register.
+    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD write function for writing a MMD register.
+    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Callback for notification of link change.
+    fn link_change_notify(_dev: &mut Device) {}
+}
+
+/// Registration structure for a PHY driver.
+///
+/// # Invariants
+///
+/// The `drivers` points to an array of `struct phy_driver`, which is
+/// registered to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    #[must_use]
+    pub fn register(
+        module: &'static crate::ThisModule,
+        drivers: &'static [Opaque<bindings::phy_driver>],
+    ) -> Result<Self> {
+        if drivers.len() == 0 {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: `drivers` has static lifetime and used only in the C side.
+        to_result(unsafe {
+            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
+        })?;
+        Ok(Registration {
+            drivers: Some(drivers),
+        })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        if let Some(drv) = self.drivers.take() {
+            // SAFETY: The type invariants guarantee that self.drivers is valid.
+            unsafe { bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32) };
+        }
+    }
+}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Send for Registration {}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Sync for Registration {}
+
+/// Represents the kernel's `struct mdio_device_id`.
+pub struct DeviceId {
+    /// Corresponds to `phy_id` in `struct mdio_device_id`.
+    pub id: u32,
+    mask: DeviceMask,
+}
+
+impl DeviceId {
+    /// Creates a new instance with the exact match mask.
+    pub const fn new_with_exact_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Exact,
+        }
+    }
+
+    /// Creates a new instance with the model match mask.
+    pub const fn new_with_model_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Model,
+        }
+    }
+
+    /// Creates a new instance with the vendor match mask.
+    pub const fn new_with_vendor_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Vendor,
+        }
+    }
+
+    /// Creates a new instance with a custom match mask.
+    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Custom(mask),
+        }
+    }
+
+    /// Creates a new instance from [`Driver`].
+    pub const fn new_with_driver<T: Driver>() -> Self {
+        T::PHY_DEVICE_ID
+    }
+
+    /// Get a mask as u32.
+    pub const fn mask_as_int(self) -> u32 {
+        self.mask.as_int()
+    }
+}
+
+enum DeviceMask {
+    Exact,
+    Model,
+    Vendor,
+    Custom(u32),
+}
+
+impl DeviceMask {
+    const MASK_EXACT: u32 = !0;
+    const MASK_MODEL: u32 = !0 << 4;
+    const MASK_VENDOR: u32 = !0 << 10;
+
+    const fn as_int(self) -> u32 {
+        match self {
+            DeviceMask::Exact => Self::MASK_EXACT,
+            DeviceMask::Model => Self::MASK_MODEL,
+            DeviceMask::Vendor => Self::MASK_VENDOR,
+            DeviceMask::Custom(mask) => mask,
+        }
+    }
+}
+
+/// Declares a kernel module for PHYs drivers.
+///
+/// This creates a static array of `struct phy_driver` and registers it.
+/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
+/// for module loading into the module binary file.
+///
+/// # Examples
+///
+/// ```ignore
+///
+/// use kernel::net::phy::{self, DeviceId, Driver};
+/// use kernel::prelude::*;
+///
+/// kernel::module_phy_driver! {
+///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
+///     device_table: [
+///         DeviceId::new_with_driver::<PhyAX88772A>(),
+///         DeviceId::new_with_driver::<PhyAX88772C>(),
+///         DeviceId::new_with_driver::<PhyAX88796B>()
+///     ],
+///     type: RustAsixPhy,
+///     name: "rust_asix_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust Asix PHYs driver",
+///     license: "GPL",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_phy_driver {
+    (@replace_expr $_t:tt $sub:expr) => {$sub};
+
+    (@count_devices $($x:expr),*) => {
+        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
+    };
+
+    (@device_table $name:ident, [$($dev:expr),+]) => {
+        ::kernel::macros::paste! {
+            #[no_mangle]
+            static [<__mod_mdio__ $name _device_table>]: [
+                kernel::bindings::mdio_device_id;
+                $crate::module_phy_driver!(@count_devices $($dev),+) + 1
+            ] = [
+                $(kernel::bindings::mdio_device_id {
+                    phy_id: $dev.id,
+                    phy_id_mask: $dev.mask_as_int()
+                }),+,
+                kernel::bindings::mdio_device_id {
+                    phy_id: 0,
+                    phy_id_mask: 0
+                }
+            ];
+        }
+    };
+
+    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
+        struct Module<$modname> {
+            _reg: kernel::net::phy::Registration,
+            _p: core::marker::PhantomData<$modname>,
+        }
+
+        type ModuleType = Module<$modname>;
+
+        $crate::prelude::module! {
+             type: ModuleType,
+             $($f)*
+        }
+
+        static mut DRIVERS: [
+            kernel::types::Opaque<kernel::bindings::phy_driver>;
+            $crate::module_phy_driver!(@count_devices $($driver),+)
+        ] = [
+            $(kernel::net::phy::create_phy_driver::<$driver>()),+
+        ];
+
+        impl kernel::Module for Module<$modname> {
+            fn init(module: &'static ThisModule) -> Result<Self> {
+                // SAFETY: static `DRIVERS` array is used only in the C side.
+                let mut reg = unsafe { kernel::net::phy::Registration::register(module, &DRIVERS) }?;
+
+                Ok(Module {
+                    _reg: reg,
+                    _p: core::marker::PhantomData,
+                })
+            }
+        }
+
+        $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
+    }
+}
-- 
2.34.1

[RFC PATCH v3 2/3] MAINTAINERS: add Rust PHY abstractions to the ETHERNET PHY LIBRARY

[FUJITA Tomonori]

Adds me as a maintainer for these Rust bindings too.

The files are placed at rust/kernel/ directory for now but the files
are likely to be moved to net/ directory once a new Rust build system
is implemented.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..20e0ccd4fcaa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7768,6 +7768,7 @@ F:	net/bridge/
 ETHERNET PHY LIBRARY
 M:	Andrew Lunn <andrew@lunn.ch>
 M:	Heiner Kallweit <hkallweit1@gmail.com>
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
 R:	Russell King <linux@armlinux.org.uk>
 L:	netdev@vger.kernel.org
 S:	Maintained
@@ -7797,6 +7798,7 @@ F:	include/trace/events/mdio.h
 F:	include/uapi/linux/mdio.h
 F:	include/uapi/linux/mii.h
 F:	net/core/of_net.c
+F:	rust/kernel/net/phy.rs
 
 EXEC & BINFMT API
 R:	Eric Biederman <ebiederm@xmission.com>
-- 
2.34.1

[RFC PATCH v3 3/3] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

This is the Rust implementation of drivers/net/phy/ax88796b.c.  The
features are equivalent. You can choose C or Rust versionon kernel
configuration.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/Kconfig          |   7 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 4 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 107880d13d21..e4d941f0ebe4 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -107,6 +107,13 @@ config AX88796B_PHY
 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
 	  AX88796B package.
 
+config AX88796B_RUST_PHY
+	bool "Rust reference driver"
+	depends on RUST && AX88796B_PHY
+	default n
+	help
+	  Uses the Rust version driver for Asix PHYs.
+
 config BROADCOM_PHY
 	tristate "Broadcom 54XX PHYs"
 	select BCM_NET_PHYLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c945ed9bd14b..58d7dfb095ab 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -41,7 +41,11 @@ aquantia-objs			+= aquantia_hwmon.o
 endif
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia.o
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o
-obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
+ifdef CONFIG_AX88796B_RUST_PHY
+  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
+else
+  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
+endif
 obj-$(CONFIG_BCM54140_PHY)	+= bcm54140.o
 obj-$(CONFIG_BCM63XX_PHY)	+= bcm63xx.o
 obj-$(CONFIG_BCM7XXX_PHY)	+= bcm7xxx.o
diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
new file mode 100644
index 000000000000..d11c82a9e847
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust Asix PHYs driver
+use kernel::c_str;
+use kernel::net::phy::{self, DeviceId, Driver};
+use kernel::prelude::*;
+use kernel::uapi;
+
+kernel::module_phy_driver! {
+    drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
+    device_table: [
+        DeviceId::new_with_driver::<PhyAX88772A>(),
+        DeviceId::new_with_driver::<PhyAX88772C>(),
+        DeviceId::new_with_driver::<PhyAX88796B>()
+    ],
+    type: RustAsixPhy,
+    name: "rust_asix_phy",
+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    description: "Rust Asix PHYs driver",
+    license: "GPL",
+}
+
+struct RustAsixPhy;
+
+// Performs a software PHY reset using the standard
+// BMCR_RESET bit and poll for the reset bit to be cleared.
+// Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation
+// such as used on the Individual Computers' X-Surf 100 Zorro card.
+fn asix_soft_reset(dev: &mut phy::Device) -> Result {
+    dev.write(uapi::MII_BMCR as u16, 0)?;
+    dev.genphy_soft_reset()
+}
+
+struct PhyAX88772A;
+
+#[vtable]
+impl phy::Driver for PhyAX88772A {
+    const FLAGS: u32 = phy::flags::IS_INTERNAL;
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
+
+    // AX88772A is not working properly with some old switches (NETGEAR EN 108TP):
+    // after autoneg is done and the link status is reported as active, the MII_LPA
+    // register is 0. This issue is not reproducible on AX88772C.
+    fn read_status(dev: &mut phy::Device) -> Result<u16> {
+        dev.genphy_update_link()?;
+        if !dev.get_link() {
+            return Ok(0);
+        }
+        // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
+        // linkmode so use MII_BMCR as default values.
+        let ret = dev.read(uapi::MII_BMCR as u16)?;
+
+        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
+            dev.set_speed(100);
+        } else {
+            dev.set_speed(10);
+        }
+
+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };
+        dev.set_duplex(duplex);
+
+        dev.genphy_read_lpa()?;
+
+        if dev.is_autoneg_enabled() && dev.is_autoneg_completed() {
+            dev.resolve_aneg_linkmode();
+        }
+
+        Ok(0)
+    }
+
+    fn suspend(dev: &mut phy::Device) -> Result {
+        dev.genphy_suspend()
+    }
+
+    fn resume(dev: &mut phy::Device) -> Result {
+        dev.genphy_resume()
+    }
+
+    fn soft_reset(dev: &mut phy::Device) -> Result {
+        asix_soft_reset(dev)
+    }
+
+    fn link_change_notify(dev: &mut phy::Device) {
+        // Reset PHY, otherwise MII_LPA will provide outdated information.
+        // This issue is reproducible only with some link partner PHYs.
+        if dev.state() == phy::DeviceState::NoLink {
+            let _ = dev.init_hw();
+            let _ = dev.start_aneg();
+        }
+    }
+}
+
+struct PhyAX88772C;
+
+#[vtable]
+impl Driver for PhyAX88772C {
+    const FLAGS: u32 = phy::flags::IS_INTERNAL;
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88772C");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1881);
+
+    fn suspend(dev: &mut phy::Device) -> Result {
+        dev.genphy_suspend()
+    }
+
+    fn resume(dev: &mut phy::Device) -> Result {
+        dev.genphy_resume()
+    }
+
+    fn soft_reset(dev: &mut phy::Device) -> Result {
+        asix_soft_reset(dev)
+    }
+}
+
+struct PhyAX88796B;
+
+#[vtable]
+impl Driver for PhyAX88796B {
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88796B");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_model_mask(0x003b1841);
+
+    fn soft_reset(dev: &mut phy::Device) -> Result {
+        asix_soft_reset(dev)
+    }
+}
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 301f5207f023..d92abe9064c2 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,3 +7,4 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mii.h>
-- 
2.34.1

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

[Greg KH]

On Fri, Sep 29, 2023 at 07:55:16AM +0900, FUJITA Tomonori wrote:
> +/// 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,
> +}
> +
> +/// Represents duplex mode.
> +pub enum DuplexMode {
> +    /// Full-duplex mode
> +    Half,
> +    /// Half-duplex mode
> +    Full,
> +    /// Unknown
> +    Unknown,
> +}

How are these enums going to be kept in sync with the C code?  This
doesn't seem like a good idea and will quickly cause very strange bugs
that will be impossible to debug :(

> +
> +/// Wraps the kernel's `struct phy_device`.

This is going to get very tricky as you are "inheriting" a "struct
device" from the driver core which has lifespan rules of its own.  How
are you ensuring that those are correct?

> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.

meta-comment, how is the proliferation of "Device" implementations all
across the kernel going to be able to be kept apart when searching the
code?  This feels like a tough namespace issue over time or am I just
not used to how rust does this?

> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *ptr.cast() }
> +    }

Why not rely on the driver model reference counting to ensure that you
get a reference to the object, use it, and then you can drop it?  That
way you "know" it can not go away from underneath you?

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

thanks,

greg k-h

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

[FUJITA Tomonori]

Hi,

On Fri, 29 Sep 2023 08:03:33 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Sep 29, 2023 at 07:55:16AM +0900, FUJITA Tomonori wrote:
>> +/// 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,
>> +}
>> +
>> +/// Represents duplex mode.
>> +pub enum DuplexMode {
>> +    /// Full-duplex mode
>> +    Half,
>> +    /// Half-duplex mode
>> +    Full,
>> +    /// Unknown
>> +    Unknown,
>> +}
> 
> How are these enums going to be kept in sync with the C code?  This
> doesn't seem like a good idea and will quickly cause very strange bugs
> that will be impossible to debug :(

enum DeviceState is guaranteed to be kept in sync with enum phy_state
in C. If the C code is changed without updating the Rust code,
compiling the Rust code fails. This is because a rustified enum is
generated from C's phy_state enum at compile time.

enum DuplexMode refers to C's defines in include/uapi/linux/ethtool.h:

#define DUPLEX_HALF             0x00
#define DUPLEX_FULL             0x01
#define DUPLEX_UNKNOWN          0xff

So we can't use the same trick. But I guess that these DUPLEX_* values
are unlikely be changed.

There was a good discussion about handling enum during the v1
review. IMHO, we could create guidelines on how to use enum on the C
code, which works nicely for Rust. It also needs to work nicely for C.


>> +/// Wraps the kernel's `struct phy_device`.
> 
> This is going to get very tricky as you are "inheriting" a "struct
> device" from the driver core which has lifespan rules of its own.  How
> are you ensuring that those are correct?

We know phy::Device cannot go away from underneath us. We use this
Device object only with callbacks in phy_driver. For example,
get_features() callback works like:

- The C side calls phy_driver->get_features(struct phy_device *phydev)
- The Rust side creates this Device object from the phydev pointer then
calls Rust callback for get_features() with the object.
- The Rust side destroies the object at the end of the callback.
- The C side continues.

>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
> 
> meta-comment, how is the proliferation of "Device" implementations all
> across the kernel going to be able to be kept apart when searching the
> code?  This feels like a tough namespace issue over time or am I just
> not used to how rust does this?

In Rust, you need a path to use those in the code. If you use pci's
Device and phy's Device, your code would be like:

fn some_function(pdev: pci::Device, phydev: net::phy::Device)

So a clever tool can search them properly. Note that there are ways to
avoid repeating these paths.


>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> +    /// may read or write to the `phy_device` object.
>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        unsafe { &mut *ptr.cast() }
>> +    }
> 
> Why not rely on the driver model reference counting to ensure that you
> get a reference to the object, use it, and then you can drop it?  That
> way you "know" it can not go away from underneath you?

As I wrote above, we use this Device only where we don't need
reference couting.


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

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index d11c82a9e847..f3556f320302 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -88,7 +88,7 @@ fn soft_reset(dev: &mut phy::Device) -> Result {
     fn link_change_notify(dev: &mut phy::Device) {
         // Reset PHY, otherwise MII_LPA will provide outdated information.
         // This issue is reproducible only with some link partner PHYs.
-        if dev.state() == phy::DeviceState::NoLink {
+        if dev.state() == phy::DeviceState::PHY_NOLINK {
             let _ = dev.init_hw();
             let _ = dev.start_aneg();
         }
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 335dfd7c9ddf..e9ac8986fd83 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -8,25 +8,24 @@
 use core::marker::PhantomData;
 
 /// 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;
 
 /// Represents duplex mode.
 pub enum DuplexMode {
@@ -66,19 +65,9 @@ pub fn id(&mut self) -> u32 {
 
     /// 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,
-        }
+        let phydev = self.0.get();
+        unsafe { (*phydev).state }
     }
 
     /// Returns true if the link is up.

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

[Trevor Gross]

I see Fujita beat me to responding :) but I'll share anyway

On Fri, Sep 29, 2023 at 2:09 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Sep 29, 2023 at 07:55:16AM +0900, FUJITA Tomonori wrote:
> > +/// 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,
> > +}
> > +
> > +/// Represents duplex mode.
> > +pub enum DuplexMode {
> > +    /// Full-duplex mode
> > +    Half,
> > +    /// Half-duplex mode
> > +    Full,
> > +    /// Unknown
> > +    Unknown,
> > +}
>
> How are these enums going to be kept in sync with the C code?  This
> doesn't seem like a good idea and will quickly cause very strange bugs
> that will be impossible to debug :(

Andrew brought this up earlier [1], we are still working on the best
way to handle interop between C and Rust enums. Having this enum is
verbose but it isn't terrible - mapping C's phy_state -> rust's
DeviceState happens in the `match` block you point out below. `match`
checks for exhaustiveness, so adding a variant in C would cause a
compile error in rust.

I think the bigger hole in the logic is that we don't have a way to
check for invalid values that aren't a variant in the C enum, unlikely
bug but should be checked. We are working on a way to do this but
don't have it yet.

> > +
> > +/// Wraps the kernel's `struct phy_device`.
>
> This is going to get very tricky as you are "inheriting" a "struct
> device" from the driver core which has lifespan rules of its own.  How
> are you ensuring that those are correct?
>
> > +///
> > +/// # Invariants
> > +///
> > +/// `self.0` is always in a valid state.
> > +#[repr(transparent)]
> > +pub struct Device(Opaque<bindings::phy_device>);
> > +
> > +impl Device {
> > +    /// Creates a new [`Device`] instance from a raw pointer.
>
> meta-comment, how is the proliferation of "Device" implementations all
> across the kernel going to be able to be kept apart when searching the
> code?  This feels like a tough namespace issue over time or am I just
> not used to how rust does this?

The complete type name is `net::phy::Device`, typically these don't
get names like `PhyDevice` since its full name already includes `phy`.
A module author _could_ import `Device` and use it directly:

    use net::phy::Device
    fn foo() -> Device { /* ... */ }

But when names can be ambiguous like here, there is somewhat of a code
style preference to import the module instead and prefix the type.
Which is more clear when reading and easier to search like you
mentioned

    use net::phy
    fn foo() -> phy::Device { /* ... */ }

e.g. the sample code uses `phy::Driver` instead of just `Driver`

> > +
> > +    /// 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 }
> > +    }
> > +
> > +    /// 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.
>
> thanks,
>
> greg k-h

Mentioned above but `match state` makes sure that we check for every
possible `phy_state`, so changing a name or adding/removing a variant
in C would cause compilation to fail until this gets updated. I think
this reasonably covers the failure modes.

- Trevor

[1]: starting around here
https://lore.kernel.org/rust-for-linux/8a7e010e-b99a-4331-b7c4-e1c9b7782e94@lunn.ch/

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

[Trevor Gross]

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.

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

[Miguel Ojeda]

On Fri, Sep 29, 2023 at 10:38 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> 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?

We have always wanted to generate as much as possible automatically,
but we still want to have the "final" types and docs that we would
like. In order to achieve that automatically, this could mean:

  - For the code part, getting exactly the `enum` that we want (i.e.
the one we would have written manually, e.g. without a `repr(*32)`),
including the conversion methods too (being discussed at
https://github.com/rust-lang/rust-bindgen/issues/2646).

  - For naming, we could use an algorithmic approach to convert them.
Or simply use the C names if we are really going to directly use the
generated `enum`. After all, these would be fieldless Rust enums, so
it may not be too bad.

  - For docs, ideally we would be able to provide them in the C side,
and then show them in the Rust side. There are several sub-approaches
here. An easy solution would be to show the C docs as-is (i.e.
fixed-width font), but it isn't ideal and would not allow us to use
some of the Rust-side features like intra-doc links. Another one is
automatically linking the C docs from the Rust side, but that assumes
the C side is well-documented. Another would be to allow to write the
Rust docs in the C side.

Ideally, for all this, we would augment the C side to give us more
information. For instance, by converting `#define`s into `enum`s
(thanks Andrew for being open to this -- it would already help
substantially), by providing documentation for each enumeration
constant, by specifying the underlying type (C23) and so on.

Some of these extra details could be considered improvements to the C
side too, so those would be likely fine. However, other details may
only be needed by the Rust side -- but if we allowed to provide those
in the C headers/types, then it would mean we may only need to
maintain things in a single place.

When we started the project, we didn't want to assume the C side
could/should change, so we went for the "manual" approach. It is the
most flexible and also allowed us to showcase what can be done when
cleanly writing abstractions (and their docs!). It worked fine so far,
even though it takes some work. As long as we can make mistakes
compile-time errors, it should be OK.

In summary, we should definitely automate as much as possible, but
only as long as it gets us to a place that is reasonably close to what
we can manually achieve otherwise.

Cheers,
Miguel

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

[Andrew Lunn]

> >> +/// Represents duplex mode.
> >> +pub enum DuplexMode {
> >> +    /// Full-duplex mode
> >> +    Half,
> >> +    /// Half-duplex mode
> >> +    Full,
> >> +    /// Unknown
> >> +    Unknown,
> >> +}
> > 
> > How are these enums going to be kept in sync with the C code?  This
> > doesn't seem like a good idea and will quickly cause very strange bugs
> > that will be impossible to debug :(
> 
> enum DeviceState is guaranteed to be kept in sync with enum phy_state
> in C. If the C code is changed without updating the Rust code,
> compiling the Rust code fails. This is because a rustified enum is
> generated from C's phy_state enum at compile time.
> 
> enum DuplexMode refers to C's defines in include/uapi/linux/ethtool.h:
> 
> #define DUPLEX_HALF             0x00
> #define DUPLEX_FULL             0x01
> #define DUPLEX_UNKNOWN          0xff
> 
> So we can't use the same trick. But I guess that these DUPLEX_* values
> are unlikely be changed.

I did suggest changing this into an enum. I've not tried it, but i
think it should just work.

      Andrew

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

[Miguel Ojeda]

On Fri, Sep 29, 2023 at 12:58 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +               --rustified-enum phy_state\

Note that `rustified-enum` is very dangerous, because Rust enums must
be one of the values listed (even `repr(*32)` ones), unlike C, and the
Rust compiler will optimize accordingly; and `bindgen` will use the
Rust `enum` when translating a function like:

    enum phy_state f(void);

becomes:

    fn f() -> phy_state;

But that is promising that the returned value has certain values.
Instead, here, `bindgen` should return the underlying type anyway (or
offer a different, safer mode -- I think it doesn't so far), e.g.

    fn f() -> core::ffi::c_int; // or whatever type the C compiler
picked for that `enum`.

Similarly, the same risk is with members of structs. This is, in fact,
the case in the patch here, i.e. you access `struct phy_device`, and
`bindgen` is promising that its `state` field has certain values. But,
in C, that member could have any bit pattern (and it is not UB, so we
cannot rely on it).

Or, even better, which is what I suggested in Zulip, it could provide
a mode that gives the Rust `enum`s, but with a way to construct it
fallibly from raw values.

Then, as needed, we could wrap that `enum` into another, "cleaner"
one, with better docs, idiomatic naming, possibly another `repr`,
potentially converting some "invalid" values to `Option`, and so on.
Or, if we get all the features, we could try generating that one
automatically too (please see my previous email on that).

When testing that "clean enum wraps generated one" approach, by the
way, I found another missed optimization:
https://github.com/rust-lang/rust/issues/116272 (and linked LLVM
issues). It is not a big deal, because we can do it with e.g.
`transmute`, but it would be ideal to be able to do it safely.

Cheers,
Miguel

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

[FUJITA Tomonori]

On Fri, 29 Sep 2023 17:38:56 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> 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 }
>>> +    }
>>> +
>>> +    /// 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?

Thanks a lot for the comments, Trevor and Miguel!

I leave this patch alone. Seems there is a good chance that bindgen
would support a feature to automatically generate something
useful. Once the requirement on the C side becomes clear, we can
propose changes to PHYLIB.

I'll drop RFC and send the patchset to netdev tomorrow.

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

[Andrew Lunn]

On Fri, Sep 29, 2023 at 05:11:21AM -0400, Trevor Gross wrote:
> 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)

Does Rust integrate with kerneldoc? The C enum has a header:

/**
 * enum phy_state - PHY state machine states:
 *
 * @PHY_DOWN: PHY device and driver are not ready for anything.  probe
 * should be called if and only if the PHY is in this state,
 * given that the PHY device exists.
 * - PHY driver probe function will set the state to @PHY_READY
 *
 * @PHY_READY: PHY is ready to send and receive packets, but the
 * controller is not.  By default, PHYs which do not implement
 * probe will be set to this state by phy_probe().
 * - start will set the state to UP
 *
 * @PHY_UP: The PHY and attached device are ready to do work.
 * Interrupts should be started here.
 * - timer moves to @PHY_NOLINK or @PHY_RUNNING
 *
 * @PHY_NOLINK: PHY is up, but not currently plugged in.
 * - irq or timer will set @PHY_RUNNING if link comes back
 * - phy_stop moves to @PHY_HALTED
 *
 * @PHY_RUNNING: PHY is currently up, running, and possibly sending
 * and/or receiving packets
 * - irq or timer will set @PHY_NOLINK if link goes down
 * - phy_stop moves to @PHY_HALTED
 *
 * @PHY_CABLETEST: PHY is performing a cable test. Packet reception/sending
 * is not expected to work, carrier will be indicated as down. PHY will be
 * poll once per second, or on interrupt for it current state.
 * Once complete, move to UP to restart the PHY.
 * - phy_stop aborts the running test and moves to @PHY_HALTED
 *
 * @PHY_HALTED: PHY is up, but no polling or interrupts are done.
 * - phy_start moves to @PHY_UP
 *
 * @PHY_ERROR: PHY is up, but is in an error state.
 * - phy_stop moves to @PHY_HALTED
 */

and this then appears in the kernel documentation:

https://docs.kernel.org/next/networking/kapi.html?c.phy_state#c.phy_state

Can we expect to see a section maybe referenced in
https://docs.kernel.org/next/networking/phy.html generate from the
Rust code?

	Andrew

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

[Miguel Ojeda]

On Mon, Oct 2, 2023 at 4:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Can we expect to see a section maybe referenced in
> https://docs.kernel.org/next/networking/phy.html generate from the
> Rust code?

If I understand correctly, you are asking about having the Rust docs
linked within the C docs, rather than the other way around, right?

If so, what would be the use cases you have in mind? Knowing that
there is some Rust abstraction is using that particular type/function
would be one, but perhaps you are thinking of others?

I imagine a way to implement that would be a pass that saves a list of
all used (i.e. not all included / `bindgen`'d) APIs and types from the
Rust side (i.e. everything in `bindings::`) and then kerneldoc can use
that to do whatever it needs while rendering the C docs. Or simply
doing it at the header level searching for the C header links we have
in the docs.

Cheers,
Miguel

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

[Trevor Gross]

On Mon, Oct 2, 2023 at 10:09 AM Andrew Lunn <andrew@lunn.ch> wrote:

> Does Rust integrate with kerneldoc? The C enum has a header:
>
> /**
>  * enum phy_state - PHY state machine states:
>  *
>  * @PHY_DOWN: PHY device and driver are not ready for anything.  probe
>  * should be called if and only if the PHY is in this state,
>  * given that the PHY device exists.
>  * - PHY driver probe function will set the state to @PHY_READY
>  *
>  * @PHY_READY: PHY is ready to send and receive packets, but the
>  * controller is not.  By default, PHYs which do not implement
>  * probe will be set to this state by phy_probe().
>  * - start will set the state to UP
>  *
>  * @PHY_UP: The PHY and attached device are ready to do work.
>  * Interrupts should be started here.
>  * - timer moves to @PHY_NOLINK or @PHY_RUNNING
>  *
>  * @PHY_NOLINK: PHY is up, but not currently plugged in.
>  * - irq or timer will set @PHY_RUNNING if link comes back
>  * - phy_stop moves to @PHY_HALTED
>  *
>  * @PHY_RUNNING: PHY is currently up, running, and possibly sending
>  * and/or receiving packets
>  * - irq or timer will set @PHY_NOLINK if link goes down
>  * - phy_stop moves to @PHY_HALTED
>  *
>  * @PHY_CABLETEST: PHY is performing a cable test. Packet reception/sending
>  * is not expected to work, carrier will be indicated as down. PHY will be
>  * poll once per second, or on interrupt for it current state.
>  * Once complete, move to UP to restart the PHY.
>  * - phy_stop aborts the running test and moves to @PHY_HALTED
>  *
>  * @PHY_HALTED: PHY is up, but no polling or interrupts are done.
>  * - phy_start moves to @PHY_UP
>  *
>  * @PHY_ERROR: PHY is up, but is in an error state.
>  * - phy_stop moves to @PHY_HALTED
>  */
>
> and this then appears in the kernel documentation:
>
> https://docs.kernel.org/next/networking/kapi.html?c.phy_state#c.phy_state
>
> Can we expect to see a section maybe referenced in
> https://docs.kernel.org/next/networking/phy.html generate from the
> Rust code?
>
>         Andrew

Miguel mentioned the linking but in case you were just looking for
docs example - docs are temporarily hosted at [1], and a sample enum
(not yet merged) is [2]. The docs aren't yet available at
docs.kernel.org, but I believe that should be done within the next
couple months.

[1]: https://rust-for-linux.github.io/docs/v6.6-rc2/kernel/
[2]: https://rust-for-linux.github.io/docs/kernel/fs/enum.Super.html

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

[Andrew Lunn]

On Mon, Oct 02, 2023 at 06:24:33PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 2, 2023 at 4:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Can we expect to see a section maybe referenced in
> > https://docs.kernel.org/next/networking/phy.html generate from the
> > Rust code?
> 
> If I understand correctly, you are asking about having the Rust docs
> linked within the C docs, rather than the other way around, right?

The kernel is documented using kerneldoc. It would seem odd to me to
have a second parallel set of Documentation for Rust. Just like Rust
is integrated into the kernel tree, is configured using Kconfig, built
using make at the top level, i would also expect it to integrate into
kerneldoc somehow. I see the Rust API for PHY drivers next to the C
API for PHY drivers. Its just another API in the kernel, nothing
special. I just use 'make htmldocs' at the top level and out come the
HTML documentation in Documentation/output/

But kerneldoc is not my subsystem. MAINTAINERS say:

DOCUMENTATION
M:      Jonathan Corbet <corbet@lwn.net>
L:      linux-doc@vger.kernel.org
S:      Maintained

So this discussion should really have Jonathon Corbet involved, if it
has not already been done.

    Andrew

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

[Miguel Ojeda]

On Mon, Oct 2, 2023 at 9:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> The kernel is documented using kerneldoc. It would seem odd to me to
> have a second parallel set of Documentation for Rust. Just like Rust
> is integrated into the kernel tree, is configured using Kconfig, built
> using make at the top level, i would also expect it to integrate into
> kerneldoc somehow. I see the Rust API for PHY drivers next to the C
> API for PHY drivers. Its just another API in the kernel, nothing
> special.

There are several differences that make it unlike other kernel APIs.
For instance, the Rust abstractions cannot be (in general) used by C
[*]. Many concepts simply have no equivalent in C.

Furthermore, the goal is to develop safe APIs in Rust that then Rust
modules can use. Modules are not supposed to mix the C and the Rust
API, so ideally a Rust module developer needs to only care about the
Rust APIs.

Therefore, so far, our intention with the documentation of the Rust
abstractions is that it can be used on its own as much as possible
(possibly linking to C docs or similar approaches to avoid duplication
as I was referring to in my previous message, but that is an
implementation detail).

In addition, the Rust native documentation system is quite good at
what it does. If there exists a Rust plugin for Sphinx that is good
enough, then yeah, we could use it.

[*] To be clear: one can export functions to C, of course, and the
kernel could actually have C functions that get reimplemented in Rust
(keeping the same signature) where it may make sense. Some people have
talked about this in the past, and it would be nice to see eventually.

> I just use 'make htmldocs' at the top level and out come the
> HTML documentation in Documentation/output/

Yes, this was discussed in the past and agreed to, and Carlos implemented it.

> But kerneldoc is not my subsystem. MAINTAINERS say:
>
> DOCUMENTATION
> M:      Jonathan Corbet <corbet@lwn.net>
> L:      linux-doc@vger.kernel.org
> S:      Maintained
>
> So this discussion should really have Jonathon Corbet involved, if it
> has not already been done.

Yes, of course. I asked Jonathan about this early 2021 and he has been
following (and reporting on) the Rust progress in the kernel for a
long time (which is very nice).

Cheers,
Miguel

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 02:25:40PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 2, 2023 at 9:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > The kernel is documented using kerneldoc. It would seem odd to me to
> > have a second parallel set of Documentation for Rust. Just like Rust
> > is integrated into the kernel tree, is configured using Kconfig, built
> > using make at the top level, i would also expect it to integrate into
> > kerneldoc somehow. I see the Rust API for PHY drivers next to the C
> > API for PHY drivers. Its just another API in the kernel, nothing
> > special.
> 
> There are several differences that make it unlike other kernel APIs.
> For instance, the Rust abstractions cannot be (in general) used by C
> [*]. Many concepts simply have no equivalent in C.
>
> Furthermore, the goal is to develop safe APIs in Rust that then Rust
> modules can use. Modules are not supposed to mix the C and the Rust
> API, so ideally a Rust module developer needs to only care about the
> Rust APIs.

For PHY drivers, not is not going to be true. Rust PHY drivers are
always going to be calling in C code to do most of the work, because a
lot of what PHY drivers do are define in IEEE 802.3, and we have a
huge number of helpers following 802.3. A PHY driver basically just
glues those helpers together, and deals with hardware/firmware bugs.
So to me, i expect the Rust binding documentation to be next to the C
documentation for all these helpers. That is what a PHY driver write
needs.

So having the documentation here:

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/kapi.rst#L107

would be great. I don't really care if it has a different look and
feel, its the content which is important.

Are PHY drivers unique like this? I don't think so. MAC drivers make
use of a large number of helpers for handling chunks of memory holding
Ethernet frames. So ideally the binding for MAC drivers should not be
too far away from the documentation all those helpers.

I would agree there are some drivers which don't have many helpers
from the subsystem, so this split model would work for them. gpio,
hwmon are probably good examples of that which come to mind.

> In addition, the Rust native documentation system is quite good at
> what it does. If there exists a Rust plugin for Sphinx that is good
> enough, then yeah, we could use it.

I think the first step is just getting the rustdoc generated HTML
integrated somehow. We can work on look and feel later.

	   Andrew

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> For PHY drivers, not is not going to be true. Rust PHY drivers are
> always going to be calling in C code to do most of the work, because a

Calling C code from the Rust abstractions is fine, of course. I
suspect you mean this.

Calling C code from the Rust driver is not allowed. The Rust driver
should call the Rust abstractions only.

That is why Rust drivers do not directly see the C APIs, and why we
try to make the Rust documentation self-contained as much as possible.

Hopefully that clarifies a bit my other answer.

This is documented at [1], but it looks better in the slide that we
usually put in talks, e.g. please see the second-to-last slide of [1]
in the backup slides (we should probably add this as a figure to the
kernel docs).

[1] https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings
[2] https://kangrejos.com/2023/The%20Rust%20for%20Linux%20Kernel%20Report.pdf

> So having the documentation here:
>
> https://elixir.bootlin.com/linux/latest/source/Documentation/networking/kapi.rst#L107
>
> would be great. I don't really care if it has a different look and
> feel, its the content which is important.

If you are OK with links to the Rust docs, that is doable, e.g. we
have already such a link in `Documentation/rust/index.rst` from commit
48fadf440075 ("docs: Move rustdoc output, cross-reference it").

We could perhaps also get `rustdoc` to output JSON output [3] and
generate the links automatically.

But integrating the actual features/functionality of `rustdoc` into
Sphinx or similar seems way harder, and I am not sure what we would be
gaining, apart from having to maintain a custom documentation system
for Rust code.

[3] https://github.com/rust-lang/rust/issues/76578

Cheers,
Miguel

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

[Andrew Lunn]

> Calling C code from the Rust driver is not allowed. The Rust driver
> should call the Rust abstractions only.
> 
> That is why Rust drivers do not directly see the C APIs, and why we
> try to make the Rust documentation self-contained as much as possible.

We have around 50 helpers, which PHY drivers are expected to use. I'm
not sure they are all documented, but are you going to duplicate the
documentation for them all? How is the Rust documentation kept in sync
with the C documentation?

Could bindgen read the kerneldoc and generate the Rust documentation?
That would solve the synchronisation problem?

> If you are OK with links to the Rust docs, that is doable, e.g. we
> have already such a link in `Documentation/rust/index.rst` from commit
> 48fadf440075 ("docs: Move rustdoc output, cross-reference it").

Yes, that is a good start. Lets include such a link in the next
revision of the patchset.

	 Andrew

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 6:29 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> We have around 50 helpers, which PHY drivers are expected to use. I'm
> not sure they are all documented, but are you going to duplicate the
> documentation for them all? How is the Rust documentation kept in sync
> with the C documentation?

Regarding this, please see the part about the docs in my reply at [1]
-- I think it should make more sense now.

In summary, we want to avoid duplication (and there are several
alternatives for that I mentioned there), but we do not want to break
the Rust driver isolation from the C APIs.

[1] https://lore.kernel.org/rust-for-linux/CANiq72n6XuRdhzOYPQ=RRSYVEN4LqwxScoD-hYuovPhAp_16nw@mail.gmail.com/

> Could bindgen read the kerneldoc and generate the Rust documentation?
> That would solve the synchronisation problem?

That is one possibility, but we would still need to somehow be able to
"forward" particular docs into the abstractions, because the bindings
generated by `bindgen` are not what Rust drivers call.

And even then, the abstractions may anyway need extra documentation
[*], or the abstractions may not be a direct translation to particular
C functions (even if internally they call those).

The alternative we had in mind is starting with the ability to link to
C items (functions, types...) from the Rust ones. By link I mean an
easy-to-write "intra-doc link" in `rustdoc`, e.g. it could potentially
look as simple as:

    /// Returns ...
    ///
    /// See [`my_c_function`] for details.

After all, the C drivers do not care about the Rust APIs, but the Rust
ones may need to link to the C ones for one reason or another. So it
still allows to take advantage of already written C docs yet is also
flexible to adapt to whatever the Rust layer needed.

This approach requires landing changes to Rust tooling (in this case
`rustdoc`), but the maintainers seemed open to the idea back when I
consulted them.

[*] Another aspect is that sometimes the C may not have docs to being
with, and in the Rust side we wanted to take the chance to improve
things by forcing documentation to be present for all public items
(checked by the compiler), and e.g. forcing that `# Safety` sections
are written for `unsafe` functions, and so on.

Moreover, the goal is that more documentation is written in the source
code itself, rather than in `Documentation/`. That enforces structure,
and keeps things closer to the relevant code. For instance, Rust
modules (i.e. not kernel modules, but `mod`) can be a very nice place
to put documentation, similar to what the Rust standard library does.
The fact that there are no headers in Rust also helps to keep things
in a single place too.

Of course, none of that guarantees that docs will be magically good or
useful, but since we start fresh, it is a good time to enforce these
things and give them a try. So far, we have had a nice experience with
this.

On top of that, we think providing extra value to docs  to make it as
easy as possible and to provide extra value, e.g. we have (already
upstream) the ability to compile and run the example code in the docs
as KUnit tests, automatically.

Cheers,
Miguel

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

[Wedson Almeida Filho]

On Thu, 28 Sept 2023 at 19:58, FUJITA Tomonori
<fujita.tomonori@gmail.com> 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.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/Makefile                   |   1 +
>  rust/bindings/bindings_helper.h |   3 +
>  rust/kernel/lib.rs              |   3 +
>  rust/kernel/net.rs              |   6 +
>  rust/kernel/net/phy.rs          | 706 ++++++++++++++++++++++++++++++++
>  5 files changed, 719 insertions(+)
>  create mode 100644 rust/kernel/net.rs
>  create mode 100644 rust/kernel/net/phy.rs
>
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
>        cmd_bindgen = \
>         $(BINDGEN) $< $(bindgen_target_flags) \
>                 --use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> +               --rustified-enum phy_state\
>                 --no-debug '.*' \
>                 -o $@ -- $(bindgen_c_flags_final) -DMODULE \
>                 $(bindgen_target_cflags) $(bindgen_target_extra)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
>
>  #include <kunit/test.h>
>  #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
>  #include <linux/slab.h>
>  #include <linux/refcount.h>
>  #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
>  #![no_std]
>  #![feature(allocator_api)]
>  #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
>  #![feature(dispatch_from_dyn)]
>  #![feature(new_uninit)]
>  #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
>  pub mod ioctl;
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>  pub mod prelude;
>  pub mod print;
>  mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..b49b052969e5
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_PHYLIB)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..335dfd7c9ddf
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,706 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// 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,
> +}
> +
> +/// Represents duplex mode.
> +pub enum DuplexMode {
> +    /// Full-duplex mode
> +    Half,
> +    /// Half-duplex mode
> +    Full,
> +    /// Unknown
> +    Unknown,
> +}
> +
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);

Since this device is refcounted, ideally you would implement the
`AlwaysRefCounted` trait for Device.(One that calls `mdio_device_get`
and `mdio_device_put` to increment and decrement the refcount.)

> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {

We don't want `bindings` to be used in public functions. We will hide
it from drivers eventually because we don't want them calling the C
functions directly.

> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// 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 }
> +    }
> +
> +    /// 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;

Where is this coming from?

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

Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
what the C side does.

> +    }
> +
> +    /// 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 }
> +    }
> +
> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&mut self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).autoneg_complete() == AUTONEG_COMPLETED }

Same comment here as in `get_link`.

> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&mut self, speed: u32) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).speed = speed as i32 };
> +    }

Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
become negative?

> +
> +    /// Sets duplex mode.
> +    pub fn set_duplex(&mut self, mode: DuplexMode) {
> +        let phydev = self.0.get();
> +        let v = match mode {
> +            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
> +            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
> +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
> +        };
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).duplex = v };
> +    }
> +
> +    /// Reads a given C22 PHY register.
> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe {
> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Writes a given C22 PHY register.
> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe {
> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> +        })
> +    }
> +
> +    /// Reads a paged register.
> +    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Resolves the advertisements into PHY settings.
> +    pub fn resolve_aneg_linkmode(&mut self) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
> +    }
> +
> +    /// Executes software reset the PHY via BMCR_RESET bit.
> +    pub fn genphy_soft_reset(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
> +    }
> +
> +    /// Initializes the PHY.
> +    pub fn init_hw(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // so an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::phy_init_hw(phydev) })
> +    }
> +
> +    /// Starts auto-negotiation.
> +    pub fn start_aneg(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::phy_start_aneg(phydev) })
> +    }
> +
> +    /// Resumes the PHY via BMCR_PDOWN bit.
> +    pub fn genphy_resume(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_resume(phydev) })
> +    }
> +
> +    /// Suspends the PHY via BMCR_PDOWN bit.
> +    pub fn genphy_suspend(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_suspend(phydev) })
> +    }
> +
> +    /// Checks the link status and updates current link state.
> +    pub fn genphy_read_status(&mut self) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_read_status(phydev) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Updates the link status.
> +    pub fn genphy_update_link(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_update_link(phydev) })
> +    }
> +
> +    /// Reads Link partner ability.
> +    pub fn genphy_read_lpa(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
> +    }
> +
> +    /// Reads PHY abilities.
> +    pub fn genphy_read_abilities(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
> +    }
> +}
> +
> +/// Defines certain other features this PHY supports (like interrupts).
> +pub mod flags {
> +    /// PHY is internal.
> +    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
> +    /// PHY needs to be reset after the refclk is enabled.
> +    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
> +    /// Polling is used to detect PHY status changes.
> +    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
> +    /// Don't suspend.
> +    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
> +}
> +
> +/// An adapter for the registration of a PHY driver.
> +struct Adapter<T: Driver> {
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> +    unsafe extern "C" fn soft_reset_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.

The safety requirements for `from_raw` are (copied from above): For
the duration of the lifetime 'a,
the pointer must be valid for writing and nobody else may read or
write to the `phy_device` object.

You don't explain how the second part is satisfied.

From a previous thread, it looks like C is holding the phydev mutex
while calling most of these functions. You should specify that as part
of the justification for safety. And for the functions that C doesn't
do that (IIRC, suspend and resume), you need to explain differently.

Even if existing C drivers "assume" exclusive access is ok, if it
isn't guaranteed by phy core, we can't rely on it. Part of what we're
trying to do is to ensure that if a driver has no unsafe blocks, then
it should not have undefined behaviour (modulo bugs on the C side).

> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::soft_reset(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn get_features_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::get_features(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::suspend(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::resume(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn config_aneg_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::config_aneg(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn read_status_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::read_status(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn match_phy_device_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::match_phy_device(dev) as i32
> +    }
> +
> +    unsafe extern "C" fn read_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> +            Ok(ret.into())
> +        })
> +    }
> +
> +    unsafe extern "C" fn write_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +        val: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::write_mmd(dev, devnum as u8, regnum, val)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::link_change_notify(dev);
> +    }
> +}
> +
> +/// Creates the kernel's `phy_driver` instance.
> +///
> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {

Another instance of `bindings` in a public function.

> +    Opaque::new(bindings::phy_driver {
> +        name: T::NAME.as_char_ptr() as *mut i8,
> +        flags: T::FLAGS,
> +        phy_id: T::PHY_DEVICE_ID.id,
> +        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
> +        soft_reset: if T::HAS_SOFT_RESET {
> +            Some(Adapter::<T>::soft_reset_callback)
> +        } else {
> +            None
> +        },
> +        get_features: if T::HAS_GET_FEATURES {
> +            Some(Adapter::<T>::get_features_callback)
> +        } else {
> +            None
> +        },
> +        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
> +            Some(Adapter::<T>::match_phy_device_callback)
> +        } else {
> +            None
> +        },
> +        suspend: if T::HAS_SUSPEND {
> +            Some(Adapter::<T>::suspend_callback)
> +        } else {
> +            None
> +        },
> +        resume: if T::HAS_RESUME {
> +            Some(Adapter::<T>::resume_callback)
> +        } else {
> +            None
> +        },
> +        config_aneg: if T::HAS_CONFIG_ANEG {
> +            Some(Adapter::<T>::config_aneg_callback)
> +        } else {
> +            None
> +        },
> +        read_status: if T::HAS_READ_STATUS {
> +            Some(Adapter::<T>::read_status_callback)
> +        } else {
> +            None
> +        },
> +        read_mmd: if T::HAS_READ_MMD {
> +            Some(Adapter::<T>::read_mmd_callback)
> +        } else {
> +            None
> +        },
> +        write_mmd: if T::HAS_WRITE_MMD {
> +            Some(Adapter::<T>::write_mmd_callback)
> +        } else {
> +            None
> +        },
> +        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
> +            Some(Adapter::<T>::link_change_notify_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
> +        // sets `Option<&F>` to be `None`.
> +        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> +    })
> +}
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[vtable]
> +pub trait Driver {
> +    /// Defines certain other features this PHY supports.
> +    const FLAGS: u32 = 0;

Are these the flags defined in the `flags` module? If so, please add a
reference here to the module containing the flags.

> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;

Nit: please add a blank line between these. (and FLAGS & NAME).

> +    /// This driver only works for PHYs with IDs which match this field.
> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> +
> +    /// Issues a PHY software reset.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Probes the hardware to determine what abilities it has.
> +    fn get_features(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Returns true if this is a suitable driver for the given phydev.
> +    /// If not implemented, matching is based on [`PHY_DEVICE_ID`].
> +    fn match_phy_device(_dev: &mut Device) -> bool {
> +        false
> +    }
> +
> +    /// Configures the advertisement and resets auto-negotiation
> +    /// if auto-negotiation is enabled.
> +    fn config_aneg(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Determines the negotiated speed and duplex.
> +    fn read_status(_dev: &mut Device) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Suspends the hardware, saving state if needed.
> +    fn suspend(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Resumes the hardware, restoring state if needed.
> +    fn resume(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD read function for reading a MMD register.
> +    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD write function for writing a MMD register.
> +    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Callback for notification of link change.
> +    fn link_change_notify(_dev: &mut Device) {}
> +}
> +
> +/// Registration structure for a PHY driver.
> +///
> +/// # Invariants
> +///
> +/// The `drivers` points to an array of `struct phy_driver`, which is
> +/// registered to the kernel via `phy_drivers_register`.
> +pub struct Registration {
> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,

Why is this optional?

> +}
> +
> +impl Registration {
> +    /// Registers a PHY driver.
> +    #[must_use]

This function returns a `Result`, which itself already has `must_use`.
Are you sure you need this here?

> +    pub fn register(
> +        module: &'static crate::ThisModule,
> +        drivers: &'static [Opaque<bindings::phy_driver>],

Another instance of `bindings` in a public function.

> +    ) -> Result<Self> {
> +        if drivers.len() == 0 {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
> +        to_result(unsafe {
> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)

driver.len().try_into()?

If someone manages to create a slice that doesn't fit in an i32, we should fail.

> +        })?;
> +        Ok(Registration {
> +            drivers: Some(drivers),
> +        })
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        if let Some(drv) = self.drivers.take() {
> +            // SAFETY: The type invariants guarantee that self.drivers is valid.
> +            unsafe { bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32) };
> +        }
> +    }
> +}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}
> +
> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
> +    pub id: u32,
> +    mask: DeviceMask,
> +}
> +
> +impl DeviceId {
> +    /// Creates a new instance with the exact match mask.
> +    pub const fn new_with_exact_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Exact,
> +        }
> +    }
> +
> +    /// Creates a new instance with the model match mask.
> +    pub const fn new_with_model_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Model,
> +        }
> +    }
> +
> +    /// Creates a new instance with the vendor match mask.
> +    pub const fn new_with_vendor_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Vendor,
> +        }
> +    }
> +
> +    /// Creates a new instance with a custom match mask.
> +    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Custom(mask),
> +        }
> +    }
> +
> +    /// Creates a new instance from [`Driver`].
> +    pub const fn new_with_driver<T: Driver>() -> Self {
> +        T::PHY_DEVICE_ID
> +    }
> +
> +    /// Get a mask as u32.
> +    pub const fn mask_as_int(self) -> u32 {
> +        self.mask.as_int()
> +    }
> +}
> +
> +enum DeviceMask {
> +    Exact,
> +    Model,
> +    Vendor,
> +    Custom(u32),
> +}
> +
> +impl DeviceMask {
> +    const MASK_EXACT: u32 = !0;
> +    const MASK_MODEL: u32 = !0 << 4;
> +    const MASK_VENDOR: u32 = !0 << 10;
> +
> +    const fn as_int(self) -> u32 {
> +        match self {
> +            DeviceMask::Exact => Self::MASK_EXACT,
> +            DeviceMask::Model => Self::MASK_MODEL,
> +            DeviceMask::Vendor => Self::MASK_VENDOR,
> +            DeviceMask::Custom(mask) => mask,
> +        }
> +    }
> +}
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver` and registers it.
> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +///     device_table: [
> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> +///         DeviceId::new_with_driver::<PhyAX88796B>()
> +///     ],

I don't like the fact that we need to repeat names in `driver` and
`device_table`. A single list should suffice, no?

> +///     type: RustAsixPhy,
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +/// ```

The example above doesn't compile. Please make sure your examples do.
(For any functions you need to implement but are not used in the
example, you can always use `todo!()`.)

> +#[macro_export]
> +macro_rules! module_phy_driver {
> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> +    (@count_devices $($x:expr),*) => {
> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> +    };
> +
> +    (@device_table $name:ident, [$($dev:expr),+]) => {
> +        ::kernel::macros::paste! {
> +            #[no_mangle]
> +            static [<__mod_mdio__ $name _device_table>]: [
> +                kernel::bindings::mdio_device_id;
> +                $crate::module_phy_driver!(@count_devices $($dev),+) + 1
> +            ] = [
> +                $(kernel::bindings::mdio_device_id {
> +                    phy_id: $dev.id,
> +                    phy_id_mask: $dev.mask_as_int()
> +                }),+,
> +                kernel::bindings::mdio_device_id {
> +                    phy_id: 0,
> +                    phy_id_mask: 0
> +                }
> +            ];
> +        }
> +    };
> +
> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
> +        struct Module<$modname> {
> +            _reg: kernel::net::phy::Registration,
> +            _p: core::marker::PhantomData<$modname>,
> +        }

Why can't this struct be implemented in core::net::phy and just
referenced from this macro?

> +
> +        type ModuleType = Module<$modname>;
> +
> +        $crate::prelude::module! {
> +             type: ModuleType,
> +             $($f)*
> +        }
> +
> +        static mut DRIVERS: [
> +            kernel::types::Opaque<kernel::bindings::phy_driver>;
> +            $crate::module_phy_driver!(@count_devices $($driver),+)
> +        ] = [
> +            $(kernel::net::phy::create_phy_driver::<$driver>()),+
> +        ];
> +
> +        impl kernel::Module for Module<$modname> {
> +            fn init(module: &'static ThisModule) -> Result<Self> {
> +                // SAFETY: static `DRIVERS` array is used only in the C side.
> +                let mut reg = unsafe { kernel::net::phy::Registration::register(module, &DRIVERS) }?;
> +
> +                Ok(Module {
> +                    _reg: reg,
> +                    _p: core::marker::PhantomData,
> +                })
> +            }
> +        }
> +
> +        $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
> +    }
> +}
> --
> 2.34.1
>

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

[Andrew Lunn]

> > +/// Wraps the kernel's `struct phy_device`.
> > +///
> > +/// # Invariants
> > +///
> > +/// `self.0` is always in a valid state.
> > +#[repr(transparent)]
> > +pub struct Device(Opaque<bindings::phy_device>);
> 
> Since this device is refcounted, ideally you would implement the
> `AlwaysRefCounted` trait for Device.(One that calls `mdio_device_get`
> and `mdio_device_put` to increment and decrement the refcount.)

Dumb question. Why? No other PHY driver does this?

phy_attach_direct() does a get_device(d), and only after that is the
probe function called. phy_detach() does a put on it.

Maybe the device should not be refcounted?

> > +        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 }
> 
> Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> what the C side does.

Not really.

        if (!phydev->link)
                return 0;

And link is a bit field. 

	unsigned link:1;

So it can only be 0 or 1.

(*phydev).link() == LINK_IS_UP seems more readable than
(*phydev).link() != 0

I also don't see how it can be more future proof. The link to the peer
is either up or not. There are only two states, Schrodinger's cat is
not involved here.

> > +    }
> > +
> > +    /// Sets the speed of the PHY.
> > +    pub fn set_speed(&mut self, speed: u32) {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        unsafe { (*phydev).speed = speed as i32 };
> > +    }
> 
> Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> become negative?

How clever is the compiler here? The speed passed to this method is
pretty much always a constant value. In C you could probably do a

BUILD_BUG_ON(speed > INT_MAX && speed != SPEED_UNKNOWN);

and have the build fail if it was passed too big a value.

> The safety requirements for `from_raw` are (copied from above): For
> the duration of the lifetime 'a,
> the pointer must be valid for writing and nobody else may read or
> write to the `phy_device` object.
> 
> You don't explain how the second part is satisfied.
> 
> >From a previous thread, it looks like C is holding the phydev mutex
> while calling most of these functions. You should specify that as part
> of the justification for safety. And for the functions that C doesn't
> do that (IIRC, suspend and resume), you need to explain differently.

Do you ever consider skipping documenting the safety assumptions and
just proving the safety assumption?

It probably costs very little to actually test if the mutex is locked.
And if it is not locked, that is probably a bug we need to find and
fix.

> > +/// kernel::module_phy_driver! {
> > +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> > +///     device_table: [
> > +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> > +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> > +///         DeviceId::new_with_driver::<PhyAX88796B>()
> > +///     ],
> 
> I don't like the fact that we need to repeat names in `driver` and
> `device_table`. A single list should suffice, no?

All the C drivers do this. It was discussed before, there is not
always a simple 1:1 mapping from struct phy_driver to struct
mdio_device_id because of the way masks are used. Rust could be more
restrictive, and that might help avoid some bugs where developers
forget to put an entry in the struct mdio_device_id table.

	Andrew

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

[Wedson Almeida Filho]

On Tue, 10 Oct 2023 at 17:28, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > +/// Wraps the kernel's `struct phy_device`.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// `self.0` is always in a valid state.
> > > +#[repr(transparent)]
> > > +pub struct Device(Opaque<bindings::phy_device>);
> >
> > Since this device is refcounted, ideally you would implement the
> > `AlwaysRefCounted` trait for Device.(One that calls `mdio_device_get`
> > and `mdio_device_put` to increment and decrement the refcount.)
>
> Dumb question. Why? No other PHY driver does this?

If you implement that trait, you get to use `ARef<Device>` as a
refcounted `Device`, which is guaranteed to not produce dangling
pointers in unsafe-free code. This is what all Rust-representations of
C ref-counted objects are supposed to use.

But if no driver needs to increment or decrement the refcount, we can
probably skip this until such a need arises (if ever).

> phy_attach_direct() does a get_device(d), and only after that is the
> probe function called. phy_detach() does a put on it.
>
> Maybe the device should not be refcounted?

I guess it needs to be refcounted because it is a kobject that exists
in sysfs, so likely unavoidable.

> > > +        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 }
> >
> > Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> > what the C side does.
>
> Not really.
>
>         if (!phydev->link)
>                 return 0;
>
> And link is a bit field.
>
>         unsigned link:1;
>
> So it can only be 0 or 1.

It happens to be a bit field with a single bit now. If it gets
converted to say a bool, or to 2 bits, the existing C code continues
to work, but Rust as written by Tomo wouldn't. That's what I mean by
future-proof.

The C code absolutely does a != 0 check: https://godbolt.org/z/3rjjcjEK4

> (*phydev).link() == LINK_IS_UP seems more readable than
> (*phydev).link() != 0

Sure, how about you define and use LINK_IS_UP on the C side? Then
we'll use your C definition and mimic the behaviour on the Rust side.

> I also don't see how it can be more future proof. The link to the peer
> is either up or not. There are only two states, Schrodinger's cat is
> not involved here.

Yes, there are only two states, but if you ever switch to say an 8-bit
boolean to represent the states, then you have 255 values but only two
states and we'll have to do the proper mapping. As I said above, if
leave the code as is, C will do will thing and Rust will do something
else. I want to avoid this.

> > > +    }
> > > +
> > > +    /// Sets the speed of the PHY.
> > > +    pub fn set_speed(&mut self, speed: u32) {
> > > +        let phydev = self.0.get();
> > > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > > +        unsafe { (*phydev).speed = speed as i32 };
> > > +    }
> >
> > Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> > become negative?
>
> How clever is the compiler here? The speed passed to this method is
> pretty much always a constant value. In C you could probably do a
>
> BUILD_BUG_ON(speed > INT_MAX && speed != SPEED_UNKNOWN);
>
> and have the build fail if it was passed too big a value.

Yes, we should be able to use build_assert for that. I like the idea.

> > The safety requirements for `from_raw` are (copied from above): For
> > the duration of the lifetime 'a,
> > the pointer must be valid for writing and nobody else may read or
> > write to the `phy_device` object.
> >
> > You don't explain how the second part is satisfied.
> >
> > >From a previous thread, it looks like C is holding the phydev mutex
> > while calling most of these functions. You should specify that as part
> > of the justification for safety. And for the functions that C doesn't
> > do that (IIRC, suspend and resume), you need to explain differently.
>
> Do you ever consider skipping documenting the safety assumptions and
> just proving the safety assumption?

Yes, I'd very much like to see that, but we don't have good-enough
tooling at the moment to that at compile-time only.

>
> It probably costs very little to actually test if the mutex is locked.
> And if it is not locked, that is probably a bug we need to find and
> fix.

Oh, you mean proving at runtime?

Torvalds has told me in the past that incrementing a cpu-local
word-sized counter was too expensive. I'd rather avoid runtime costs
unless really necessary.

> > > +/// kernel::module_phy_driver! {
> > > +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> > > +///     device_table: [
> > > +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> > > +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> > > +///         DeviceId::new_with_driver::<PhyAX88796B>()
> > > +///     ],
> >
> > I don't like the fact that we need to repeat names in `driver` and
> > `device_table`. A single list should suffice, no?
>
> All the C drivers do this. It was discussed before, there is not
> always a simple 1:1 mapping from struct phy_driver to struct
> mdio_device_id because of the way masks are used. Rust could be more
> restrictive, and that might help avoid some bugs where developers
> forget to put an entry in the struct mdio_device_id table.

If each driver supports more than one ID, what we should do is allow
them (the drivers) to declare a list of ids, not just one.

This is what we do for other drivers.

>
>         Andrew

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

[Trevor Gross]

Linking in some context, discussion is somewhat fragmented across this
patch series. This is an old set anyway, the latest is at
https://lore.kernel.org/rust-for-linux/ZSQXqX2%2Flhf5ICZP@gpd/T/#t -
but I don't think anything reviewed has changed.

On Tue, Oct 10, 2023 at 3:19 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&mut self) -> bool {
> > +        const LINK_IS_UP: u32 = 1;
>
> Where is this coming from?
>
> > +        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 }
>
> Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> what the C side does.

I suggested it as more clear hint about the magic value, logic could
potentially be flipped though

https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/

> > +    /// Sets the speed of the PHY.
> > +    pub fn set_speed(&mut self, speed: u32) {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        unsafe { (*phydev).speed = speed as i32 };
> > +    }
>
> Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> become negative?

https://lore.kernel.org/rust-for-linux/96800001-5d19-4b48-b43e-0cfbeccb48c1@lunn.ch/

I agree that saturating probably isn't a bad idea here, but the C side
does validate it

> > +/// ```ignore
> > +///
> > +/// use kernel::net::phy::{self, DeviceId, Driver};
> > +/// use kernel::prelude::*;
> > +///
> > +/// kernel::module_phy_driver! {
> > +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> > +///     device_table: [
> > +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> > +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> > +///         DeviceId::new_with_driver::<PhyAX88796B>()
> > +///     ],
>
> I don't like the fact that we need to repeat names in `driver` and
> `device_table`. A single list should suffice, no?

I agree, but there needs to be a way to use a driver more than once
with different IDs. See
https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
and the followup, maybe you have a better suggestion

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

[Miguel Ojeda]

On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> I agree, but there needs to be a way to use a driver more than once
> with different IDs. See
> https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
> and the followup, maybe you have a better suggestion

Does it make sense to generate one of those based on the other, unless
both are provided, for instance?

Otherwise, we could perhaps have a common macro to handle the
"advanced" case, and a simpler one that calls into the first for the
simple cases.

Cheers,
Miguel

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 00:53:59 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
>>
>> I agree, but there needs to be a way to use a driver more than once
>> with different IDs. See
>> https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
>> and the followup, maybe you have a better suggestion
> 
> Does it make sense to generate one of those based on the other, unless
> both are provided, for instance?
> 
> Otherwise, we could perhaps have a common macro to handle the
> "advanced" case, and a simpler one that calls into the first for the
> simple cases.

As discussed before, there are three cases:

1. one phy driver has one id in device_table.
2. one phy driver has multiple ids in device_table.
3. multiple phy drivers has one id in device_table.

This macro can handle all the cases.

It might be nice to have a macro that can handle only one case but is
more handy. But I don't think we need to discuss this for merging.

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

[Trevor Gross]

On Tue, Oct 10, 2023 at 6:54 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
> >
> > I agree, but there needs to be a way to use a driver more than once
> > with different IDs. See
> > https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
> > and the followup, maybe you have a better suggestion
>
> Does it make sense to generate one of those based on the other, unless
> both are provided, for instance?

That is what I was aiming for with my example `{ phy: PhyAX88772A, id:
some_other_device_id }`, it seems to make sense that we default
somehow.

Or, did you mean something specific like storing >1 device ID in the
`Driver` trait? Changing from:

    trait Driver{
        const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
    }

To:

    trait Driver{
        const PHY_DEVICE_IDS: &'static [DeviceId] =
            &[DeviceId::new_with_custom_mask(0, 0)];
    }

Maybe that would be better because the information is all in one place
and the macro doesn't need to gain any more syntax

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

[Andrew Lunn]

On Tue, Oct 10, 2023 at 06:00:15PM -0300, Wedson Almeida Filho wrote:
> On Tue, 10 Oct 2023 at 17:28, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > > +/// Wraps the kernel's `struct phy_device`.
> > > > +///
> > > > +/// # Invariants
> > > > +///
> > > > +/// `self.0` is always in a valid state.
> > > > +#[repr(transparent)]
> > > > +pub struct Device(Opaque<bindings::phy_device>);
> > >
> > > Since this device is refcounted, ideally you would implement the
> > > `AlwaysRefCounted` trait for Device.(One that calls `mdio_device_get`
> > > and `mdio_device_put` to increment and decrement the refcount.)
> >
> > Dumb question. Why? No other PHY driver does this?
> 
> If you implement that trait, you get to use `ARef<Device>` as a
> refcounted `Device`, which is guaranteed to not produce dangling
> pointers in unsafe-free code. This is what all Rust-representations of
> C ref-counted objects are supposed to use.
> 
> But if no driver needs to increment or decrement the refcount, we can
> probably skip this until such a need arises (if ever).
> 
> > phy_attach_direct() does a get_device(d), and only after that is the
> > probe function called. phy_detach() does a put on it.
> >
> > Maybe the device should not be refcounted?
> 
> I guess it needs to be refcounted because it is a kobject that exists
> in sysfs, so likely unavoidable.

Is Rust putting a kobject in sysfs? I don't think so. So it is not
really Rust problem. The core should be doing all the device
references. And we don't want a driver papering over bugs in the core,
we want to fix the core.

> > > > +        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 }
> > >
> > > Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> > > what the C side does.
> >
> > Not really.
> >
> >         if (!phydev->link)
> >                 return 0;
> >
> > And link is a bit field.
> >
> >         unsigned link:1;
> >
> > So it can only be 0 or 1.
> 
> It happens to be a bit field with a single bit now. If it gets
> converted to say a bool, or to 2 bits, the existing C code continues
> to work, but Rust as written by Tomo wouldn't. That's what I mean by
> future-proof.
> 
> The C code absolutely does a != 0 check: https://godbolt.org/z/3rjjcjEK4

It would be better to say the generated assembler does a != 0.
On one particular instruction set. AVR seems to be a compare with
1. But that does seem to be the exception....

> 
> > (*phydev).link() == LINK_IS_UP seems more readable than
> > (*phydev).link() != 0
> 
> Sure, how about you define and use LINK_IS_UP on the C side? Then
> we'll use your C definition and mimic the behaviour on the Rust side.

Patches welcome.

> > I also don't see how it can be more future proof. The link to the peer
> > is either up or not. There are only two states, Schrodinger's cat is
> > not involved here.
> 
> Yes, there are only two states, but if you ever switch to say an 8-bit
> boolean to represent the states, then you have 255 values but only two
> states and we'll have to do the proper mapping.

Can you do a build time assert that link is a 1 bit bitfield? Fail to
compile if it ever changes. 

> > > > +    /// Sets the speed of the PHY.
> > > > +    pub fn set_speed(&mut self, speed: u32) {
> > > > +        let phydev = self.0.get();
> > > > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > > > +        unsafe { (*phydev).speed = speed as i32 };
> > > > +    }
> > >
> > > Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> > > become negative?
> >
> > How clever is the compiler here? The speed passed to this method is
> > pretty much always a constant value. In C you could probably do a
> >
> > BUILD_BUG_ON(speed > INT_MAX && speed != SPEED_UNKNOWN);
> >
> > and have the build fail if it was passed too big a value.
> 
> Yes, we should be able to use build_assert for that. I like the idea.

Yes, it clearly is a bug. Speed here is in Mbps. So that would be
2147483647Mbps. Current copper PHYs stop at around 40,000Mbps, and
physics is going to get in the way going to much faster, you need to
use fibre, and that is not phylibs job.


> > > The safety requirements for `from_raw` are (copied from above): For
> > > the duration of the lifetime 'a,
> > > the pointer must be valid for writing and nobody else may read or
> > > write to the `phy_device` object.
> > >
> > > You don't explain how the second part is satisfied.
> > >
> > > >From a previous thread, it looks like C is holding the phydev mutex
> > > while calling most of these functions. You should specify that as part
> > > of the justification for safety. And for the functions that C doesn't
> > > do that (IIRC, suspend and resume), you need to explain differently.
> >
> > Do you ever consider skipping documenting the safety assumptions and
> > just proving the safety assumption?
> 
> Yes, I'd very much like to see that, but we don't have good-enough
> tooling at the moment to that at compile-time only.

I don't see how tooling can determine that at compile time. Its in C
code for a start. Maybe some sort of cross C and Rust LTO if it had
access to the intermediary representation?

       Andrew

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

[Wedson Almeida Filho]

On Tue, 10 Oct 2023 at 20:34, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Oct 10, 2023 at 06:00:15PM -0300, Wedson Almeida Filho wrote:
> > On Tue, 10 Oct 2023 at 17:28, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > > +/// Wraps the kernel's `struct phy_device`.
> > > > > +///
> > > > > +/// # Invariants
> > > > > +///
> > > > > +/// `self.0` is always in a valid state.
> > > > > +#[repr(transparent)]
> > > > > +pub struct Device(Opaque<bindings::phy_device>);
> > > >
> > > > Since this device is refcounted, ideally you would implement the
> > > > `AlwaysRefCounted` trait for Device.(One that calls `mdio_device_get`
> > > > and `mdio_device_put` to increment and decrement the refcount.)
> > >
> > > Dumb question. Why? No other PHY driver does this?
> >
> > If you implement that trait, you get to use `ARef<Device>` as a
> > refcounted `Device`, which is guaranteed to not produce dangling
> > pointers in unsafe-free code. This is what all Rust-representations of
> > C ref-counted objects are supposed to use.
> >
> > But if no driver needs to increment or decrement the refcount, we can
> > probably skip this until such a need arises (if ever).
> >
> > > phy_attach_direct() does a get_device(d), and only after that is the
> > > probe function called. phy_detach() does a put on it.
> > >
> > > Maybe the device should not be refcounted?
> >
> > I guess it needs to be refcounted because it is a kobject that exists
> > in sysfs, so likely unavoidable.
>
> Is Rust putting a kobject in sysfs? I don't think so. So it is not
> really Rust problem. The core should be doing all the device
> references. And we don't want a driver papering over bugs in the core,
> we want to fix the core.

Rust isn't itself putting kobjects in sysfs. And I'm not suggesting
papering over bugs anywhere.

If a driver wants to hold on to a pointer to a `Device` beyond the
scope of the function (e.g., to defer some work to a work queue
thread), then it needs to increment the refcount to ensure that the
device remains alive -- nothing new here. I'm saying that Rust
implements unified abstractions for all C ref-counted types; for a
type to participate, it just needs to specify the incref and decref
functions.

It seems like you're arguing that PHY devices are so simple that they
should never need to do anything that involves
incrementing/decrementing the refcount. If that's the case, then fine,
we don't need to implement the trait now. We can do it later if the
need ever arises.

> > > > > +        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 }
> > > >
> > > > Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> > > > what the C side does.
> > >
> > > Not really.
> > >
> > >         if (!phydev->link)
> > >                 return 0;
> > >
> > > And link is a bit field.
> > >
> > >         unsigned link:1;
> > >
> > > So it can only be 0 or 1.
> >
> > It happens to be a bit field with a single bit now. If it gets
> > converted to say a bool, or to 2 bits, the existing C code continues
> > to work, but Rust as written by Tomo wouldn't. That's what I mean by
> > future-proof.
> >
> > The C code absolutely does a != 0 check: https://godbolt.org/z/3rjjcjEK4
>
> It would be better to say the generated assembler does a != 0.
> On one particular instruction set. AVR seems to be a compare with
> 1. But that does seem to be the exception....

Let's look at the C spec:
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Last paragraph of page 79:
The result of the logical negation operator ! is 0 if the value of its
operand compares unequal to 0, 1 if the value of its operand compares
equal to 0. The result has type int. The expression !E is equivalent
to (0==E).

The compiler may generate whatever code it wants as long as it
complies with the semantics of the language. And the semantics calls
for a ==0 or != 0 comparison.

> >
> > > (*phydev).link() == LINK_IS_UP seems more readable than
> > > (*phydev).link() != 0
> >
> > Sure, how about you define and use LINK_IS_UP on the C side? Then
> > we'll use your C definition and mimic the behaviour on the Rust side.
>
> Patches welcome.

Tomo?

> > > I also don't see how it can be more future proof. The link to the peer
> > > is either up or not. There are only two states, Schrodinger's cat is
> > > not involved here.
> >
> > Yes, there are only two states, but if you ever switch to say an 8-bit
> > boolean to represent the states, then you have 255 values but only two
> > states and we'll have to do the proper mapping.
>
> Can you do a build time assert that link is a 1 bit bitfield? Fail to
> compile if it ever changes.

I don't know off the top of my head of a way to do it, but yeah, if we
can find one, that would be a better way to detect potentially
breaking changes.

> > > > > +    /// Sets the speed of the PHY.
> > > > > +    pub fn set_speed(&mut self, speed: u32) {
> > > > > +        let phydev = self.0.get();
> > > > > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > > > > +        unsafe { (*phydev).speed = speed as i32 };
> > > > > +    }
> > > >
> > > > Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> > > > become negative?
> > >
> > > How clever is the compiler here? The speed passed to this method is
> > > pretty much always a constant value. In C you could probably do a
> > >
> > > BUILD_BUG_ON(speed > INT_MAX && speed != SPEED_UNKNOWN);
> > >
> > > and have the build fail if it was passed too big a value.
> >
> > Yes, we should be able to use build_assert for that. I like the idea.
>
> Yes, it clearly is a bug. Speed here is in Mbps. So that would be
> 2147483647Mbps. Current copper PHYs stop at around 40,000Mbps, and
> physics is going to get in the way going to much faster, you need to
> use fibre, and that is not phylibs job.

Ack.

> > > > The safety requirements for `from_raw` are (copied from above): For
> > > > the duration of the lifetime 'a,
> > > > the pointer must be valid for writing and nobody else may read or
> > > > write to the `phy_device` object.
> > > >
> > > > You don't explain how the second part is satisfied.
> > > >
> > > > >From a previous thread, it looks like C is holding the phydev mutex
> > > > while calling most of these functions. You should specify that as part
> > > > of the justification for safety. And for the functions that C doesn't
> > > > do that (IIRC, suspend and resume), you need to explain differently.
> > >
> > > Do you ever consider skipping documenting the safety assumptions and
> > > just proving the safety assumption?
> >
> > Yes, I'd very much like to see that, but we don't have good-enough
> > tooling at the moment to that at compile-time only.
>
> I don't see how tooling can determine that at compile time. Its in C
> code for a start. Maybe some sort of cross C and Rust LTO if it had
> access to the intermediary representation?

One way to do it at compile time is to attach pre and post conditions
to each function (Hoare style). Then pre-conditions are checked
locally at call sites and the tool fails (or requires manual
intervention) if it can't prove it; pre-conditions in function
definitions are then always considered true and can be used to infer
additional properties.

The language used to express the pre and post conditions would likely
need to be more expressive than C and Rust.

Anyway, we don't have that now and we won't have it anytime soon. So
we need to argue in prose about the validity of our safety
requirements. Tomo's argument is lacking here; AFAICT, it should be
easy to fix, except for suspend and resume (which I still don't know
what the claim is).

Cheers,
-Wedson

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

[FUJITA Tomonori]

On Tue, 10 Oct 2023 22:56:52 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

>> > > (*phydev).link() == LINK_IS_UP seems more readable than
>> > > (*phydev).link() != 0
>> >
>> > Sure, how about you define and use LINK_IS_UP on the C side? Then
>> > we'll use your C definition and mimic the behaviour on the Rust side.
>>
>> Patches welcome.
> 
> Tomo?

Sure, I'll put this on my TODO list.


>> > > > The safety requirements for `from_raw` are (copied from above): For
>> > > > the duration of the lifetime 'a,
>> > > > the pointer must be valid for writing and nobody else may read or
>> > > > write to the `phy_device` object.
>> > > >
>> > > > You don't explain how the second part is satisfied.
>> > > >
>> > > > >From a previous thread, it looks like C is holding the phydev mutex
>> > > > while calling most of these functions. You should specify that as part
>> > > > of the justification for safety. And for the functions that C doesn't
>> > > > do that (IIRC, suspend and resume), you need to explain differently.
>> > >
>> > > Do you ever consider skipping documenting the safety assumptions and
>> > > just proving the safety assumption?
>> >
>> > Yes, I'd very much like to see that, but we don't have good-enough
>> > tooling at the moment to that at compile-time only.
>>
>> I don't see how tooling can determine that at compile time. Its in C
>> code for a start. Maybe some sort of cross C and Rust LTO if it had
>> access to the intermediary representation?
> 
> One way to do it at compile time is to attach pre and post conditions
> to each function (Hoare style). Then pre-conditions are checked
> locally at call sites and the tool fails (or requires manual
> intervention) if it can't prove it; pre-conditions in function
> definitions are then always considered true and can be used to infer
> additional properties.
> 
> The language used to express the pre and post conditions would likely
> need to be more expressive than C and Rust.
> 
> Anyway, we don't have that now and we won't have it anytime soon. So
> we need to argue in prose about the validity of our safety
> requirements. Tomo's argument is lacking here; AFAICT, it should be
> easy to fix, except for suspend and resume (which I still don't know
> what the claim is).

That comment was updated by review comments. Can you check the latest patchset?

https://lore.kernel.org/netdev/20231009013912.4048593-1-fujita.tomonori@gmail.com/T/

If you think that it's not still enough, why not sending a patch after
it's merged? It should be easy to fix.

About suspend and resume, what should we do? An exclusive access to a
device isn't guaranteed by locking. Instead a PHY driver trusts PHYLIB.

Adding more comments? Or make the functions unsafe?

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

[FUJITA Tomonori]

On Tue, 10 Oct 2023 16:19:02 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> +    /// may read or write to the `phy_device` object.
>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> 
> We don't want `bindings` to be used in public functions. We will hide
> it from drivers eventually because we don't want them calling the C
> functions directly.

Sorry, I should have removed `pub` here. PHY drivers don't use this
function.


>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
> 
> Another instance of `bindings` in a public function.

Reexporting works?

type DriverType = bindings::phy_driver;


>> +/// Corresponds to functions in `struct phy_driver`.
>> +///
>> +/// This is used to register a PHY driver.
>> +#[vtable]
>> +pub trait Driver {
>> +    /// Defines certain other features this PHY supports.
>> +    const FLAGS: u32 = 0;
> 
> Are these the flags defined in the `flags` module? If so, please add a
> reference here to the module containing the flags.

Ok

>> +    /// The friendly name of this PHY type.
>> +    const NAME: &'static CStr;
> 
> Nit: please add a blank line between these. (and FLAGS & NAME).

Ok

>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
>> +pub struct Registration {
>> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> 
> Why is this optional?

Oops, leftover of old code.


>> +}
>> +
>> +impl Registration {
>> +    /// Registers a PHY driver.
>> +    #[must_use]
> 
> This function returns a `Result`, which itself already has `must_use`.
> Are you sure you need this here?

Ah, we don't need `must_use` here.


>> +    pub fn register(
>> +        module: &'static crate::ThisModule,
>> +        drivers: &'static [Opaque<bindings::phy_driver>],
> 
> Another instance of `bindings` in a public function.
> 
>> +    ) -> Result<Self> {
>> +        if drivers.len() == 0 {
>> +            return Err(code::EINVAL);
>> +        }
>> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
>> +        to_result(unsafe {
>> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
> 
> driver.len().try_into()?
> 
> If someone manages to create a slice that doesn't fit in an i32, we should fail.

Ok

>> +///     type: RustAsixPhy,
>> +///     name: "rust_asix_phy",
>> +///     author: "Rust for Linux Contributors",
>> +///     description: "Rust Asix PHYs driver",
>> +///     license: "GPL",
>> +/// }
>> +/// ```
> 
> The example above doesn't compile. Please make sure your examples do.
> (For any functions you need to implement but are not used in the
> example, you can always use `todo!()`.)

Ok

>> +#[macro_export]
>> +macro_rules! module_phy_driver {
>> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
>> +
>> +    (@count_devices $($x:expr),*) => {
>> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> +    };
>> +
>> +    (@device_table $name:ident, [$($dev:expr),+]) => {
>> +        ::kernel::macros::paste! {
>> +            #[no_mangle]
>> +            static [<__mod_mdio__ $name _device_table>]: [
>> +                kernel::bindings::mdio_device_id;
>> +                $crate::module_phy_driver!(@count_devices $($dev),+) + 1
>> +            ] = [
>> +                $(kernel::bindings::mdio_device_id {
>> +                    phy_id: $dev.id,
>> +                    phy_id_mask: $dev.mask_as_int()
>> +                }),+,
>> +                kernel::bindings::mdio_device_id {
>> +                    phy_id: 0,
>> +                    phy_id_mask: 0
>> +                }
>> +            ];
>> +        }
>> +    };
>> +
>> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
>> +        struct Module<$modname> {
>> +            _reg: kernel::net::phy::Registration,
>> +            _p: core::marker::PhantomData<$modname>,
>> +        }
> 
> Why can't this struct be implemented in core::net::phy and just
> referenced from this macro?

Seems that driver! macro's type works only with type inside the
current crate.

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

[Wedson Almeida Filho]

On Wed, 11 Oct 2023 at 03:56, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> On Tue, 10 Oct 2023 16:19:02 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> >> +impl Device {
> >> +    /// Creates a new [`Device`] instance from a raw pointer.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >> +    /// may read or write to the `phy_device` object.
> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >
> > We don't want `bindings` to be used in public functions. We will hide
> > it from drivers eventually because we don't want them calling the C
> > functions directly.
>
> Sorry, I should have removed `pub` here. PHY drivers don't use this
> function.
>
>
> >> +/// Creates the kernel's `phy_driver` instance.
> >> +///
> >> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> >> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
> >
> > Another instance of `bindings` in a public function.
>
> Reexporting works?
>
> type DriverType = bindings::phy_driver;

No, that's a type alias. If you really want to expose it, you're
better off creating a [transparent] new type that wraps
bindings::phy_driver. This way `bindings` is never visible to drivers.

>
> >> +/// Corresponds to functions in `struct phy_driver`.
> >> +///
> >> +/// This is used to register a PHY driver.
> >> +#[vtable]
> >> +pub trait Driver {
> >> +    /// Defines certain other features this PHY supports.
> >> +    const FLAGS: u32 = 0;
> >
> > Are these the flags defined in the `flags` module? If so, please add a
> > reference here to the module containing the flags.
>
> Ok
>
> >> +    /// The friendly name of this PHY type.
> >> +    const NAME: &'static CStr;
> >
> > Nit: please add a blank line between these. (and FLAGS & NAME).
>
> Ok
>
> >> +/// Registration structure for a PHY driver.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// The `drivers` points to an array of `struct phy_driver`, which is
> >> +/// registered to the kernel via `phy_drivers_register`.
> >> +pub struct Registration {
> >> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> >
> > Why is this optional?
>
> Oops, leftover of old code.
>
>
> >> +}
> >> +
> >> +impl Registration {
> >> +    /// Registers a PHY driver.
> >> +    #[must_use]
> >
> > This function returns a `Result`, which itself already has `must_use`.
> > Are you sure you need this here?
>
> Ah, we don't need `must_use` here.
>
>
> >> +    pub fn register(
> >> +        module: &'static crate::ThisModule,
> >> +        drivers: &'static [Opaque<bindings::phy_driver>],
> >
> > Another instance of `bindings` in a public function.
> >
> >> +    ) -> Result<Self> {
> >> +        if drivers.len() == 0 {
> >> +            return Err(code::EINVAL);
> >> +        }
> >> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
> >> +        to_result(unsafe {
> >> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
> >
> > driver.len().try_into()?
> >
> > If someone manages to create a slice that doesn't fit in an i32, we should fail.
>
> Ok
>
> >> +///     type: RustAsixPhy,
> >> +///     name: "rust_asix_phy",
> >> +///     author: "Rust for Linux Contributors",
> >> +///     description: "Rust Asix PHYs driver",
> >> +///     license: "GPL",
> >> +/// }
> >> +/// ```
> >
> > The example above doesn't compile. Please make sure your examples do.
> > (For any functions you need to implement but are not used in the
> > example, you can always use `todo!()`.)
>
> Ok
>
> >> +#[macro_export]
> >> +macro_rules! module_phy_driver {
> >> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> >> +
> >> +    (@count_devices $($x:expr),*) => {
> >> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> >> +    };
> >> +
> >> +    (@device_table $name:ident, [$($dev:expr),+]) => {
> >> +        ::kernel::macros::paste! {
> >> +            #[no_mangle]
> >> +            static [<__mod_mdio__ $name _device_table>]: [
> >> +                kernel::bindings::mdio_device_id;
> >> +                $crate::module_phy_driver!(@count_devices $($dev),+) + 1
> >> +            ] = [
> >> +                $(kernel::bindings::mdio_device_id {
> >> +                    phy_id: $dev.id,
> >> +                    phy_id_mask: $dev.mask_as_int()
> >> +                }),+,
> >> +                kernel::bindings::mdio_device_id {
> >> +                    phy_id: 0,
> >> +                    phy_id_mask: 0
> >> +                }
> >> +            ];
> >> +        }
> >> +    };
> >> +
> >> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
> >> +        struct Module<$modname> {
> >> +            _reg: kernel::net::phy::Registration,
> >> +            _p: core::marker::PhantomData<$modname>,
> >> +        }
> >
> > Why can't this struct be implemented in core::net::phy and just
> > referenced from this macro?
>
> Seems that driver! macro's type works only with type inside the
> current crate.

What `driver!` macro are you referring to? And where does this
requirement come from? (As a data point, other driver macros we
implemented in the past all use Module implementations in the kernel
crate.)

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 11:28:06 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

>> >> +#[macro_export]
>> >> +macro_rules! module_phy_driver {
>> >> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
>> >> +
>> >> +    (@count_devices $($x:expr),*) => {
>> >> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> >> +    };
>> >> +
>> >> +    (@device_table $name:ident, [$($dev:expr),+]) => {
>> >> +        ::kernel::macros::paste! {
>> >> +            #[no_mangle]
>> >> +            static [<__mod_mdio__ $name _device_table>]: [
>> >> +                kernel::bindings::mdio_device_id;
>> >> +                $crate::module_phy_driver!(@count_devices $($dev),+) + 1
>> >> +            ] = [
>> >> +                $(kernel::bindings::mdio_device_id {
>> >> +                    phy_id: $dev.id,
>> >> +                    phy_id_mask: $dev.mask_as_int()
>> >> +                }),+,
>> >> +                kernel::bindings::mdio_device_id {
>> >> +                    phy_id: 0,
>> >> +                    phy_id_mask: 0
>> >> +                }
>> >> +            ];
>> >> +        }
>> >> +    };
>> >> +
>> >> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
>> >> +        struct Module<$modname> {
>> >> +            _reg: kernel::net::phy::Registration,
>> >> +            _p: core::marker::PhantomData<$modname>,
>> >> +        }
>> >
>> > Why can't this struct be implemented in core::net::phy and just
>> > referenced from this macro?
>>
>> Seems that driver! macro's type works only with type inside the
>> current crate.
> 
> What `driver!` macro are you referring to? And where does this
> requirement come from? (As a data point, other driver macros we
> implemented in the past all use Module implementations in the kernel
> crate.)

Sorry, module! macro and please see the net-next v3 patchset because
this macro was updateded. With your approach, I got the following
error:

error[E0117]: only traits defined in the current crate can be
implemented for types defined outside of the crate
  --> drivers/net/phy/ax88796b_rust.rs:13:1
     |
     13 | / kernel::module_phy_driver! {
     14 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
     15 | |     device_table: [
     16 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
     ...  |
     23 | |     license: "GPL",
     24 | | }
        | | ^
        | | |
        | |_impl doesn't use only types from inside the current crate
        |   `kernel::net::phy::Module` is not defined in the current crate

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

[Trevor Gross]

On Wed, Oct 11, 2023 at 2:57 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:


> >> +}
> >> +
> >> +impl Registration {
> >> +    /// Registers a PHY driver.
> >> +    #[must_use]
> >
> > This function returns a `Result`, which itself already has `must_use`.
> > Are you sure you need this here?
>
> Ah, we don't need `must_use` here.
>

Whoops, I suggested this but gave you the wrong position. I think that
applying it to the `struct Registration` rather than the function
would give the correct behavior of warning if the handle isn't held.
Mind double checking this?

It doesn't prevent all cases (still needs to be assigned to something
like `_handle` and not just the black hole `_`), but it should give a
hint if anyone uses it directly. You can also add a comment

    #[must_use = "this must be assigned to ensure it is not
immediately dropped and unregistered"]

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 11:28:06 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

>> >> +/// Creates the kernel's `phy_driver` instance.
>> >> +///
>> >> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> >> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
>> >
>> > Another instance of `bindings` in a public function.
>>
>> Reexporting works?
>>
>> type DriverType = bindings::phy_driver;
> 
> No, that's a type alias. If you really want to expose it, you're
> better off creating a [transparent] new type that wraps
> bindings::phy_driver. This way `bindings` is never visible to drivers.

Understood, fixed.

The first version doesn't export it. Using the static array of
bindings::phy_driver in a kernel module makes the code simpler (avoid
dynamic memory allocation).

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

[FUJITA Tomonori]

On Tue, 10 Oct 2023 19:12:43 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Tue, Oct 10, 2023 at 6:54 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
>> >
>> > I agree, but there needs to be a way to use a driver more than once
>> > with different IDs. See
>> > https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
>> > and the followup, maybe you have a better suggestion
>>
>> Does it make sense to generate one of those based on the other, unless
>> both are provided, for instance?
> 
> That is what I was aiming for with my example `{ phy: PhyAX88772A, id:
> some_other_device_id }`, it seems to make sense that we default
> somehow.
> 
> Or, did you mean something specific like storing >1 device ID in the
> `Driver` trait? Changing from:
> 
>     trait Driver{
>         const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
>     }
> 
> To:
> 
>     trait Driver{
>         const PHY_DEVICE_IDS: &'static [DeviceId] =
>             &[DeviceId::new_with_custom_mask(0, 0)];
>     }
> 
> Maybe that would be better because the information is all in one place
> and the macro doesn't need to gain any more syntax

3. multiple phy drivers has one id in device_table.

How these approach can handle the case?

Also note that there is a driver doesn't define PHY_ID, uses
match_phy_device() instead.

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

[Trevor Gross]

On Wed, Oct 11, 2023 at 7:57 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Tue, 10 Oct 2023 19:12:43 -0400
> Trevor Gross <tmgross@umich.edu> wrote:
>
> > On Tue, Oct 10, 2023 at 6:54 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> >>
> >> On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
> >> >
> >> > I agree, but there needs to be a way to use a driver more than once
> >> > with different IDs. See
> >> > https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
> >> > and the followup, maybe you have a better suggestion
> >>
> >> Does it make sense to generate one of those based on the other, unless
> >> both are provided, for instance?
> >
> > That is what I was aiming for with my example `{ phy: PhyAX88772A, id:
> > some_other_device_id }`, it seems to make sense that we default
> > somehow.
> >
> > Or, did you mean something specific like storing >1 device ID in the
> > `Driver` trait? Changing from:
> >
> >     trait Driver{
> >         const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> >     }
> >
> > To:
> >
> >     trait Driver{
> >         const PHY_DEVICE_IDS: &'static [DeviceId] =
> >             &[DeviceId::new_with_custom_mask(0, 0)];
> >     }
> >
> > Maybe that would be better because the information is all in one place
> > and the macro doesn't need to gain any more syntax
>
> 3. multiple phy drivers has one id in device_table.
>
> How these approach can handle the case?
>
> Also note that there is a driver doesn't define PHY_ID, uses
> match_phy_device() instead.

In this case you would just need to put the same `DeviceId` in the
array for both drivers, perhaps as a const. I think this proposal
would look roughly like this:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c391891fd49db196c0f3b71fca68ce45

But I am curious what Miguel had in mind. @Alice Ryhl mentioned at the
meeting that there may be some better patterns for this sort of thing
as well.

Context for the new reviewers: our macro gets a list of `impl Driver`s
and needs to create a static array based on drivers' `DeviceId`s.
Usually each `impl Driver` has exactly one `DeviceId` (currently part
of the `Driver` trait) but sometimes there can be >1, which is the
tricky part. The macro in this patch works around this by requesting
both a list of `Driver`s and a list of `DeviceId`s (e.g. [1]), but it
would be nice to not have to duplicate this. I suggested something
where we can specify in the macro when we need >1 mdio id [2], but it
would be best if we could just store all `DeviceId`s in the trait
somehow (see above).

See [3] and its sibling messages for some more context

[1]: https://lore.kernel.org/rust-for-linux/20231009013912.4048593-4-fujita.tomonori@gmail.com/
[2]: https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
[3]: https://lore.kernel.org/rust-for-linux/20231011.080639.531402968625485566.fujita.tomonori@gmail.com/

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 23:09:03 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Wed, Oct 11, 2023 at 7:57 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Tue, 10 Oct 2023 19:12:43 -0400
>> Trevor Gross <tmgross@umich.edu> wrote:
>>
>> > On Tue, Oct 10, 2023 at 6:54 PM Miguel Ojeda
>> > <miguel.ojeda.sandonis@gmail.com> wrote:
>> >>
>> >> On Wed, Oct 11, 2023 at 12:50 AM Trevor Gross <tmgross@umich.edu> wrote:
>> >> >
>> >> > I agree, but there needs to be a way to use a driver more than once
>> >> > with different IDs. See
>> >> > https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
>> >> > and the followup, maybe you have a better suggestion
>> >>
>> >> Does it make sense to generate one of those based on the other, unless
>> >> both are provided, for instance?
>> >
>> > That is what I was aiming for with my example `{ phy: PhyAX88772A, id:
>> > some_other_device_id }`, it seems to make sense that we default
>> > somehow.
>> >
>> > Or, did you mean something specific like storing >1 device ID in the
>> > `Driver` trait? Changing from:
>> >
>> >     trait Driver{
>> >         const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
>> >     }
>> >
>> > To:
>> >
>> >     trait Driver{
>> >         const PHY_DEVICE_IDS: &'static [DeviceId] =
>> >             &[DeviceId::new_with_custom_mask(0, 0)];
>> >     }
>> >
>> > Maybe that would be better because the information is all in one place
>> > and the macro doesn't need to gain any more syntax
>>
>> 3. multiple phy drivers has one id in device_table.
>>
>> How these approach can handle the case?
>>
>> Also note that there is a driver doesn't define PHY_ID, uses
>> match_phy_device() instead.
> 
> In this case you would just need to put the same `DeviceId` in the
> array for both drivers, perhaps as a const. I think this proposal

I'm not sure modpost can handle it.

I guess that you would have two exact same lines in modules.alias,
which might works but I don't think that it's not designed to handle
such.

Or the macro needs to find the same id and keep only one.

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

[Trevor Gross]

On Wed, Oct 11, 2023 at 11:17 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> >> 3. multiple phy drivers has one id in device_table.
> >>
> >> How these approach can handle the case?
> >>
> >> Also note that there is a driver doesn't define PHY_ID, uses
> >> match_phy_device() instead.
> >
> > In this case you would just need to put the same `DeviceId` in the
> > array for both drivers, perhaps as a const. I think this proposal
>
> I'm not sure modpost can handle it.
>
> I guess that you would have two exact same lines in modules.alias,
> which might works but I don't think that it's not designed to handle
> such.
>
> Or the macro needs to find the same id and keep only one.

Good point, scratch that. What about this logic:

1. Leave `Driver` as you have it, one `DeviceId` with a null default
2. Use the macro to create a `const fn` to build the mdio_device_table
so you can add logic (like in the playground I linked)
3. Make this function skip anything that doesn't specify a `DeviceId`,
i.e. if the `DeviceID` is null don't add it to the table
4. Any devices that don't specify a `DeviceId` must specify
`match_phy_device` (I think, since it will never get matched
otherwise?). This could be verified in `create_phy_driver`
4. In the macro, allow specifying extra `DeviceId`s that aren't
specific to any phy.

Something complex like the icplus driver [1] is probably a good test
to see how any of these work out. I think that would look something
like:

    const IP175C_PHY_ID: u32 = 0x02430d80;
    const IP1001_PHY_ID: u32 = 0x02430d90;
    const IP101A_PHY_ID: u32 = 0x02430c54;

    impl Driver for Ip175c {
        const PHY_DEVICE_ID: phy::DeviceId =
phy::DeviceId::match_model(IP175C_PHY_ID);
        // ...
    }

    impl Driver for Ip1001 {
        const PHY_DEVICE_ID: phy::DeviceId =
phy::DeviceId::match_model(IP1001_PHY_ID);
        // ...
    }

    impl Driver for Ip101a {
        // no override of PHY_DEVICE_ID
        fn match_phy_device(_dev: &mut Device) -> bool { /* ... */ }
        // ...
    }

    impl Driver for Ip101g {
        // no override of PHY_DEVICE_ID
        fn match_phy_device(_dev: &mut Device) -> bool { /* ... */ }
        // ...
    }

    kernel::module_phy_driver! {
        // the first two drivers provide match_model for IP175C_PHY_ID
and IP1001_PHY_ID
        drivers: [Ip175, Ip1001, Ip101a, Ip101g],
        // this provides the extra match_exact
        // this field is optional, most drivers won't need it
        additional_matches: [phy::DeviceId::match_exact(IP101A_PHY_ID)],
        name: "...",
        author: "...",
        description: "ICPlus IP175C/IP101A/IP101G/IC1001 PHY drivers",
        license: "...",
    }

Nice because the easy default behavior is to add PHY_DEVICE_ID to the
table if it is specified. But you get the flexibility to not provide
it, or add extra entries that aren't specific to a device. Any
thoughts?

If this works, maybe PHY_DEVICE_ID should be Option<DeviceId> to make
it more clear that you don't have to specify anything?

---

I know you have the `DeviceId` functions called `new_with_exact_mask`
and similar. Maybe rename them to `match_exact`, `match_vendor`,
`match_model` so they are easier to discover based on the C macros?
Also more terse.

[1]: https://elixir.bootlin.com/linux/v6.6-rc5/source/drivers/net/phy/icplus.c

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

[Andrew Lunn]

> Good point, scratch that. What about this logic:
> 
> 1. Leave `Driver` as you have it, one `DeviceId` with a null default
> 2. Use the macro to create a `const fn` to build the mdio_device_table
> so you can add logic (like in the playground I linked)
> 3. Make this function skip anything that doesn't specify a `DeviceId`,
> i.e. if the `DeviceID` is null don't add it to the table
> 4. Any devices that don't specify a `DeviceId` must specify
> `match_phy_device` (I think, since it will never get matched
> otherwise?). This could be verified in `create_phy_driver`
> 4. In the macro, allow specifying extra `DeviceId`s that aren't
> specific to any phy.

To some extent, we try to avoid over engineering a solution. There
might never be a Rust driver which needs to have different entries in
these tables.

For the moment, i suggest KISS. Have a 1:1 mapping between these two
tables. If a Rust PHY driver ever comes along which needs something
different, then solve the problem only then. And hopefully when that
happens, there will be a lot more Rust drivers in general, and a
better understanding of this problem in general, since it is not
really limited to PHY drivers. Many drivers need two tables like this.

	Andrew