[PATCH net-next v9 0/4] Rust abstractions for network PHY drivers

[PATCH net-next v9 0/4] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

This patchset adds Rust abstractions for phylib. It doesn't fully
cover the C APIs 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.

The first patch introduces Rust bindings for phylib.

It's unlikely that bindgen will support safe access to a bit field in
phy_device struct until the next merge window. So this version uses a
workaround to access to a bit field safely instead of the generated
code by bindgen.

The second patch adds a macro to declare a kernel module for PHYs
drivers.

The third adds the Rust ETHERNET PHY LIBRARY entry to MAINTAINERS
file; adds the binding file and me as a maintainer (as Andrew Lunn
suggested) with Trevor Gross as a reviewer.

The last patch introduces the Rust version of Asix PHY driver,
drivers/net/phy/ax88796b.c. The features are equivalent to the C
version. You can choose C (by default) or Rust version on kernel
configuration.

v9:
  - adds a workaround to access to a bit field in phy_device
  - fixes a comment typo
v8: https://lore.kernel.org/netdev/20231123050412.1012252-1-fujita.tomonori@gmail.com/
  - updates the safety comments on Device and its related code
  - uses _phy_start_aneg instead of phy_start_aneg
  - drops the patch for enum synchronization
  - moves Sync From Registration to DriverVTable
  - fixes doctest errors
  - minor cleanups
v7: https://lore.kernel.org/netdev/20231026001050.1720612-1-fujita.tomonori@gmail.com/T/
  - renames get_link() to is_link_up()
  - improves the macro format
  - improves the commit log in the third patch
  - improves comments
v6: https://lore.kernel.org/netdev/20231025.090243.1437967503809186729.fujita.tomonori@gmail.com/T/
  - improves comments
  - makes the requirement of phy_drivers_register clear
  - fixes Makefile of the third patch
v5: https://lore.kernel.org/all/20231019.094147.1808345526469629486.fujita.tomonori@gmail.com/T/
  - drops the rustified-enum option, writes match by hand; no *risk* of UB
  - adds Miguel's patch for enum checking
  - moves CONFIG_RUST_PHYLIB_ABSTRACTIONS to drivers/net/phy/Kconfig
  - adds a new entry for this abstractions in MAINTAINERS
  - changes some of Device's methods to take &mut self
  - comment improvment
v4: https://lore.kernel.org/netdev/20231012125349.2702474-1-fujita.tomonori@gmail.com/T/
  - split the core patch
  - making Device::from_raw() private
  - comment improvement with code update
  - commit message improvement
  - avoiding using bindings::phy_driver in public functions
  - using an anonymous constant in module_phy_driver macro
v3: https://lore.kernel.org/netdev/20231011.231607.1747074555988728415.fujita.tomonori@gmail.com/T/
  - changes the base tree to net-next from rust-next
  - makes this feature optional; only enabled with CONFIG_RUST_PHYLIB_BINDINGS=y
  - cosmetic code and comment improvement
  - adds copyright
v2: https://lore.kernel.org/netdev/20231006094911.3305152-2-fujita.tomonori@gmail.com/T/
  - build failure fix
  - function renaming
v1: https://lore.kernel.org/netdev/20231002085302.2274260-3-fujita.tomonori@gmail.com/T/


FUJITA Tomonori (4):
  rust: core abstractions for network PHY drivers
  rust: net::phy add module_phy_driver macro
  MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY
  net: phy: add Rust Asix PHY driver

 MAINTAINERS                      |  16 +
 drivers/net/phy/Kconfig          |  16 +
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 135 +++++
 rust/bindings/bindings_helper.h  |   3 +
 rust/kernel/lib.rs               |   3 +
 rust/kernel/net.rs               |   6 +
 rust/kernel/net/phy.rs           | 900 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 9 files changed, 1086 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: 8470e4368b0f3ba788814f3b3c1142ce51d87e21
-- 
2.34.1

[PATCH net-next v9 1/4] 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.

This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.

This patch enables unstable const_maybe_uninit_zeroed feature for
kernel crate to enable unsafe code to handle a constant value with
uninitialized data. With the feature, the abstractions can initialize
a phy_driver structure with zero easily; instead of initializing all
the members by hand. It's supposed to be stable in the not so distant
future.

Link: https://github.com/rust-lang/rust/pull/116218

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

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 25cfc5ded1da..8d00cece5e23 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -60,6 +60,14 @@ config FIXED_PHY
 
 	  Currently tested with mpc866ads and mpc8349e-mitx.
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "Rust PHYLIB abstractions support"
+        depends on RUST
+        depends on PHYLIB=y
+        help
+          Adds support needed for PHY drivers written in Rust. It provides
+          a wrapper around the C phylib core.
+
 config SFP
 	tristate "SFP cage support"
 	depends on I2C && PHYLINK
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 85f013ed4ca4..eaf01df7d97a 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 e6aff80b521f..7ac39874aeac 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(offset_of)]
@@ -38,6 +39,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..fe415cb369d3
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..5d220187eec9
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,754 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
+
+use core::marker::PhantomData;
+
+/// PHY state machine states.
+///
+/// Corresponds to the kernel's [`enum phy_state`].
+///
+/// Some of PHY drivers access to the state of PHY's software state machine.
+///
+/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
+#[derive(PartialEq, Eq)]
+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+}
+
+/// A mode of Ethernet communication.
+///
+/// PHY drivers get duplex information from hardware and update the current state.
+pub enum DuplexMode {
+    /// PHY is in full-duplex mode.
+    Full,
+    /// PHY is in half-duplex mode.
+    Half,
+    /// PHY is in unknown duplex mode.
+    Unknown,
+}
+
+/// An instance of a PHY device.
+///
+/// Wraps the kernel's [`struct phy_device`].
+///
+/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
+/// executes [`Driver`]'s methods during the callback.
+///
+/// # Invariants
+///
+/// Referencing a `phy_device` using this struct asserts that you are in
+/// a context where all methods defined on this struct are safe to call.
+///
+/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
+// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
+// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
+// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
+// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
+// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
+// to the instance.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
+    /// and the caller must be in a context where all methods defined on this struct
+    /// are safe to call.
+    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
+        let ptr = ptr.cast::<Self>();
+        // SAFETY: by the function requirements the pointer is valid and we have unique access for
+        // the duration of `'a`.
+        unsafe { &mut *ptr }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn phy_id(&self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Gets the state of PHY state machine states.
+    pub fn state(&self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let state = unsafe { (*phydev).state };
+        // TODO: this conversion code will be replaced with automatically generated code by bindgen
+        // when it becomes possible.
+        // better to call WARN_ONCE() when the state is out-of-range.
+        match state {
+            bindings::phy_state_PHY_DOWN => DeviceState::Down,
+            bindings::phy_state_PHY_READY => DeviceState::Ready,
+            bindings::phy_state_PHY_HALTED => DeviceState::Halted,
+            bindings::phy_state_PHY_ERROR => DeviceState::Error,
+            bindings::phy_state_PHY_UP => DeviceState::Up,
+            bindings::phy_state_PHY_RUNNING => DeviceState::Running,
+            bindings::phy_state_PHY_NOLINK => DeviceState::NoLink,
+            bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest,
+            _ => DeviceState::Error,
+        }
+    }
+
+    /// Gets the current link state.
+    ///
+    /// It returns true if the link is up.
+    pub fn is_link_up(&self) -> bool {
+        const LINK_IS_UP: u64 = 1;
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(14, 1) == LINK_IS_UP
+    }
+
+    /// Gets the current auto-negotiation configuration.
+    ///
+    /// It returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&self) -> bool {
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
+    }
+
+    /// Gets the current auto-negotiation state.
+    ///
+    /// It returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&self) -> bool {
+        const AUTONEG_COMPLETED: u64 = 1;
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(15, 1) == AUTONEG_COMPLETED
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&mut self, speed: u32) {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        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: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).duplex = v };
+    }
+
+    /// Reads a given C22 PHY register.
+    // This function reads a hardware register and updates the stats so takes `&mut self`.
+    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).
+///
+/// These flag values are used in [`Driver::FLAGS`].
+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> {
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C core code ensures that the accessors on
+            // `Device` are okay to call even though `phy_device->lock`
+            // might not be held.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C core code ensures that the accessors on
+            // `Device` are okay to call even though `phy_device->lock`
+            // might not be held.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: This callback is called only in contexts
+        // where we hold `phy_device->lock`, so the accessors on
+        // `Device` are okay to call.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            // CAST: the C side verifies devnum < 32.
+            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
+            Ok(ret.into())
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum as u8, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: This callback is called only in contexts
+        // where we hold `phy_device->lock`, so the accessors on
+        // `Device` are okay to call.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+}
+
+/// Driver structure for a particular PHY type.
+///
+/// Wraps the kernel's [`struct phy_driver`].
+/// This is used to register a driver for a particular PHY type with the kernel.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+///
+/// [`struct phy_driver`]: ../../../../../../../include/linux/phy.h
+#[repr(transparent)]
+pub struct DriverVTable(Opaque<bindings::phy_driver>);
+
+// SAFETY: `DriverVTable` has no &self methods, so immutable references to it
+// are useless.
+unsafe impl Sync for DriverVTable {}
+
+/// Creates a [`DriverVTable`] instance from [`Driver`].
+///
+/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
+///
+/// [`module_phy_driver`]: crate::module_phy_driver
+pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
+    // INVARIANT: All the fields of `struct phy_driver` are initialized properly.
+    DriverVTable(Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr().cast_mut(),
+        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() }
+    }))
+}
+
+/// Driver implementation for a particular PHY type.
+///
+/// This trait is used to create a [`DriverVTable`].
+#[vtable]
+pub trait Driver {
+    /// Defines certain other features this PHY supports.
+    /// It is a combination of the flags in the [`flags`] module.
+    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.
+    /// The default id and mask are zero.
+    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 [`Driver::PHY_DEVICE_ID`].
+    fn match_phy_device(_dev: &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 PHY drivers.
+///
+/// Registers [`DriverVTable`] instances with the kernel. They will be unregistered when dropped.
+///
+/// # Invariants
+///
+/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: Pin<&'static mut [DriverVTable]>,
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    pub fn register(
+        module: &'static crate::ThisModule,
+        drivers: Pin<&'static mut [DriverVTable]>,
+    ) -> Result<Self> {
+        if drivers.is_empty() {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of
+        // the `drivers` slice are initialized properly. `drivers` will not be moved.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
+        })?;
+        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
+        Ok(Registration { drivers })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `self.drivers` is valid.
+        // So an FFI call with a valid pointer.
+        unsafe {
+            bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32)
+        };
+    }
+}
+
+/// An identifier for PHY devices on an MDIO/MII bus.
+///
+/// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate
+/// PHY driver.
+pub struct DeviceId {
+    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()
+    }
+
+    // macro use only
+    #[doc(hidden)]
+    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
+        bindings::mdio_device_id {
+            phy_id: self.id,
+            phy_id_mask: 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,
+        }
+    }
+}
-- 
2.34.1

[PATCH net-next v9 2/4] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

This macro creates an array of kernel's `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.

A PHY driver should use this macro.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 5d220187eec9..d9cec139324a 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -752,3 +752,149 @@ const fn as_int(&self) -> u32 {
         }
     }
 }
+
+/// Declares a kernel module for PHYs drivers.
+///
+/// This creates a static array of kernel's `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. Every driver needs an entry in `device_table`.
+///
+/// # Examples
+///
+/// ```
+/// # mod module_phy_driver_sample {
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// kernel::module_phy_driver! {
+///     drivers: [PhySample],
+///     device_table: [
+///         DeviceId::new_with_driver::<PhySample>()
+///     ],
+///     name: "rust_sample_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust sample PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhySample;
+///
+/// #[vtable]
+/// impl phy::Driver for PhySample {
+///     const NAME: &'static CStr = c_str!("PhySample");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001);
+/// }
+/// # }
+/// ```
+///
+/// This expands to the following code:
+///
+/// ```ignore
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// struct Module {
+///     _reg: ::kernel::net::phy::Registration,
+/// }
+///
+/// module! {
+///     type: Module,
+///     name: "rust_sample_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust sample PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhySample;
+///
+/// #[vtable]
+/// impl phy::Driver for PhySample {
+///     const NAME: &'static CStr = c_str!("PhySample");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001);
+/// }
+///
+/// const _: () = {
+///     static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] =
+///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
+///
+///     impl ::kernel::Module for Module {
+///         fn init(module: &'static ThisModule) -> Result<Self> {
+///             let drivers = unsafe { &mut DRIVERS };
+///             let mut reg = ::kernel::net::phy::Registration::register(
+///                 module,
+///                 ::core::pin::Pin::static_mut(drivers),
+///             )?;
+///             Ok(Module { _reg: reg })
+///         }
+///     }
+/// };
+///
+/// #[cfg(MODULE)]
+/// #[no_mangle]
+/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0x00000001,
+///         phy_id_mask: 0xffffffff,
+///     },
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0,
+///         phy_id_mask: 0,
+///     },
+/// ];
+/// ```
+#[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 [$($dev:expr),+]) => {
+        // SAFETY: C will not read off the end of this constant since the last element is zero.
+        #[cfg(MODULE)]
+        #[no_mangle]
+        static __mod_mdio__phydev_device_table: [$crate::bindings::mdio_device_id;
+            $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [
+            $($dev.mdio_device_id()),+,
+            $crate::bindings::mdio_device_id {
+                phy_id: 0,
+                phy_id_mask: 0
+            }
+        ];
+    };
+
+    (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {
+        struct Module {
+            _reg: $crate::net::phy::Registration,
+        }
+
+        $crate::prelude::module! {
+            type: Module,
+            $($f)*
+        }
+
+        const _: () = {
+            static mut DRIVERS: [$crate::net::phy::DriverVTable;
+                $crate::module_phy_driver!(@count_devices $($driver),+)] =
+                [$($crate::net::phy::create_phy_driver::<$driver>()),+];
+
+            impl $crate::Module for Module {
+                fn init(module: &'static ThisModule) -> Result<Self> {
+                    // SAFETY: The anonymous constant guarantees that nobody else can access
+                    // the `DRIVERS` static. The array is used only in the C side.
+                    let drivers = unsafe { &mut DRIVERS };
+                    let mut reg = $crate::net::phy::Registration::register(
+                        module,
+                        ::core::pin::Pin::static_mut(drivers),
+                    )?;
+                    Ok(Module { _reg: reg })
+                }
+            }
+        };
+
+        $crate::module_phy_driver!(@device_table [$($dev),+]);
+    }
+}
-- 
2.34.1

[PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[FUJITA Tomonori]

Adds me as a maintainer and Trevor as a reviewer.

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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d2865c1e479..0166188446ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7922,6 +7922,14 @@ F:	include/uapi/linux/mdio.h
 F:	include/uapi/linux/mii.h
 F:	net/core/of_net.c
 
+ETHERNET PHY LIBRARY [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:	Trevor Gross <tmgross@umich.edu>
+L:	netdev@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/net/phy.rs
+
 EXEC & BINFMT API
 R:	Eric Biederman <ebiederm@xmission.com>
 R:	Kees Cook <keescook@chromium.org>
-- 
2.34.1

[PATCH net-next v9 4/4] 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 version kernel
configuration.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
 MAINTAINERS                      |   8 ++
 drivers/net/phy/Kconfig          |   8 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 135 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 5 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 0166188446ac..1ed8b2ae5099 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3083,6 +3083,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/net/asix,ax88796c.yaml
 F:	drivers/net/ethernet/asix/ax88796c_*
 
+ASIX PHY DRIVER [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:	Trevor Gross <tmgross@umich.edu>
+L:	netdev@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/ax88796b_rust.rs
+
 ASPEED CRYPTO DRIVER
 M:	Neal Liu <neal_liu@aspeedtech.com>
 L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 8d00cece5e23..0f483c3dfed9 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -112,6 +112,14 @@ 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 for Asix PHYs"
+	depends on RUST_PHYLIB_ABSTRACTIONS && AX88796B_PHY
+	help
+	  Uses the Rust reference driver for Asix PHYs (ax88796b_rust.ko).
+	  The features are equivalent. It supports the Asix Electronics PHY
+	  found in the X-Surf 100 AX88796B package.
+
 config BROADCOM_PHY
 	tristate "Broadcom 54XX PHYs"
 	select BCM_NET_PHYLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f65e85c91fc1..ab450a46b855 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -37,7 +37,11 @@ obj-$(CONFIG_ADIN1100_PHY)	+= adin1100.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
 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..5c92572962dc
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Rust Asix PHYs driver
+//!
+//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
+use kernel::{
+    c_str,
+    net::phy::{self, DeviceId, Driver},
+    prelude::*,
+    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>()
+    ],
+    name: "rust_asix_phy",
+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    description: "Rust Asix PHYs driver",
+    license: "GPL",
+}
+
+const MII_BMCR: u16 = uapi::MII_BMCR as u16;
+const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;
+const BMCR_FULLDPLX: u16 = uapi::BMCR_FULLDPLX as u16;
+
+// 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 Driver for PhyAX88772A {
+    const FLAGS: u32 = phy::flags::IS_INTERNAL;
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
+    const PHY_DEVICE_ID: DeviceId = 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.is_link_up() {
+            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(MII_BMCR)?;
+
+        if ret & BMCR_SPEED100 != 0 {
+            dev.set_speed(uapi::SPEED_100);
+        } else {
+            dev.set_speed(uapi::SPEED_10);
+        }
+
+        let duplex = if ret & 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: DeviceId = 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: DeviceId = 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..08f5e9334c9e 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,3 +7,5 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mii.h>
+#include <uapi/linux/ethtool.h>
-- 
2.34.1

Re: [PATCH net-next v9 0/4] Rust abstractions for network PHY drivers

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> This patchset adds Rust abstractions for phylib. It doesn't fully
> cover the C APIs 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.

Does qemu virt platform emulate either?

BR, Jarkko

Re: [PATCH net-next v9 0/4] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

On Tue, 05 Dec 2023 03:35:10 +0200
"Jarkko Sakkinen" <jarkko@kernel.org> wrote:

> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
>> This patchset adds Rust abstractions for phylib. It doesn't fully
>> cover the C APIs 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.
> 
> Does qemu virt platform emulate either?

No. I tested these drivers with hardware. It would be useful if Qeumu
spports PHYs.

https://lore.kernel.org/all/460e7d2f-2fd4-475b-9156-88d61c9f7347@lunn.ch/

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.

Just a question: is `_ABSTRACTIONS` a convention or just for this
config flag?

That would read anyway that this rust absraction of phy absraction
layer or similar.

Why not e.g.

- `CONFIG_RUST_PHYLIB_BINDINGS`
- `CONFIG_RUST_PHYLIB_API`
- Or even just `CONFIG_RUST_PHYLIB`?

BR, Jarkko

Re: [PATCH net-next v9 2/4] rust: net::phy add module_phy_driver macro

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> This macro creates an array of kernel's `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.
>
> A PHY driver should use this macro.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 5d220187eec9..d9cec139324a 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -752,3 +752,149 @@ const fn as_int(&self) -> u32 {
>          }
>      }
>  }
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of kernel's `struct phy_driver` and registers it.

s/This creates a static array/Creates a static array/

Suggestion for better formulation:

"Creates a static array of `struct phy driver` instances, and registers them.""

> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.

s/This/`kernel::module_phy_driver`/

Or at least I did not see it introduced earlier in the text.

BR, Jarkko

Re: [PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> Adds me as a maintainer and Trevor as a reviewer.
>
> 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>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

This is lacking sob from Trevor.

BR, Jarkko

Re: [PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[Trevor Gross]

On Mon, Dec 4, 2023 at 8:16 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Adds me as a maintainer and Trevor as a reviewer.
>
> 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>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---

You may add:

Signed-off-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[Trevor Gross]

On Mon, Dec 4, 2023 at 8:49 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> > Adds me as a maintainer and Trevor as a reviewer.
> >
> > 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>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> This is lacking sob from Trevor.
>
> BR, Jarkko

Thanks, was not aware this was needed.

Re: [PATCH net-next v9 4/4] net: phy: add Rust Asix PHY driver

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
> features are equivalent. You can choose C or Rust version kernel
> configuration.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Hardware-agnostic feature must have something that spins in qemu, at least
for a wide-coverage feature like phy it would be imho grazy to merge these
without a phy driver that anyone can test out.

BR, Jarkko

Re: [PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[Jarkko Sakkinen]

On Tue Dec 5, 2023 at 3:53 AM EET, Trevor Gross wrote:
> On Mon, Dec 4, 2023 at 8:49 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> > > Adds me as a maintainer and Trevor as a reviewer.
> > >
> > > 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>
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >
> > This is lacking sob from Trevor.
> >
> > BR, Jarkko
>
> Thanks, was not aware this was needed.

Trevor is an actor here so it is just common sense to be aware that
the person is signed up for the job. Compare to any other contract
that you've made in your span of life :-)

BR, Jarkko

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[Boqun Feng]

On Tue, Dec 05, 2023 at 10:14:17AM +0900, FUJITA Tomonori wrote:
[...]
> +    /// Gets the current link state.
> +    ///
> +    /// It returns true if the link is up.
> +    pub fn is_link_up(&self) -> bool {
> +        const LINK_IS_UP: u64 = 1;
> +        // TODO: the code to access to the bit field will be replaced with automatically
> +        // generated code by bindgen when it becomes possible.
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> +        bit_field.get(14, 1) == LINK_IS_UP

I made a mistake here [1], this should be:

    let bit_field = unsafe { &*(core::ptr::addr_of!((*self.0.get())._bitfield_1)) };
    bit_field.get(14, 1) == LINK_IS_UP

without `core::ptr::add_of!`, `*(self.0.get())` would still create a
temporary `&` to the underlying object I believe. `addr_of!` is the way
to avoid create the temporary reference. Same for the other functions.

[1]: https://lore.kernel.org/rust-for-linux/ZT6fzfV9GUQOZnlx@boqun-archlinux/

Regards,
Boqun

> +    }
> +
> +    /// Gets the current auto-negotiation configuration.
> +    ///
> +    /// It returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&self) -> bool {
> +        // TODO: the code to access to the bit field will be replaced with automatically
> +        // generated code by bindgen when it becomes possible.
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> +        bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
> +    }
> +
> +    /// Gets the current auto-negotiation state.
> +    ///
> +    /// It returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u64 = 1;
> +        // TODO: the code to access to the bit field will be replaced with automatically
> +        // generated code by bindgen when it becomes possible.
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> +        bit_field.get(15, 1) == AUTONEG_COMPLETED
> +    }
> +
[...]

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[FUJITA Tomonori]

On Tue, 05 Dec 2023 03:43:50 +0200
"Jarkko Sakkinen" <jarkko@kernel.org> wrote:

> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>>
>> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> Just a question: is `_ABSTRACTIONS` a convention or just for this
> config flag?

It's a convention.

https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings


> That would read anyway that this rust absraction of phy absraction
> layer or similar.
> 
> Why not e.g.
> 
> - `CONFIG_RUST_PHYLIB_BINDINGS`
> - `CONFIG_RUST_PHYLIB_API`
> - Or even just `CONFIG_RUST_PHYLIB`?

I guess that CONFIG_RUST_PHYLIB_API or CONFIG_RUST_PHYLIB could be
used. CONFIG_RUST_PHYLIB_ABSTRACTIONS was preferred during the past
reviewing.

Re: [PATCH net-next v9 4/4] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

On Tue, Dec 05, 2023 at 03:54:48AM +0200, Jarkko Sakkinen wrote:
> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> > This is the Rust implementation of drivers/net/phy/ax88796b.c. The
> > features are equivalent. You can choose C or Rust version kernel
> > configuration.
> >
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > Reviewed-by: Trevor Gross <tmgross@umich.edu>
> > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> Hardware-agnostic feature must have something that spins in qemu, at least
> for a wide-coverage feature like phy it would be imho grazy to merge these
> without a phy driver that anyone can test out.

There are no hardware agnostic features here. PHY drivers are pretty
much thin layers which talk to the hardware. Take a look at the C
drivers, what hardware agnostic features do you see in them?

People can easily test this driver, its an easily available USB
dongle, which will work on any USB port, with any SoC architectures.

    Andrew

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[FUJITA Tomonori]

On Mon, 4 Dec 2023 18:00:15 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Dec 05, 2023 at 10:14:17AM +0900, FUJITA Tomonori wrote:
> [...]
>> +    /// Gets the current link state.
>> +    ///
>> +    /// It returns true if the link is up.
>> +    pub fn is_link_up(&self) -> bool {
>> +        const LINK_IS_UP: u64 = 1;
>> +        // TODO: the code to access to the bit field will be replaced with automatically
>> +        // generated code by bindgen when it becomes possible.
>> +        // SAFETY: The struct invariant ensures that we may access
>> +        // this field without additional synchronization.
>> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
>> +        bit_field.get(14, 1) == LINK_IS_UP
> 
> I made a mistake here [1], this should be:
> 
>     let bit_field = unsafe { &*(core::ptr::addr_of!((*self.0.get())._bitfield_1)) };
>     bit_field.get(14, 1) == LINK_IS_UP
> 
> without `core::ptr::add_of!`, `*(self.0.get())` would still create a
> temporary `&` to the underlying object I believe. `addr_of!` is the way
> to avoid create the temporary reference. Same for the other functions.

If so, how about functions to access to non bit field like phy_id()?

pub fn phy_id(&self) -> u32 {
    let phydev = self.0.get();
    unsafe { (*phydev).phy_id }
}

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[Boqun Feng]

On Tue, Dec 05, 2023 at 12:23:20PM +0900, FUJITA Tomonori wrote:
> On Mon, 4 Dec 2023 18:00:15 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 05, 2023 at 10:14:17AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> +    /// Gets the current link state.
> >> +    ///
> >> +    /// It returns true if the link is up.
> >> +    pub fn is_link_up(&self) -> bool {
> >> +        const LINK_IS_UP: u64 = 1;
> >> +        // TODO: the code to access to the bit field will be replaced with automatically
> >> +        // generated code by bindgen when it becomes possible.
> >> +        // SAFETY: The struct invariant ensures that we may access
> >> +        // this field without additional synchronization.
> >> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> >> +        bit_field.get(14, 1) == LINK_IS_UP
> > 
> > I made a mistake here [1], this should be:
> > 
> >     let bit_field = unsafe { &*(core::ptr::addr_of!((*self.0.get())._bitfield_1)) };
> >     bit_field.get(14, 1) == LINK_IS_UP
> > 
> > without `core::ptr::add_of!`, `*(self.0.get())` would still create a
> > temporary `&` to the underlying object I believe. `addr_of!` is the way
> > to avoid create the temporary reference. Same for the other functions.
> 
> If so, how about functions to access to non bit field like phy_id()?
> 
> pub fn phy_id(&self) -> u32 {
>     let phydev = self.0.get();
>     unsafe { (*phydev).phy_id }
> }

Good point, so cancel the above comment. I had a misunderstanding that a
place expression would create temporary references, but I was wrong.

Regards,
Boqun

Re: [PATCH net-next v9 2/4] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Tue, 05 Dec 2023 03:48:32 +0200
"Jarkko Sakkinen" <jarkko@kernel.org> wrote:

> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
>> This macro creates an array of kernel's `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.
>>
>> A PHY driver should use this macro.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> ---
>>  rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 146 insertions(+)
>>
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 5d220187eec9..d9cec139324a 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -752,3 +752,149 @@ const fn as_int(&self) -> u32 {
>>          }
>>      }
>>  }
>> +
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of kernel's `struct phy_driver` and registers it.
> 
> s/This creates a static array/Creates a static array/
> 
> Suggestion for better formulation:
> 
> "Creates a static array of `struct phy driver` instances, and registers them.""

I follow Rust comment style here. Maybe I should do:

s/This creates/This macro creates/

>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
> 
> s/This/`kernel::module_phy_driver`/
> 
> Or at least I did not see it introduced earlier in the text.

s/This/This macro/ works for you?

Likely `kernel::` part would be changed in the future.

Re: [PATCH net-next v9 3/4] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY

[FUJITA Tomonori]

On Mon, 4 Dec 2023 20:53:23 -0500
Trevor Gross <tmgross@umich.edu> wrote:

> On Mon, Dec 4, 2023 at 8:16 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Adds me as a maintainer and Trevor as a reviewer.
>>
>> 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>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> ---
> 
> You may add:
> 
> Signed-off-by: Trevor Gross <tmgross@umich.edu>

Thanks, I will.

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[Benno Lossin]

On 12/5/23 02:14, FUJITA Tomonori wrote:
> @@ -0,0 +1,754 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
> +
> +use core::marker::PhantomData;
> +
> +/// PHY state machine states.
> +///
> +/// Corresponds to the kernel's [`enum phy_state`].
> +///
> +/// Some of PHY drivers access to the state of PHY's software state machine.

This sentence reads a bit weird, what are you trying to say?

> +///
> +/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
> +#[derive(PartialEq, Eq)]
> +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,

I took a look at `enum phy_state` and found that you only copied the
first sentence of each state description, why is that?

> +}
> +
> +/// A mode of Ethernet communication.
> +///
> +/// PHY drivers get duplex information from hardware and update the current state.

Are you trying to say that the driver automatically queries the
hardware? You could express this more clearly.

> +pub enum DuplexMode {
> +    /// PHY is in full-duplex mode.
> +    Full,
> +    /// PHY is in half-duplex mode.
> +    Half,
> +    /// PHY is in unknown duplex mode.
> +    Unknown,
> +}
> +
> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's [`struct phy_device`].
> +///
> +/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
> +/// executes [`Driver`]'s methods during the callback.
> +///
> +/// # Invariants
> +///
> +/// Referencing a `phy_device` using this struct asserts that you are in
> +/// a context where all methods defined on this struct are safe to call.

I know that Alice suggested this, but I reading it now, it sounds a
bit weird. When reading this it sounds like a requirement for everyone
using a `Device`. It would be better to phrase it so that it sounds like
something that users of `Device` can rely upon.

Also, I would prefer for this invariant to be a simple one, for example:
"The mutex of `self.0` is held".
The only problem with that are the `resume` and `suspend` methods.
Andrew mentioned that there is some tribal knowledge on this topic, but
I don't see this written down anywhere here. I can't really suggest an
improvement to invariant without knowing the whole picture.

> +/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
> +// to the instance.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
> +    /// and the caller must be in a context where all methods defined on this struct
> +    /// are safe to call.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
> +        // the duration of `'a`.
> +        unsafe { &mut *ptr }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn phy_id(&self) -> u32 {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.

At the moment the invariant only states that "all functions on
`Device` are safe to call". It does not say anything about accessing
fields. I hope this shows why I think the invariant is problematic.

-- 
Cheers,
Benno

Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

[FUJITA Tomonori]

On Thu, 07 Dec 2023 17:25:22 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 12/5/23 02:14, FUJITA Tomonori wrote:
>> @@ -0,0 +1,754 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
>> +
>> +//! Network PHY device.
>> +//!
>> +//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
>> +
>> +use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>> +
>> +use core::marker::PhantomData;
>> +
>> +/// PHY state machine states.
>> +///
>> +/// Corresponds to the kernel's [`enum phy_state`].
>> +///
>> +/// Some of PHY drivers access to the state of PHY's software state machine.
> 
> This sentence reads a bit weird, what are you trying to say?

It's copy of the PHY doc. For me, it means that if my PHY driver
doesn't need access to that state, I don't need to know anything about
this enum.


>> +/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
>> +#[derive(PartialEq, Eq)]
>> +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,
> 
> I took a look at `enum phy_state` and found that you only copied the
> first sentence of each state description, why is that?

I thought that the first sentence is enough but I'll copy the full
description if you prefer.


>> +/// A mode of Ethernet communication.
>> +///
>> +/// PHY drivers get duplex information from hardware and update the current state.
> 
> Are you trying to say that the driver automatically queries the
> hardware? You could express this more clearly.

It's the copy from the PHY doc. I assume that it's clear for driver
developers; your driver gets the information from the hardware and
updates the state via the APIs.


>> +pub enum DuplexMode {
>> +    /// PHY is in full-duplex mode.
>> +    Full,
>> +    /// PHY is in half-duplex mode.
>> +    Half,
>> +    /// PHY is in unknown duplex mode.
>> +    Unknown,
>> +}
>> +
>> +/// An instance of a PHY device.
>> +///
>> +/// Wraps the kernel's [`struct phy_device`].
>> +///
>> +/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
>> +/// executes [`Driver`]'s methods during the callback.
>> +///
>> +/// # Invariants
>> +///
>> +/// Referencing a `phy_device` using this struct asserts that you are in
>> +/// a context where all methods defined on this struct are safe to call.
> 
> I know that Alice suggested this, but I reading it now, it sounds a
> bit weird. When reading this it sounds like a requirement for everyone
> using a `Device`. It would be better to phrase it so that it sounds like
> something that users of `Device` can rely upon.

I guess that every reviewer has their preferences. I don't think that
I can write a comment that makes every reviewer fully happy about.

For me, as Alice said, "at least it is correct". 


> Also, I would prefer for this invariant to be a simple one, for example:
> "The mutex of `self.0` is held".
> The only problem with that are the `resume` and `suspend` methods.
> Andrew mentioned that there is some tribal knowledge on this topic, but
> I don't see this written down anywhere here. I can't really suggest an
> improvement to invariant without knowing the whole picture.
> 
>> +/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
>> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
>> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
>> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
>> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
>> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
>> +// to the instance.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
>> +    /// and the caller must be in a context where all methods defined on this struct
>> +    /// are safe to call.
>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>> +        let ptr = ptr.cast::<Self>();
>> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
>> +        // the duration of `'a`.
>> +        unsafe { &mut *ptr }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&self) -> u32 {
>> +        let phydev = self.0.get();
>> +        // SAFETY: The struct invariant ensures that we may access
>> +        // this field without additional synchronization.
> 
> At the moment the invariant only states that "all functions on
> `Device` are safe to call". It does not say anything about accessing
> fields. I hope this shows why I think the invariant is problematic.

The previous invariant was:

`self.0` is always in a valid state.

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

It says accessing fields, right? For me, if so, the current invariant
suggested by Alice also says it.

Re: [PATCH net-next v9 2/4] rust: net::phy add module_phy_driver macro

[Miguel Ojeda]

On Tue, Dec 5, 2023 at 4:44 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I follow Rust comment style here. Maybe I should do:
>
> s/This creates/This macro creates/

I think "Creates..." is fine, it is what we do in other files for functions.

> Likely `kernel::` part would be changed in the future.

Similarly, you could say "Corresponds...".

Cheers,
Miguel

Re: [PATCH net-next v9 2/4] rust: net::phy add module_phy_driver macro

[Benno Lossin]

On 12/5/23 02:14, FUJITA Tomonori wrote:
> This macro creates an array of kernel's `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.
> 
> A PHY driver should use this macro.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

-- 
Cheers,
Benno