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

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

[FUJITA Tomonori]

No code change since v9; only commit log updates.

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.

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.

v10:
  - adds Trevor's SoB to the third patch
  - adds Benno's Reviewed-by to the second patch
v9: https://lore.kernel.org/netdev/20231205.124531.842372711631366729.fujita.tomonori@gmail.com/T/
  - 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: 172db56d90d29e47e7d0d64885d5dbd92c87ec42
-- 
2.34.1

[PATCH net-next v10 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

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

[Andrew Lunn]

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

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[Boqun Feng]

The current safety comment for impl Sync for DriverVTable has two
problem:

* the correctness is unclear, since all types impl Any[1], therefore all
  types have a `&self` method (Any::type_id).

* it doesn't explain why useless of immutable references can ensure the
  safety.

Fix this by rewritting the comment.

[1]: https://doc.rust-lang.org/std/any/trait.Any.html

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
This is a follow-up for my ignored feedback:

	https://lore.kernel.org/rust-for-linux/ZV5FjEM1EWm6iTAm@boqun-archlinux/	

Honestly, I believe that people who are active in the review process all
get it right, but I want to make sure who read the code later can also
understand it and reuse the reasoning in their code. Hence this
improvement.

Tomo, feel free to fold it in your patch if you and others think the
wording is fine.

 rust/kernel/net/phy.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index d9cec139324a..e3377f8f36b7 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -489,8 +489,8 @@ impl<T: Driver> Adapter<T> {
 #[repr(transparent)]
 pub struct DriverVTable(Opaque<bindings::phy_driver>);
 
-// SAFETY: `DriverVTable` has no &self methods, so immutable references to it
-// are useless.
+// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to
+// share `&DriverVTable` across execution context boundries.
 unsafe impl Sync for DriverVTable {}
 
 /// Creates a [`DriverVTable`] instance from [`Driver`].
-- 
2.43.0

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[Boqun Feng]

+ Tomo

On Mon, Dec 11, 2023 at 11:49:09AM -0800, Boqun Feng wrote:
> The current safety comment for impl Sync for DriverVTable has two
> problem:
> 
> * the correctness is unclear, since all types impl Any[1], therefore all
>   types have a `&self` method (Any::type_id).
> 
> * it doesn't explain why useless of immutable references can ensure the
>   safety.
> 
> Fix this by rewritting the comment.
> 
> [1]: https://doc.rust-lang.org/std/any/trait.Any.html
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> This is a follow-up for my ignored feedback:
> 
> 	https://lore.kernel.org/rust-for-linux/ZV5FjEM1EWm6iTAm@boqun-archlinux/	
> 
> Honestly, I believe that people who are active in the review process all
> get it right, but I want to make sure who read the code later can also
> understand it and reuse the reasoning in their code. Hence this
> improvement.
> 
> Tomo, feel free to fold it in your patch if you and others think the
> wording is fine.
> 
>  rust/kernel/net/phy.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index d9cec139324a..e3377f8f36b7 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -489,8 +489,8 @@ impl<T: Driver> Adapter<T> {
>  #[repr(transparent)]
>  pub struct DriverVTable(Opaque<bindings::phy_driver>);
>  
> -// SAFETY: `DriverVTable` has no &self methods, so immutable references to it
> -// are useless.
> +// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to
> +// share `&DriverVTable` across execution context boundries.
>  unsafe impl Sync for DriverVTable {}
>  
>  /// Creates a [`DriverVTable`] instance from [`Driver`].
> -- 
> 2.43.0
> 

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[Alice Ryhl]

On 12/11/23 20:49, Boqun Feng wrote:
> The current safety comment for impl Sync for DriverVTable has two
> problem:
> 
> * the correctness is unclear, since all types impl Any[1], therefore all
>    types have a `&self` method (Any::type_id).
> 
> * it doesn't explain why useless of immutable references can ensure the
>    safety.
> 
> Fix this by rewritting the comment.
> 
> [1]: https://doc.rust-lang.org/std/any/trait.Any.html
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

It's fine if you want to change it, but I think the current safety 
comment is good enough.

Alice

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[Boqun Feng]

On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote:
> On 12/11/23 20:49, Boqun Feng wrote:
> > The current safety comment for impl Sync for DriverVTable has two
> > problem:
> > 
> > * the correctness is unclear, since all types impl Any[1], therefore all
> >    types have a `&self` method (Any::type_id).
> > 
> > * it doesn't explain why useless of immutable references can ensure the
> >    safety.
> > 
> > Fix this by rewritting the comment.
> > 
> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> 
> It's fine if you want to change it,

Does it mean you are OK with the new version in this patch? If so...

> but I think the current safety comment is good enough.

... let's change it since the current version doesn't look good enough
to me as I explained above (it's not wrong, but less straight-forward to
me).

Regards,
Boqun

> 
> Alice

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[FUJITA Tomonori]

On Mon, 11 Dec 2023 15:22:54 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote:
>> On 12/11/23 20:49, Boqun Feng wrote:
>> > The current safety comment for impl Sync for DriverVTable has two
>> > problem:
>> > 
>> > * the correctness is unclear, since all types impl Any[1], therefore all
>> >    types have a `&self` method (Any::type_id).
>> > 
>> > * it doesn't explain why useless of immutable references can ensure the
>> >    safety.
>> > 
>> > Fix this by rewritting the comment.
>> > 
>> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html
>> > 
>> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> 
>> It's fine if you want to change it,
> 
> Does it mean you are OK with the new version in this patch? If so...
> 
>> but I think the current safety comment is good enough.
> 
> ... let's change it since the current version doesn't look good enough
> to me as I explained above (it's not wrong, but less straight-forward to
> me).

I'll leave this alone and wait for opinions from other reviewers since
you guys have different options. It's improvement so I don't need to
hurry.

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[Alice Ryhl]

On Tue, Dec 12, 2023 at 12:55 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Mon, 11 Dec 2023 15:22:54 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
>
> > On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote:
> >> On 12/11/23 20:49, Boqun Feng wrote:
> >> > The current safety comment for impl Sync for DriverVTable has two
> >> > problem:
> >> >
> >> > * the correctness is unclear, since all types impl Any[1], therefore all
> >> >    types have a `&self` method (Any::type_id).
> >> >
> >> > * it doesn't explain why useless of immutable references can ensure the
> >> >    safety.
> >> >
> >> > Fix this by rewritting the comment.
> >> >
> >> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html
> >> >
> >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >>
> >> It's fine if you want to change it,
> >
> > Does it mean you are OK with the new version in this patch? If so...
> >
> >> but I think the current safety comment is good enough.
> >
> > ... let's change it since the current version doesn't look good enough
> > to me as I explained above (it's not wrong, but less straight-forward to
> > me).
>
> I'll leave this alone and wait for opinions from other reviewers since
> you guys have different options. It's improvement so I don't need to
> hurry.

To clarify, the modified safety comment is also okay with me.

Alice

Re: [net-next PATCH] rust: net: phy: Correct the safety comment for impl Sync

[FUJITA Tomonori]

On Tue, 12 Dec 2023 10:17:40 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Dec 12, 2023 at 12:55 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Mon, 11 Dec 2023 15:22:54 -0800
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>> > On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote:
>> >> On 12/11/23 20:49, Boqun Feng wrote:
>> >> > The current safety comment for impl Sync for DriverVTable has two
>> >> > problem:
>> >> >
>> >> > * the correctness is unclear, since all types impl Any[1], therefore all
>> >> >    types have a `&self` method (Any::type_id).
>> >> >
>> >> > * it doesn't explain why useless of immutable references can ensure the
>> >> >    safety.
>> >> >
>> >> > Fix this by rewritting the comment.
>> >> >
>> >> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html
>> >> >
>> >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> >>
>> >> It's fine if you want to change it,
>> >
>> > Does it mean you are OK with the new version in this patch? If so...
>> >
>> >> but I think the current safety comment is good enough.
>> >
>> > ... let's change it since the current version doesn't look good enough
>> > to me as I explained above (it's not wrong, but less straight-forward to
>> > me).
>>
>> I'll leave this alone and wait for opinions from other reviewers since
>> you guys have different options. It's improvement so I don't need to
>> hurry.
> 
> To clarify, the modified safety comment is also okay with me.

Thanks for the clarification. Then I'll fold this in the patchset.

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

[Alice Ryhl]

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

Overall looks fine to me. Just a few comments below that confuse me:

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

Did you mix up two comments here? This doesn't parse in my brain.
> +    /// 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.

This sentence also doesn't parse in my brain. Perhaps "So it's just an 
FFI call" or similar?

Alice

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

[FUJITA Tomonori]

On Mon, 11 Dec 2023 22:46:01 +0100
Alice Ryhl <alice@ryhl.io> wrote:

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

Thanks.

>> +    /// 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.
> 
> Did you mix up two comments here? This doesn't parse in my brain.

I'll remove the second comment because all we have to do here is using
bindgen.


>> +    /// 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.
> 
> This sentence also doesn't parse in my brain. Perhaps "So it's just an
> FFI call" or similar?

"So it's just an FFI call" looks good. I'll fix all the places that
use the same comment.

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

[Boqun Feng]

On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
[...]
> >> +    /// 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.
> > 
> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> > FFI call" or similar?
> 
> "So it's just an FFI call" looks good. I'll fix all the places that
> use the same comment.

I would also mention that `(*phydev).mdio.addr` is smaller than
PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
comments are supposed to describe why calling the C function won't cause
memory safety issues..

Regards,
Boqun

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

[FUJITA Tomonori]

On Mon, 11 Dec 2023 15:40:33 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
> [...]
>> >> +    /// 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.
>> > 
>> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
>> > FFI call" or similar?
>> 
>> "So it's just an FFI call" looks good. I'll fix all the places that
>> use the same comment.
> 
> I would also mention that `(*phydev).mdio.addr` is smaller than
> PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
> mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
> comments are supposed to describe why calling the C function won't cause
> memory safety issues..

(*phydev).mdio.addr is managed in the C side and Rust code doesn't
touch it (doesn't need to know anything about it). What safety comment
should be written here?

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

[Boqun Feng]

On Tue, Dec 12, 2023 at 08:47:53AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 15:40:33 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> >> +    /// 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.
> >> > 
> >> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> >> > FFI call" or similar?
> >> 
> >> "So it's just an FFI call" looks good. I'll fix all the places that
> >> use the same comment.
> > 
> > I would also mention that `(*phydev).mdio.addr` is smaller than
> > PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
> > mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
> > comments are supposed to describe why calling the C function won't cause
> > memory safety issues..
> 
> (*phydev).mdio.addr is managed in the C side and Rust code doesn't

It's OK to rely on C side to give a correct addr value.

> touch it (doesn't need to know anything about it). What safety comment
> should be written here?

Basically, here Rust just does the same as C does in phy_read(), right?
So why phy_read() is implemented correctly, because C side maintains the
`(*phydev).mdio.addr` in that way. We ususally don't call it out in C
code, since it's obvious(TM), and there is no safe/unsafe boundary in C
side. But in Rust code, that matters. Yes, Rust doesn't control the
value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
words, as long as C side holds the invariants, calling mdiobus_read() is
safe here. How about 

// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also `(*phydev).mdio` is totally maintained by C in a way
// that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
// `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
// `mdiobus_read`.

?

Regards,
Boqun

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

[FUJITA Tomonori]

On Mon, 11 Dec 2023 16:49:39 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> touch it (doesn't need to know anything about it). What safety comment
>> should be written here?
> 
> Basically, here Rust just does the same as C does in phy_read(), right?
> So why phy_read() is implemented correctly, because C side maintains the
> `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> side. But in Rust code, that matters. Yes, Rust doesn't control the
> value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> words, as long as C side holds the invariants, calling mdiobus_read() is
> safe here. How about 
> 
> // SAFETY: `phydev` points to valid object per the type invariant of
> // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> // `mdiobus_read`.

I thought that "`phydev` is pointing to a valid object by the type
invariant of `Self`." comment implies that "C side holds the invariants"

Do we need a comment about the C implementation details like
PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
side because the C code is changed any time.

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

[Boqun Feng]

On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 16:49:39 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> >> touch it (doesn't need to know anything about it). What safety comment
> >> should be written here?
> > 
> > Basically, here Rust just does the same as C does in phy_read(), right?
> > So why phy_read() is implemented correctly, because C side maintains the
> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> > side. But in Rust code, that matters. Yes, Rust doesn't control the
> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> > words, as long as C side holds the invariants, calling mdiobus_read() is
> > safe here. How about 
> > 
> > // SAFETY: `phydev` points to valid object per the type invariant of
> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> > // `mdiobus_read`.
> 
> I thought that "`phydev` is pointing to a valid object by the type
> invariant of `Self`." comment implies that "C side holds the invariants"
> 

By the type invariant of `Self`, you mean:

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

my read on that only tells me the context is guaranteed to be in a
driver callback, nothing has been said about all other invariants C side
should hold.

> Do we need a comment about the C implementation details like
> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
> side because the C code is changed any time.

Well, exactly, "the C code is changed any time", I thought having more
information in Rust helps people who is going to change the C side to
see whether they may break Rust side. Plus it's the safety comment, you
need to prove that it's safe to call the function, the function is
unsafe for a reason: there are inputs that may cause issues, and writing
the safety comment is a process to think and double check.

Maybe we can simplify this a little bit, since IIUC, you just want to
call phy_read() here, but due to that Rust cannot call inline C
functions directly, hence the open-code. How about:


// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also the following just minics what `phy_read()` does in C
// side, which should be safe as long as `phydev` is valid.

?

Regards,
Boqun

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

[FUJITA Tomonori]

On Mon, 11 Dec 2023 18:30:36 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
>> On Mon, 11 Dec 2023 16:49:39 -0800
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> >> touch it (doesn't need to know anything about it). What safety comment
>> >> should be written here?
>> > 
>> > Basically, here Rust just does the same as C does in phy_read(), right?
>> > So why phy_read() is implemented correctly, because C side maintains the
>> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
>> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
>> > side. But in Rust code, that matters. Yes, Rust doesn't control the
>> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
>> > words, as long as C side holds the invariants, calling mdiobus_read() is
>> > safe here. How about 
>> > 
>> > // SAFETY: `phydev` points to valid object per the type invariant of
>> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
>> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
>> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
>> > // `mdiobus_read`.
>> 
>> I thought that "`phydev` is pointing to a valid object by the type
>> invariant of `Self`." comment implies that "C side holds the invariants"
>> 
> 
> By the type invariant of `Self`, you mean:
> 
> /// # 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.
> 
> my read on that only tells me the context is guaranteed to be in a
> driver callback, nothing has been said about all other invariants C side
> should hold.

I meant that phydev points to a valid object, thus mdio and mdio.addr
do too. But after reading you phy_read() comment, I suspect that you
aren't talking about if mdio.addr points to valid memory or not. Your
point is about the validity of calling mdiobus_read() with mdio.addr?


>> Do we need a comment about the C implementation details like
>> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
>> side because the C code is changed any time.
> 
> Well, exactly, "the C code is changed any time", I thought having more
> information in Rust helps people who is going to change the C side to
> see whether they may break Rust side. Plus it's the safety comment, you

The C side people read the Rust code before changing the C code? Let's
see. 


> need to prove that it's safe to call the function, the function is
> unsafe for a reason: there are inputs that may cause issues, and writing
> the safety comment is a process to think and double check.
> 
> Maybe we can simplify this a little bit, since IIUC, you just want to
> call phy_read() here, but due to that Rust cannot call inline C
> functions directly, hence the open-code. How about:

Yeah, I hope that the discussion about inline C functions would end
with a solution.


> // SAFETY: `phydev` points to valid object per the type invariant of
> // `Self`, also the following just minics what `phy_read()` does in C
> // side, which should be safe as long as `phydev` is valid.
> 
> ?

Looks ok to me but after a quick look at in-tree Rust code, I can't
find a comment like X is valid for the first argument in this C
function. What I found are comments like X points to valid memory.

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

[Boqun Feng]

On Tue, Dec 12, 2023 at 01:04:10PM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 18:30:36 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
> >> On Mon, 11 Dec 2023 16:49:39 -0800
> >> Boqun Feng <boqun.feng@gmail.com> wrote:
> >> 
> >> >> touch it (doesn't need to know anything about it). What safety comment
> >> >> should be written here?
> >> > 
> >> > Basically, here Rust just does the same as C does in phy_read(), right?
> >> > So why phy_read() is implemented correctly, because C side maintains the
> >> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> >> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> >> > side. But in Rust code, that matters. Yes, Rust doesn't control the
> >> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> >> > words, as long as C side holds the invariants, calling mdiobus_read() is
> >> > safe here. How about 
> >> > 
> >> > // SAFETY: `phydev` points to valid object per the type invariant of
> >> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> >> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> >> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> >> > // `mdiobus_read`.
> >> 
> >> I thought that "`phydev` is pointing to a valid object by the type
> >> invariant of `Self`." comment implies that "C side holds the invariants"
> >> 
> > 
> > By the type invariant of `Self`, you mean:
> > 
> > /// # 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.
> > 
> > my read on that only tells me the context is guaranteed to be in a
> > driver callback, nothing has been said about all other invariants C side
> > should hold.
> 
> I meant that phydev points to a valid object, thus mdio and mdio.addr
> do too. But after reading you phy_read() comment, I suspect that you
> aren't talking about if mdio.addr points to valid memory or not. Your
> point is about the validity of calling mdiobus_read() with mdio.addr?
> 

Yes, I was talking about the safety comment for calling mdiobus_read().

> 
> >> Do we need a comment about the C implementation details like
> >> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
> >> side because the C code is changed any time.
> > 
> > Well, exactly, "the C code is changed any time", I thought having more
> > information in Rust helps people who is going to change the C side to
> > see whether they may break Rust side. Plus it's the safety comment, you
> 
> The C side people read the Rust code before changing the C code? Let's
> see. 
> 

Hmm... I usually won't call someone "C side people". I mean, the project
has C part and Rust part, but the community is one.

In case of myself, I write both C and Rust, if I'm going to change some
C side function, I may want to see the usage at Rust side, especially
whether my changes could break the safety, and safety comments may be
important.

> 
> > need to prove that it's safe to call the function, the function is
> > unsafe for a reason: there are inputs that may cause issues, and writing
> > the safety comment is a process to think and double check.
> > 
> > Maybe we can simplify this a little bit, since IIUC, you just want to
> > call phy_read() here, but due to that Rust cannot call inline C
> > functions directly, hence the open-code. How about:
> 
> Yeah, I hope that the discussion about inline C functions would end
> with a solution.
> 
> 
> > // SAFETY: `phydev` points to valid object per the type invariant of
> > // `Self`, also the following just minics what `phy_read()` does in C
> > // side, which should be safe as long as `phydev` is valid.
> > 
> > ?
> 
> Looks ok to me but after a quick look at in-tree Rust code, I can't
> find a comment like X is valid for the first argument in this C
> function. What I found are comments like X points to valid memory.

Hmm.. maybe "is valid" could be a confusing term, so the point is: if
`phydev` is pointing to a properly maintained struct phy_device, then an
open code of phy_read() should be safe. Maybe "..., which should be safe
as long as `phydev` points to a valid struct phy_device" ?

Regards,
Boqun

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

[FUJITA Tomonori]

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

>> > // SAFETY: `phydev` points to valid object per the type invariant of
>> > // `Self`, also the following just minics what `phy_read()` does in C
>> > // side, which should be safe as long as `phydev` is valid.
>> > 
>> > ?
>> 
>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>> find a comment like X is valid for the first argument in this C
>> function. What I found are comments like X points to valid memory.
> 
> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
> `phydev` is pointing to a properly maintained struct phy_device, then an
> open code of phy_read() should be safe. Maybe "..., which should be safe
> as long as `phydev` points to a valid struct phy_device" ?

As Alice suggested, I updated the comment. The current comment is:

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call.
let ret = unsafe {
    bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
};

If phy_read() is called here, I assume that you are happy about the
above comment. The way to call mdiobus_read() here is safe because it
just an open code of phy_read(). Simply adding it works for you?

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call, open code of `phy_read()`.

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

[Benno Lossin]

On 12/12/23 14:02, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 22:11:15 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>> // side, which should be safe as long as `phydev` is valid.
>>>>
>>>> ?
>>>
>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>> find a comment like X is valid for the first argument in this C
>>> function. What I found are comments like X points to valid memory.
>>
>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>> `phydev` is pointing to a properly maintained struct phy_device, then an
>> open code of phy_read() should be safe. Maybe "..., which should be safe
>> as long as `phydev` points to a valid struct phy_device" ?
> 
> As Alice suggested, I updated the comment. The current comment is:
> 
> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> // So it's just an FFI call.
> let ret = unsafe {
>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> };

I still think you need to justify why `mdio.bus` is a pointer that you
can give to `midobus_read`. After looking at the C code, it seems like
that the pointer needs to point to a valid `struct mii_bus`.
This *could* just be an invariant of `struct phy_device` [1], but where
do we document that?

We could make an exception here and treat this differently until bindgen
can handle the `static inline` functions, but I am not so sure if we
want to have this as a general pattern. We need to discuss this more.


[1]: Technically it is a combination of the following invariants:
- the `mdio` field of `struct phy_device` is a valid `struct mido_device`
- the `bus` field of `struct mdio_device` is a valid pointer to a valid
  `struct mii_bus`.

> If phy_read() is called here, I assume that you are happy about the
> above comment. The way to call mdiobus_read() here is safe because it
> just an open code of phy_read(). Simply adding it works for you?
> 
> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> // So it's just an FFI call, open code of `phy_read()`.

This would be fine if we decide to go with the exception I detailed
above. Although instead of "open code" I would write "see implementation
of `phy_read()`".

-- 
Cheers,
Benno

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

[Boqun Feng]

On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
> On 12/12/23 14:02, FUJITA Tomonori wrote:
> > On Mon, 11 Dec 2023 22:11:15 -0800
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> >>>> // SAFETY: `phydev` points to valid object per the type invariant of
> >>>> // `Self`, also the following just minics what `phy_read()` does in C
> >>>> // side, which should be safe as long as `phydev` is valid.
> >>>>
> >>>> ?
> >>>
> >>> Looks ok to me but after a quick look at in-tree Rust code, I can't
> >>> find a comment like X is valid for the first argument in this C
> >>> function. What I found are comments like X points to valid memory.
> >>
> >> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
> >> `phydev` is pointing to a properly maintained struct phy_device, then an
> >> open code of phy_read() should be safe. Maybe "..., which should be safe
> >> as long as `phydev` points to a valid struct phy_device" ?
> > 
> > As Alice suggested, I updated the comment. The current comment is:
> > 
> > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > // So it's just an FFI call.
> > let ret = unsafe {
> >     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> > };
> 
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`. After looking at the C code, it seems like
> that the pointer needs to point to a valid `struct mii_bus`.
> This *could* just be an invariant of `struct phy_device` [1], but where
> do we document that?

Yeah, it's better if we call it out in the type invariant.

> 
> We could make an exception here and treat this differently until bindgen
> can handle the `static inline` functions, but I am not so sure if we
> want to have this as a general pattern. We need to discuss this more.
> 

Agreed, here my latest suggestion was definitely a workaround.

> 
> [1]: Technically it is a combination of the following invariants:
> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>   `struct mii_bus`.
> 
> > If phy_read() is called here, I assume that you are happy about the
> > above comment. The way to call mdiobus_read() here is safe because it
> > just an open code of phy_read(). Simply adding it works for you?
> > 
> > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > // So it's just an FFI call, open code of `phy_read()`.
> 
> This would be fine if we decide to go with the exception I detailed
> above. Although instead of "open code" I would write "see implementation
> of `phy_read()`".
> 

So the rationale here is the callsite of mdiobus_read() is just a
open-code version of phy_read(), so if we meet the same requirement of
phy_read(), we should be safe here. Maybe:

	"... open code of `phy_read()` with a valid phy_device pointer
	`phydev`"

?

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 

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

[Benno Lossin]

On 12/12/23 21:23, Boqun Feng wrote:
> On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
>> [1]: Technically it is a combination of the following invariants:
>> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
>> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>>   `struct mii_bus`.
>>
>>> If phy_read() is called here, I assume that you are happy about the
>>> above comment. The way to call mdiobus_read() here is safe because it
>>> just an open code of phy_read(). Simply adding it works for you?
>>>
>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> // So it's just an FFI call, open code of `phy_read()`.
>>
>> This would be fine if we decide to go with the exception I detailed
>> above. Although instead of "open code" I would write "see implementation
>> of `phy_read()`".
>>
> 
> So the rationale here is the callsite of mdiobus_read() is just a
> open-code version of phy_read(), so if we meet the same requirement of
> phy_read(), we should be safe here. Maybe:
> 
> 	"... open code of `phy_read()` with a valid phy_device pointer
> 	`phydev`"
> 
> ?

Hmm that might be OK if we add "TODO: replace this with `phy_read` once
bindgen can handle static inline functions.".

Actually, why can't we just use the normal `rust_helper_*` approach? So
just create a `rust_helper_phy_read` that calls `phy_read`. Then call
that from the rust side. Doing this means that we can just keep the
invariants of `struct phy_device` opaque to the Rust side.
That would probably be preferable to adding the `TODO`, since when
bindgen has this feature available, we will automatically handle this
and not forget it. Also we have no issue with diverging code.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Tue, 12 Dec 2023 22:40:01 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 12/12/23 21:23, Boqun Feng wrote:
>> On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
>>> [1]: Technically it is a combination of the following invariants:
>>> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
>>> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>>>   `struct mii_bus`.
>>>
>>>> If phy_read() is called here, I assume that you are happy about the
>>>> above comment. The way to call mdiobus_read() here is safe because it
>>>> just an open code of phy_read(). Simply adding it works for you?
>>>>
>>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>>> // So it's just an FFI call, open code of `phy_read()`.
>>>
>>> This would be fine if we decide to go with the exception I detailed
>>> above. Although instead of "open code" I would write "see implementation
>>> of `phy_read()`".
>>>
>> 
>> So the rationale here is the callsite of mdiobus_read() is just a
>> open-code version of phy_read(), so if we meet the same requirement of
>> phy_read(), we should be safe here. Maybe:
>> 
>> 	"... open code of `phy_read()` with a valid phy_device pointer
>> 	`phydev`"
>> 
>> ?
> 
> Hmm that might be OK if we add "TODO: replace this with `phy_read` once
> bindgen can handle static inline functions.".

That's the conclusion? I suppose that a way to handle static inline
functions is still under discussion.


> Actually, why can't we just use the normal `rust_helper_*` approach? So
> just create a `rust_helper_phy_read` that calls `phy_read`. Then call
> that from the rust side. Doing this means that we can just keep the
> invariants of `struct phy_device` opaque to the Rust side.
> That would probably be preferable to adding the `TODO`, since when
> bindgen has this feature available, we will automatically handle this
> and not forget it. Also we have no issue with diverging code.

I wasn't sure that `rust_helper_*` approach is the way to go.

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

[Benno Lossin]

On 13.12.23 00:27, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 22:40:01 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> Actually, why can't we just use the normal `rust_helper_*` approach? So
>> just create a `rust_helper_phy_read` that calls `phy_read`. Then call
>> that from the rust side. Doing this means that we can just keep the
>> invariants of `struct phy_device` opaque to the Rust side.
>> That would probably be preferable to adding the `TODO`, since when
>> bindgen has this feature available, we will automatically handle this
>> and not forget it. Also we have no issue with diverging code.
> 
> I wasn't sure that `rust_helper_*` approach is the way to go.

It is the way to go to avoid code duplication and call those
functions.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Tue, 12 Dec 2023 12:23:04 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> So the rationale here is the callsite of mdiobus_read() is just a
> open-code version of phy_read(), so if we meet the same requirement of
> phy_read(), we should be safe here. Maybe:
> 
> 	"... open code of `phy_read()` with a valid phy_device pointer
> 	`phydev`"
> 
> ?

I'll add the above comment with the similar for phy_write(), thanks!

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

[Benno Lossin]

On 13.12.23 00:31, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 12:23:04 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> So the rationale here is the callsite of mdiobus_read() is just a
>> open-code version of phy_read(), so if we meet the same requirement of
>> phy_read(), we should be safe here. Maybe:
>>
>> 	"... open code of `phy_read()` with a valid phy_device pointer
>> 	`phydev`"
>>
>> ?
> 
> I'll add the above comment with the similar for phy_write(), thanks!

Why don't you use the `rust_helper` approach?

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Tue, 12 Dec 2023 17:35:34 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 12/12/23 14:02, FUJITA Tomonori wrote:
>> On Mon, 11 Dec 2023 22:11:15 -0800
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>>> // side, which should be safe as long as `phydev` is valid.
>>>>>
>>>>> ?
>>>>
>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>>> find a comment like X is valid for the first argument in this C
>>>> function. What I found are comments like X points to valid memory.
>>>
>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>>> `phydev` is pointing to a properly maintained struct phy_device, then an
>>> open code of phy_read() should be safe. Maybe "..., which should be safe
>>> as long as `phydev` points to a valid struct phy_device" ?
>> 
>> As Alice suggested, I updated the comment. The current comment is:
>> 
>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> // So it's just an FFI call.
>> let ret = unsafe {
>>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>> };
> 
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`. After looking at the C code, it seems like
> that the pointer needs to point to a valid `struct mii_bus`.
> This *could* just be an invariant of `struct phy_device` [1], but where
> do we document that?

If phy_device points to a valid object, phy_device.mdio is valid.

A mii_bus must exist before a phy device. A bus is scanned and then a
phy device is found (so phy_device object is crated).

https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634

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

[Benno Lossin]

On 12/13/23 00:01, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 17:35:34 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 12/12/23 14:02, FUJITA Tomonori wrote:
>>> On Mon, 11 Dec 2023 22:11:15 -0800
>>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>>
>>>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>>>> // side, which should be safe as long as `phydev` is valid.
>>>>>>
>>>>>> ?
>>>>>
>>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>>>> find a comment like X is valid for the first argument in this C
>>>>> function. What I found are comments like X points to valid memory.
>>>>
>>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>>>> `phydev` is pointing to a properly maintained struct phy_device, then an
>>>> open code of phy_read() should be safe. Maybe "..., which should be safe
>>>> as long as `phydev` points to a valid struct phy_device" ?
>>>
>>> As Alice suggested, I updated the comment. The current comment is:
>>>
>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> // So it's just an FFI call.
>>> let ret = unsafe {
>>>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>> };
>>
>> I still think you need to justify why `mdio.bus` is a pointer that you
>> can give to `midobus_read`. After looking at the C code, it seems like
>> that the pointer needs to point to a valid `struct mii_bus`.
>> This *could* just be an invariant of `struct phy_device` [1], but where
>> do we document that?
> 
> If phy_device points to a valid object, phy_device.mdio is valid.

Where is this documented?

> A mii_bus must exist before a phy device. A bus is scanned and then a
> phy device is found (so phy_device object is crated).
> 
> https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634

I know that this is the status. But for this to be useful as a
justification it must be written down somewhere where you can expect to
find it. Not some knowledge that "everyone just knows".
I would prefer to use the solution that I detailed in the other thread.

-- 
Cheers,
Benno

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

[Andrew Lunn]

> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`.

Maybe a dumb question. Why are you limiting this to just a few members
of struct phy_device? It has ~50 members, any of which can be used by
the C side when Rust calls into C.

    Andrew

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

[Benno Lossin]

On 12/13/23 11:28, Andrew Lunn wrote:
>> I still think you need to justify why `mdio.bus` is a pointer that you
>> can give to `midobus_read`.
> 
> Maybe a dumb question. Why are you limiting this to just a few members
> of struct phy_device? It has ~50 members, any of which can be used by
> the C side when Rust calls into C.

I limited it to those few members, because the Rust side only uses
those.
Theoretically one could specify all invariants for all members, but that
seems like a *lot* of work.

In Rust everything [1] has to be initialized with a valid value.
Contrast this with C where everything is potentially uninitialized. As
an example, let's look at the first few fields of `PhyDevice`:

    struct PhyDevice {
        mdio: MdioDevice,
        drv: Box<PhyDriver>,
        devlink: Box<DeviceLink>,
        phy_id: u32,
        c45_ids: PhyC45DeviceIds,
        // ...
    }

Note that in Rust we now do not need to write down any invariants, since
they are all implicitly encoded. For example the `drv` field is of type
`Box<PhyDriver>`, it *always* is a pointer to an allocation that
contains a valid `PhyDriver`. So while on the C side you would have to
state this, on the Rust side we get it for free.

Rust function also make use of the "everything is in a valid state"
rule, they know that the fields are valid and thus the Rust equivalent
of `phy_read` could be safe and without any comments:

    impl PhyDevice {
        fn phy_read(&self, regnum: u32) -> i32 {
            self.mdio.bus.mdiobus_read(self.mdio.addr, regnum)
        }
    }

All of this only applies to safe code, `unsafe` code is allowed to
violate all of these things temporarily. However, when it gives a value
back to safe code [2], that value needs to be valid.

I think that specifying all of these implicit invariants in C will be
extremely laborious. Especially since the usual way of doing things in C
is not considering these invariants (at least not consciously), but
rather "just do the thing".

The way we do the interoperability is to just fully trust the C side to
produce valid values that we can feed back to the C side. Of course
there are caveats, so e.g. one needs to initialize a `struct mutex`
before it can be used, but that is what we need to capture with the
safety comments.

[1]: There are exceptions for this, but for the purposes of this
     discussion, they can be ignored. If you want to know more, you can
     read this article in the nomicon:
     https://doc.rust-lang.org/nomicon/unchecked-uninit.html

[2]: There are also exceptions, but I will omit them here.

-- 
Cheers,
Benno

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

[Andrew Lunn]

> > The C side people read the Rust code before changing the C code? Let's
> > see. 
> > 
> 
> Hmm... I usually won't call someone "C side people". I mean, the project
> has C part and Rust part, but the community is one.
> 
> In case of myself, I write both C and Rust, if I'm going to change some
> C side function, I may want to see the usage at Rust side, especially
> whether my changes could break the safety, and safety comments may be
> important.

While i agree with your sentiment, ideally we want bilingual
developers, in reality that is not going to happen for a long time. I
could be wrong, but i expect developers to be either C developers, or
Rust developers. They are existing kernel developers who know C, or
Rust developers who are new to the kernel, and may not know much C. So
we should try to keep that in mind.

I personally don't think i have enough Rust knowledge to of even
reached the dangerous stage. But at least the hard part with Rust
seems to be the comments, not the actual code :-(

	Andrew

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

[Boqun Feng]

On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote:
> > > The C side people read the Rust code before changing the C code? Let's
> > > see. 
> > > 
> > 
> > Hmm... I usually won't call someone "C side people". I mean, the project
> > has C part and Rust part, but the community is one.
> > 
> > In case of myself, I write both C and Rust, if I'm going to change some
> > C side function, I may want to see the usage at Rust side, especially
> > whether my changes could break the safety, and safety comments may be
> > important.
> 
> While i agree with your sentiment, ideally we want bilingual
> developers, in reality that is not going to happen for a long time. I
> could be wrong, but i expect developers to be either C developers, or
> Rust developers. They are existing kernel developers who know C, or
> Rust developers who are new to the kernel, and may not know much C. So

Sorry, I cannot agree with you. Why do we try to divide the community in
two parts? In fact, I keep telling people who want to contribute
Rust-for-Linux that one way to contribute is trying to do some C code
changes first to get familiar with the subsystem and kernel development.

The sentence from Tomo really read like: I don't want to put this
information here, since I don't think anyone would use it. Why do we
want to shutdown the door for more people to collaborate, really, why?
The only downside here is that Tomo needs to maintain a few more lines
of comments. Also the comment is not a random comment, it's the safety
comment, please see below..

> we should try to keep that in mind.
> 
> I personally don't think i have enough Rust knowledge to of even
> reached the dangerous stage. But at least the hard part with Rust
> seems to be the comments, not the actual code :-(
> 

Well, a safety comment is a basic part of Rust, which identifies the
safe/unsafe boundary (i.e. where the code could go wrong in memory
safety) and without that, the code will be just using Rust syntax and
grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
boundaries, why do they try to use Rust? Unsafe Rust is harder to write
than C, and safe Rust is pointless without a clear safe/unsafe boundary.
Plus the syntax is not liked by anyone last time I heard ;-)

Having a correct safety comment is really the bottom line. Without that,
it's just bad Rust code, which I don't think netdev doesn't want either?
Am I missing something here?

Regards,
Boqun

> 	Andrew
> 

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

[Boqun Feng]

On Wed, Dec 13, 2023 at 08:43:09AM -0800, Boqun Feng wrote:
> On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote:
> > > > The C side people read the Rust code before changing the C code? Let's
> > > > see. 
> > > > 
> > > 
> > > Hmm... I usually won't call someone "C side people". I mean, the project
> > > has C part and Rust part, but the community is one.
> > > 
> > > In case of myself, I write both C and Rust, if I'm going to change some
> > > C side function, I may want to see the usage at Rust side, especially
> > > whether my changes could break the safety, and safety comments may be
> > > important.
> > 
> > While i agree with your sentiment, ideally we want bilingual
> > developers, in reality that is not going to happen for a long time. I
> > could be wrong, but i expect developers to be either C developers, or
> > Rust developers. They are existing kernel developers who know C, or
> > Rust developers who are new to the kernel, and may not know much C. So
> 
> Sorry, I cannot agree with you. Why do we try to divide the community in
> two parts? In fact, I keep telling people who want to contribute
> Rust-for-Linux that one way to contribute is trying to do some C code
> changes first to get familiar with the subsystem and kernel development.
> 
> The sentence from Tomo really read like: I don't want to put this
> information here, since I don't think anyone would use it. Why do we
> want to shutdown the door for more people to collaborate, really, why?
> The only downside here is that Tomo needs to maintain a few more lines
> of comments. Also the comment is not a random comment, it's the safety
> comment, please see below..
> 
> > we should try to keep that in mind.
> > 
> > I personally don't think i have enough Rust knowledge to of even
> > reached the dangerous stage. But at least the hard part with Rust
> > seems to be the comments, not the actual code :-(
> > 
> 
> Well, a safety comment is a basic part of Rust, which identifies the
> safe/unsafe boundary (i.e. where the code could go wrong in memory
> safety) and without that, the code will be just using Rust syntax and
> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Having a correct safety comment is really the bottom line. Without that,
> it's just bad Rust code, which I don't think netdev doesn't want either?

s/doesn't//

> Am I missing something here?
> 
> Regards,
> Boqun
> 
> > 	Andrew
> > 

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

[Andrew Lunn]

> Well, a safety comment is a basic part of Rust, which identifies the
> safe/unsafe boundary (i.e. where the code could go wrong in memory
> safety) and without that, the code will be just using Rust syntax and
> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> Plus the syntax is not liked by anyone last time I heard ;-)

Maybe comments are the wrong format for this? Maybe it should be a
formal language? It could then be compiled into an executable form and
tested? It won't show it is complete, but it would at least show it is
correct/incorrect description of the assumptions. For normal builds it
would not be included in the final binary, but maybe debug or formal
verification builds it would be included?

> Having a correct safety comment is really the bottom line. Without that,
> it's just bad Rust code, which I don't think netdev doesn't want either?
> Am I missing something here?

It seems much easier to agree actual code is correct, maybe because it
is a formal language, with a compiler, and a method to test it. Is
that code really bad without the comments? It would be interesting to
look back and see how much the actual code has changed because of
these comments? I _think_ most of the review comments have resulted in
changes to the comments, not the executable code itself. Does that
mean it is much harder to write correct comments than correct code?

       Andrew

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

[Benno Lossin]

On 12/13/23 22:48, Andrew Lunn wrote:
>> Well, a safety comment is a basic part of Rust, which identifies the
>> safe/unsafe boundary (i.e. where the code could go wrong in memory
>> safety) and without that, the code will be just using Rust syntax and
>> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
>> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
>> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
>> Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Maybe comments are the wrong format for this? Maybe it should be a
> formal language? It could then be compiled into an executable form and
> tested? It won't show it is complete, but it would at least show it is
> correct/incorrect description of the assumptions. For normal builds it
> would not be included in the final binary, but maybe debug or formal
> verification builds it would be included?

That is an interesting suggestion, do you have any specific tools in
mind?
There are some Rust tools for formal verification, see
https://rust-formal-methods.github.io/tools.html
but I don't know if they can be used in the kernel, especially since we
would need a tool that also supports C (I have no experience/knowledge
of verification tools for C, so maybe you have something).
Also my experience tells me that there are several issues with formal
verification in practice:

1. When you want to use some formal system to prove something it is
   often an "all or nothing" game. So you will have to first verify
   everything that lies beneath you, or assume that it is correctly
   implemented. But assuming that everything is correctly implemented is
   rather dangerous, because if you base your formal system on classical
   logic [1], then a single contradiction allows you to prove
   everything. So in order for you to be _sure_ that it is correct, you
   need to work from the ground up.

2. There is no formal Rust memory model. So proving anything for
   interoperability between Rust and C is going to be challenging.

3. The burden of fully verifying a program is great. I know this, as I
   have some experience in this field. Now the programmer not only needs
   to know how to write a piece of code, but also how to write the
   required statements in the formal system and most importantly how to
   prove said statements from the axioms and theorems.


When using safety comments, we avoid the problems of having to prove the
statements formally (which is _very_ difficult). Of course people still
need to know how to write safety comments, which is why I am working on
a standard for safety comments. I hope to post an RFC in a couple weeks.
It will also make the safety comments more formal by having a fixed
set of phrases with exact interpretations, so there can be less room for
misunderstandings.


[2]: You might try to work around this by using a paraconsistent logic,
     but I have little to no experience with that field, so I cannot
     really say more than "it exists".


>> Having a correct safety comment is really the bottom line. Without that,
>> it's just bad Rust code, which I don't think netdev doesn't want either?
>> Am I missing something here?
> 
> It seems much easier to agree actual code is correct, maybe because it
> is a formal language, with a compiler, and a method to test it. Is

I disagree. You always have to consider the entire kernel when you want
to determine if some piece of code is correct. This is by the nature of
the formal language, anything can affect anything.
For example you consider this:

    foo(void **ptr) {
        *ptr = NULL;
        synchronize_rcu()
        print(**x)
    }

How do you know that this is correct? Well you have to look at all
locations where `foo` is invoked and see if after a `synchronize_rcu`
the supplied pointer is valid.

If we do the safety comment stuff correctly, then we have a _local_
property, so something where you do not have to consider the entire
kernel. Instead you assume that all other safety comments are correct
and then only verify the boundary. If all boundaries agree, we have a
reasonably correct program.

> that code really bad without the comments? It would be interesting to
> look back and see how much the actual code has changed because of
> these comments? I _think_ most of the review comments have resulted in
> changes to the comments, not the executable code itself. Does that
> mean it is much harder to write correct comments than correct code?

The code not having changed does not mean that it is correct. There are
no obvious issues present, but can we really know that it is correct?
Only time will tell (or a formal verification).

The issue that we currently have with this patch series is that the
people who know how the stuff works and the people who know how to
write good safety comments are not the same. I hope that my safety
standard will help close this gap.

For example we do not know how the synchronization mechanism for
`phy_suspend` and `phy_resume` work, but you mentioned in some previous
thread that the knowledge is actually somewhere out there. It would help
us a lot if you could give us a link or an explanation. Then we can work
on a suitable safety comment.

-- 
Cheers,
Benno

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

[Benno Lossin]

On 12/14/23 00:40, Benno Lossin wrote:
> On 12/13/23 22:48, Andrew Lunn wrote:
>>> Well, a safety comment is a basic part of Rust, which identifies the
>>> safe/unsafe boundary (i.e. where the code could go wrong in memory
>>> safety) and without that, the code will be just using Rust syntax and
>>> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
>>> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
>>> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
>>> Plus the syntax is not liked by anyone last time I heard ;-)
>>
>> Maybe comments are the wrong format for this? Maybe it should be a
>> formal language? It could then be compiled into an executable form and
>> tested? It won't show it is complete, but it would at least show it is
>> correct/incorrect description of the assumptions. For normal builds it
>> would not be included in the final binary, but maybe debug or formal
>> verification builds it would be included?
> 
> That is an interesting suggestion, do you have any specific tools in
> mind?
> There are some Rust tools for formal verification, see
> https://rust-formal-methods.github.io/tools.html
> but I don't know if they can be used in the kernel, especially since we
> would need a tool that also supports C (I have no experience/knowledge
> of verification tools for C, so maybe you have something).
> Also my experience tells me that there are several issues with formal
> verification in practice:

Don't get me wrong, I would welcome a more formalized approach. I just
have seen what that entails and I believe Rust (with safety comments)
to be a good compromise that still allows programmers with no knowledge
in formal systems to program and reasonable correctness.

> 1. When you want to use some formal system to prove something it is
>    often an "all or nothing" game. So you will have to first verify
>    everything that lies beneath you, or assume that it is correctly
>    implemented. But assuming that everything is correctly implemented is
>    rather dangerous, because if you base your formal system on classical
>    logic [1], then a single contradiction allows you to prove
>    everything. So in order for you to be _sure_ that it is correct, you
>    need to work from the ground up.
> 
> 2. There is no formal Rust memory model. So proving anything for
>    interoperability between Rust and C is going to be challenging.
> 
> 3. The burden of fully verifying a program is great. I know this, as I
>    have some experience in this field. Now the programmer not only needs
>    to know how to write a piece of code, but also how to write the
>    required statements in the formal system and most importantly how to
>    prove said statements from the axioms and theorems.
> 
> 
> When using safety comments, we avoid the problems of having to prove the
> statements formally (which is _very_ difficult). Of course people still
> need to know how to write safety comments, which is why I am working on
> a standard for safety comments. I hope to post an RFC in a couple weeks.
> It will also make the safety comments more formal by having a fixed
> set of phrases with exact interpretations, so there can be less room for
> misunderstandings.
> 
> 
> [2]: You might try to work around this by using a paraconsistent logic,

This should actually be [1]:.

>      but I have little to no experience with that field, so I cannot
>      really say more than "it exists".

-- 
Cheers,
Benno

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

[Andrew Lunn]

On Wed, Dec 13, 2023 at 11:40:26PM +0000, Benno Lossin wrote:
> On 12/13/23 22:48, Andrew Lunn wrote:
> >> Well, a safety comment is a basic part of Rust, which identifies the
> >> safe/unsafe boundary (i.e. where the code could go wrong in memory
> >> safety) and without that, the code will be just using Rust syntax and
> >> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> >> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> >> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> >> Plus the syntax is not liked by anyone last time I heard ;-)
> > 
> > Maybe comments are the wrong format for this? Maybe it should be a
> > formal language? It could then be compiled into an executable form and
> > tested? It won't show it is complete, but it would at least show it is
> > correct/incorrect description of the assumptions. For normal builds it
> > would not be included in the final binary, but maybe debug or formal
> > verification builds it would be included?
> 
> That is an interesting suggestion, do you have any specific tools in
> mind?

Sorry, no. I've no experience in this field at all. But given the
discussions this patch has caused, simply a list of C or Rust
expressions which evaluate to True when an assumption is correct would
be a good start.

We have said that we assume the phydev->lock is held. That is easy to
express in code. We have said that phydev->mdio must be set, which is
again easy to express. phydev->mdio.addr must be in the range
0..PHY_MAX_ADDR, etc.

You probably cannot express all the safety requirements this way, but
the set you can describe should be easy to understand and also
unambiguous, since it is code. The rest still can be as comments.  It
would be easy to compile this code and insert it before the function
on a verification build. Its only runtime checking, but its more
functional than comments which the compiler just throws away. And
maybe subsystems like this which are pretty much always slow path
might even leave them enabled all the time, to act as a set of
assert()s, which you sometimes see in code bases.

	Andrew

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

[Boqun Feng]

On Wed, Dec 13, 2023 at 10:48:39PM +0100, Andrew Lunn wrote:
> > Well, a safety comment is a basic part of Rust, which identifies the
> > safe/unsafe boundary (i.e. where the code could go wrong in memory
> > safety) and without that, the code will be just using Rust syntax and
> > grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> > boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> > than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> > Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Maybe comments are the wrong format for this? Maybe it should be a

Maybe, but they are what we have right now. I do believe the unsafe <->
safe boundary in Rust needs better tooling: using comments may be a
little bit abitrary. However, let's stick to what we have right now.
Benno is actually working our Rust safety standard (feel free to correct
me if I get the name wrong), hopefully that helps.

> formal language? It could then be compiled into an executable form and
> tested? It won't show it is complete, but it would at least show it is
> correct/incorrect description of the assumptions. For normal builds it
> would not be included in the final binary, but maybe debug or formal
> verification builds it would be included?
> 

Good idea, we actually want something similar when we were talking about
Benno's safety standard, but as you may know a complete one would take
years, or maybe impossbile. Also, kernel's safety requirement sometimes
is weird and non-trivial, it's going to be a learning process ;-) A tool
is certainly what we would like to look into if we have more experience
in Rust abstraction and more time.

> > Having a correct safety comment is really the bottom line. Without that,
> > it's just bad Rust code, which I don't think netdev doesn't want either?
> > Am I missing something here?
> 
> It seems much easier to agree actual code is correct, maybe because it
> is a formal language, with a compiler, and a method to test it. Is
> that code really bad without the comments? It would be interesting to

Note that most of the comment reviews are on *safety* comments (type
invariants are related to safety comments). To me, in the review
process, safety comments are similar to commit logs. In kernel patch
reviews, we sometimes say "thanks for the explanation but please put
that in the commit log", that is, even we know the code is correct and
the patchset has been explained, we still want to see the explanation
written down somewhere. Commit logs are ways to explain "why do I want
to do this", and safety comments are ways to explain "why can I do the
following safely". They are both ways to communicate during the review
and for the future readers.

So is a patchset really bad without commit logs? ;-)

> look back and see how much the actual code has changed because of
> these comments? I _think_ most of the review comments have resulted in

I was trying to figure out the answer, but the patchset just has too
many versions.. and discussions were splitted between versions...

> changes to the comments, not the executable code itself. Does that
> mean it is much harder to write correct comments than correct code?
> 

If one can explain their code to others and make sure others understand
the correctness of the code, then the code is very likely correct. So
I think yes, writing correct comments is harder than correct code.

I admit the review process took a bit long because it's a learning
process for both developers and reviewers: being the first in-tree Rust
driver may not be that easy, please understand the high standard
expectation.

Regards,
Boqun

>        Andrew

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

[Miguel Ojeda]

On Tue, Dec 12, 2023 at 5:04 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> The C side people read the Rust code before changing the C code? Let's
> see.

In the beginning, it is true that developers may forget often. But it
is still the case that they need to be mindful of the Rust side, just
like when changing some C function you need to take care of changing
its callers. That is also why we want to have maintainers of each
subsystem onboard.

In some cases it may be useful to annotate/mark the C side with a
comment warning about it (or similar) if you think it is likely that a
change on the C side will go unnoticed.

Cheers,
Miguel

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

[Alice Ryhl]

On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Mon, 11 Dec 2023 22:46:01 +0100
> Alice Ryhl <alice@ryhl.io> wrote:
> >> +    /// 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.
> >
> > Did you mix up two comments here? This doesn't parse in my brain.
>
> I'll remove the second comment because all we have to do here is using
> bindgen.
>
>
> >> +    /// 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.
> >
> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> > FFI call" or similar?
>
> "So it's just an FFI call" looks good. I'll fix all the places that
> use the same comment.

If you make those two comment changes, then you can add

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

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

[FUJITA Tomonori]

On Tue, 12 Dec 2023 10:23:30 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Mon, 11 Dec 2023 22:46:01 +0100
>> Alice Ryhl <alice@ryhl.io> wrote:
>> >> +    /// 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.
>> >
>> > Did you mix up two comments here? This doesn't parse in my brain.
>>
>> I'll remove the second comment because all we have to do here is using
>> bindgen.
>>
>>
>> >> +    /// 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.
>> >
>> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
>> > FFI call" or similar?
>>
>> "So it's just an FFI call" looks good. I'll fix all the places that
>> use the same comment.
> 
> If you make those two comment changes, then you can add
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

I will, thanks!

[PATCH net-next v10 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>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
 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

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

[Andrew Lunn]

On Mon, Dec 11, 2023 at 08:49:22AM +0900, 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Trevor Gross]

On Sun, Dec 10, 2023 at 6:50 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:

> +/// 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",
> +/// }

I still feel that device_table should be an optional field since it is
only rarely different from `drivers`.

But this is not worth blocking on and I can send a follow up patch after merge.

> [...]
> 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>

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

[PATCH net-next v10 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>
Signed-off-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7fb66210069b..089394a0af2e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7921,6 +7921,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 v10 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 089394a0af2e..bbc6db6bebf4 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 v10 4/4] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

On Mon, Dec 11, 2023 at 08:49:24AM +0900, 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Alice Ryhl]

On 12/11/23 00:49, 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>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>