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

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

[FUJITA Tomonori]

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

The first patch introduces Rust bindings for the C APIs for network
PHY drivers.

The second patch adds the bindings to the ETHERNET PHY LIBRARY, and
also me as a maintainer of the Rust bindings (as Andrew Lunn
suggested).

The last patch introduces the Rust version of Asix PHY drivers,
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.

You can find discussions on the RFC patches at rust-for-linux ml:

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


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

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


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

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

[FUJITA Tomonori]

This patch adds abstractions to implement network PHY drivers; the
driver registration and bindings for some of callback functions in
struct phy_driver and many genphy_ functions.

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

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

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

[FUJITA Tomonori]

Adds me as a maintainer for these Rust bindings too.

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

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

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

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

[FUJITA Tomonori]

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

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

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

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

[Greg KH]

On Mon, Oct 02, 2023 at 05:53:00PM +0900, FUJITA Tomonori wrote:
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +}

I still think these should come straight from the C code, and that
moving them to an enum makes sense to make this possible, but hey, it's
not my subsystem to maintain!  :)

> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn id(&mut self) -> u32 {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).phy_id }
> +    }

Naming question, why are you making this "id" instead of "phy_id"
like the C code has?  Same for many of these bindings.

> +    /// Returns true if the link is up.
> +    pub fn get_link(&mut self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).link() == LINK_IS_UP }
> +    }

"get_" normally means to grab a reference, if this isn't matching a C
call, why not just call it link_is_up or to match your other state
checks "is_link_up"?

but hey, I'm just bikeshedding at this point in time, if they maintainer
likes these as-is, keep them :)

thanks,

greg k-h

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

[Andrew Lunn]

> +//! Networking.
> +
> +#[cfg(CONFIG_PHYLIB)]

I brought this up on the rust for linux list, but did not get a answer
which convinced me.

Have you tried building this with PHYLIB as a kernel module? 

My understanding is that at the moment, this binding code is always
built in. So you somehow need to force phylib core to also be builtin.
Or you don't build the binding, and also don't allow a module to use
the binding.

	Andrew

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

[Andrew Lunn]

> +    /// Gets the id of the PHY.
> +    pub fn id(&mut self) -> u32 {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).phy_id }
> +    }

I somewhat agree with GregKH here. It will be easier to review and
maintain if the naming of well known things stay the same in the C and
Rust world. So phy_id. However....

> +    /// Gets the state of the PHY.
> +    pub fn state(&mut self) -> DeviceState {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let state = unsafe { (*phydev).state };
> +        match state {
> +            bindings::phy_state::PHY_DOWN => DeviceState::Down,
> +            bindings::phy_state::PHY_READY => DeviceState::Ready,
> +            bindings::phy_state::PHY_HALTED => DeviceState::Halted,
> +            bindings::phy_state::PHY_ERROR => DeviceState::Error,
> +            bindings::phy_state::PHY_UP => DeviceState::Up,
> +            bindings::phy_state::PHY_RUNNING => DeviceState::Running,
> +            bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
> +            bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
> +        }
> +    }
> +
> +    /// Returns true if the link is up.
> +    pub fn get_link(&mut self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).link() == LINK_IS_UP }
> +    }

Naming is hard.

This one is trickier and shows a difference between C and Rust. C just
does phydev->link and treats it as a boolean, setter/getters are not
needed. But Rust does seem to need setter/getters, and it is a lot
less clear what link() does. get_link() is a bit more
obvious. has_link() would also work. But as GregKH said, get_foo() and
put_foo() are often used to represent getting a reference on an object
and releasing it. I am however of the opinion that many driver writers
don't understand locking, so it is best to hide all the locking in the
core. I would not actually expect to see a PHY driver need to take a
reference on anything.

Since we forced into a world of getter/setter, the previous one
probably should be get_phy_id() and we want consistent set_ and get_
prefixes for plain accesses to members without further interpretation.

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

Should this maybe be get_autoneg_enabled()? I don't know.

       Andrew

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

[FUJITA Tomonori]

On Mon, 2 Oct 2023 16:52:45 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +//! Networking.
>> +
>> +#[cfg(CONFIG_PHYLIB)]
> 
> I brought this up on the rust for linux list, but did not get a answer
> which convinced me.

Sorry, I overlooked that discussion.


> Have you tried building this with PHYLIB as a kernel module? 

I've just tried and failed to build due to linker errors.


> My understanding is that at the moment, this binding code is always
> built in. So you somehow need to force phylib core to also be builtin.

Right. It means if you add Rust bindings for a subsystem, the
subsystem must be builtin, cannot be a module. I'm not sure if it's
acceptable.


> Or you don't build the binding, and also don't allow a module to use
> the binding.

I made PHY bindings available only if PHYLIB is builtin like the
following. However, we want modularity for Rust support, fully or
partially (e.g., per subsystem), I think.

Miguel, I suppose that you have worked on a new build system. It can
handle this problem?


diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index e4d941f0ebe4..a4776fdd9b6c 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -110,6 +110,7 @@ config AX88796B_PHY
 config AX88796B_RUST_PHY
 	bool "Rust reference driver"
 	depends on RUST && AX88796B_PHY
+	depends on PHYLIB=y
 	default n
 	help
 	  Uses the Rust version driver for Asix PHYs.
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
index b49b052969e5..da988d59b69d 100644
--- a/rust/kernel/net.rs
+++ b/rust/kernel/net.rs
@@ -2,5 +2,5 @@
 
 //! Networking.
 
-#[cfg(CONFIG_PHYLIB)]
+#[cfg(CONFIG_PHYLIB = "y")]
 pub mod phy;

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

[Andrew Lunn]

On Tue, Oct 03, 2023 at 09:33:38AM +0900, FUJITA Tomonori wrote:
> On Mon, 2 Oct 2023 16:52:45 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +//! Networking.
> >> +
> >> +#[cfg(CONFIG_PHYLIB)]
> > 
> > I brought this up on the rust for linux list, but did not get a answer
> > which convinced me.
> 
> Sorry, I overlooked that discussion.
> 
> 
> > Have you tried building this with PHYLIB as a kernel module? 
> 
> I've just tried and failed to build due to linker errors.
> 
> 
> > My understanding is that at the moment, this binding code is always
> > built in. So you somehow need to force phylib core to also be builtin.
> 
> Right. It means if you add Rust bindings for a subsystem, the
> subsystem must be builtin, cannot be a module. I'm not sure if it's
> acceptable.
 
You just need Kconfig in the Rust code to indicate it depends on
PHYLIB. Kconfig should then remove the option to build the phylib core
as a module. And that is acceptable.  

> > Or you don't build the binding, and also don't allow a module to use
> > the binding.
> 
> I made PHY bindings available only if PHYLIB is builtin like the
> following. However, we want modularity for Rust support, fully or
> partially (e.g., per subsystem), I think.
> 
> Miguel, I suppose that you have worked on a new build system. It can
> handle this problem?
> 
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index e4d941f0ebe4..a4776fdd9b6c 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -110,6 +110,7 @@ config AX88796B_PHY
>  config AX88796B_RUST_PHY
>  	bool "Rust reference driver"
>  	depends on RUST && AX88796B_PHY
> +	depends on PHYLIB=y

No, this is wrong. Miguel has said that leaf devices can be
modules. This driver is a leaf, all it depends on is the Rust
binding. And the Rust binding is built in, or not build at all. So the
first depends on covers that.

What is missing is that the Rust binding depends on PHYLIB.

     Andrew

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

[FUJITA Tomonori]

On Tue, 3 Oct 2023 03:40:50 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Oct 03, 2023 at 09:33:38AM +0900, FUJITA Tomonori wrote:
>> On Mon, 2 Oct 2023 16:52:45 +0200
>> Andrew Lunn <andrew@lunn.ch> wrote:
>> 
>> >> +//! Networking.
>> >> +
>> >> +#[cfg(CONFIG_PHYLIB)]
>> > 
>> > I brought this up on the rust for linux list, but did not get a answer
>> > which convinced me.
>> 
>> Sorry, I overlooked that discussion.
>> 
>> 
>> > Have you tried building this with PHYLIB as a kernel module? 
>> 
>> I've just tried and failed to build due to linker errors.
>> 
>> 
>> > My understanding is that at the moment, this binding code is always
>> > built in. So you somehow need to force phylib core to also be builtin.
>> 
>> Right. It means if you add Rust bindings for a subsystem, the
>> subsystem must be builtin, cannot be a module. I'm not sure if it's
>> acceptable.
>  
> You just need Kconfig in the Rust code to indicate it depends on
> PHYLIB. Kconfig should then remove the option to build the phylib core
> as a module. And that is acceptable.  

The following works. If you set the phylib as a module, the rust
option isn't available.

diff --git a/init/Kconfig b/init/Kconfig
index 6d35728b94b2..4b4e3df1658d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1889,6 +1889,7 @@ config RUST
 	depends on !GCC_PLUGINS
 	depends on !RANDSTRUCT
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
+	depends on PHYLIB=y
 	select CONSTRUCTORS
 	help
 	  Enables Rust support in the kernel.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 0588422e273c..f9883bde4459 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -37,7 +37,6 @@
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
-#[cfg(CONFIG_NET)]
 pub mod net;
 pub mod prelude;
 pub mod print;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
index b49b052969e5..fbb6d9683012 100644
--- a/rust/kernel/net.rs
+++ b/rust/kernel/net.rs
@@ -2,5 +2,4 @@
 
 //! Networking.
 
-#[cfg(CONFIG_PHYLIB)]
 pub mod phy;

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

[Greg KH]

On Tue, Oct 03, 2023 at 12:43:11PM +0900, FUJITA Tomonori wrote:
> On Tue, 3 Oct 2023 03:40:50 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Tue, Oct 03, 2023 at 09:33:38AM +0900, FUJITA Tomonori wrote:
> >> On Mon, 2 Oct 2023 16:52:45 +0200
> >> Andrew Lunn <andrew@lunn.ch> wrote:
> >> 
> >> >> +//! Networking.
> >> >> +
> >> >> +#[cfg(CONFIG_PHYLIB)]
> >> > 
> >> > I brought this up on the rust for linux list, but did not get a answer
> >> > which convinced me.
> >> 
> >> Sorry, I overlooked that discussion.
> >> 
> >> 
> >> > Have you tried building this with PHYLIB as a kernel module? 
> >> 
> >> I've just tried and failed to build due to linker errors.
> >> 
> >> 
> >> > My understanding is that at the moment, this binding code is always
> >> > built in. So you somehow need to force phylib core to also be builtin.
> >> 
> >> Right. It means if you add Rust bindings for a subsystem, the
> >> subsystem must be builtin, cannot be a module. I'm not sure if it's
> >> acceptable.
> >  
> > You just need Kconfig in the Rust code to indicate it depends on
> > PHYLIB. Kconfig should then remove the option to build the phylib core
> > as a module. And that is acceptable.  
> 
> The following works. If you set the phylib as a module, the rust
> option isn't available.

That does not seem wise.  Why not make the binding a module as well?

thanks,

greg k-h

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

[FUJITA Tomonori]

On Tue, 3 Oct 2023 08:31:38 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Oct 03, 2023 at 12:43:11PM +0900, FUJITA Tomonori wrote:
>> On Tue, 3 Oct 2023 03:40:50 +0200
>> Andrew Lunn <andrew@lunn.ch> wrote:
>> 
>> > On Tue, Oct 03, 2023 at 09:33:38AM +0900, FUJITA Tomonori wrote:
>> >> On Mon, 2 Oct 2023 16:52:45 +0200
>> >> Andrew Lunn <andrew@lunn.ch> wrote:
>> >> 
>> >> >> +//! Networking.
>> >> >> +
>> >> >> +#[cfg(CONFIG_PHYLIB)]
>> >> > 
>> >> > I brought this up on the rust for linux list, but did not get a answer
>> >> > which convinced me.
>> >> 
>> >> Sorry, I overlooked that discussion.
>> >> 
>> >> 
>> >> > Have you tried building this with PHYLIB as a kernel module? 
>> >> 
>> >> I've just tried and failed to build due to linker errors.
>> >> 
>> >> 
>> >> > My understanding is that at the moment, this binding code is always
>> >> > built in. So you somehow need to force phylib core to also be builtin.
>> >> 
>> >> Right. It means if you add Rust bindings for a subsystem, the
>> >> subsystem must be builtin, cannot be a module. I'm not sure if it's
>> >> acceptable.
>> >  
>> > You just need Kconfig in the Rust code to indicate it depends on
>> > PHYLIB. Kconfig should then remove the option to build the phylib core
>> > as a module. And that is acceptable.  
>> 
>> The following works. If you set the phylib as a module, the rust
>> option isn't available.
> 
> That does not seem wise.  Why not make the binding a module as well?

Agreed, as I wrote in the previous mail. But Rust bindings doesn't
support such now. I suppose that Miguel has worked on a new build
system for Rust. He might have plans.

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

[Andrew Lunn]

> >> The following works. If you set the phylib as a module, the rust
> >> option isn't available.
> > 
> > That does not seem wise.  Why not make the binding a module as well?
> 
> Agreed, as I wrote in the previous mail. But Rust bindings doesn't
> support such now. I suppose that Miguel has worked on a new build
> system for Rust. He might have plans.

It would be nice to get the basic Kconfig symbols in place to support
this, for when the build does support bindings in modules. And you
should be able to use the same symbols now, just with slightly
different semantics.

The binding should have its own Kconfig Symbol, something like
RUST_PHYLIB_BINDING, with a depends on RUST and a depends on
PHYLIB. At the moment that should just cause the binding to be
compile/not compiled as part of the core rust support. The dependency
on PHYLIB will also ensure it is built in the correct way.

In the future, it can be used to build the binding as a standalone
module, built in, or not at all.

The PHY driver would then have a depends on RUST_PHYLIB_BINDING.

    Andrew

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

[FUJITA Tomonori]

On Mon, 2 Oct 2023 17:24:17 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +    /// Gets the id of the PHY.
>> +    pub fn id(&mut self) -> u32 {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        unsafe { (*phydev).phy_id }
>> +    }
> 
> I somewhat agree with GregKH here. It will be easier to review and
> maintain if the naming of well known things stay the same in the C and
> Rust world. So phy_id. However....

phy_id() is fine by me.

The complete type name is `net::phy::Device` so I guess that the
method names usually don't start with `phy`. But we maintain both C
and Rust so I think that we need a balance between them.


>> +    /// Gets the state of the PHY.
>> +    pub fn state(&mut self) -> DeviceState {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let state = unsafe { (*phydev).state };
>> +        match state {
>> +            bindings::phy_state::PHY_DOWN => DeviceState::Down,
>> +            bindings::phy_state::PHY_READY => DeviceState::Ready,
>> +            bindings::phy_state::PHY_HALTED => DeviceState::Halted,
>> +            bindings::phy_state::PHY_ERROR => DeviceState::Error,
>> +            bindings::phy_state::PHY_UP => DeviceState::Up,
>> +            bindings::phy_state::PHY_RUNNING => DeviceState::Running,
>> +            bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
>> +            bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
>> +        }
>> +    }
>> +
>> +    /// Returns true if the link is up.
>> +    pub fn get_link(&mut self) -> bool {
>> +        const LINK_IS_UP: u32 = 1;
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        unsafe { (*phydev).link() == LINK_IS_UP }
>> +    }
> 
> Naming is hard.
> 
> This one is trickier and shows a difference between C and Rust. C just
> does phydev->link and treats it as a boolean, setter/getters are not
> needed. But Rust does seem to need setter/getters, and it is a lot
> less clear what link() does. get_link() is a bit more
> obvious. has_link() would also work. But as GregKH said, get_foo() and
> put_foo() are often used to represent getting a reference on an object
> and releasing it. I am however of the opinion that many driver writers
> don't understand locking, so it is best to hide all the locking in the
> core. I would not actually expect to see a PHY driver need to take a
> reference on anything.
> 
> Since we forced into a world of getter/setter, the previous one
> probably should be get_phy_id() and we want consistent set_ and get_
> prefixes for plain accesses to members without further interpretation.

get/set_something names aren't commonly used in Rust, I guess. Some examples
follows in the standard library.

https://doc.rust-lang.org/stable/std/net/struct.TcpStream.html

there are set_linger(), set_nodelay(), set_read_timeout(),
set_write_timeout(). correspondingly, linger(), nodelay(),
read_timeout(), write_timeout() are provided.

https://doc.rust-lang.org/stable/std/io/struct.Cursor.html

There are set_position() and position().

As I wrote above, I don't think that we need to follow Rust naming
practices strictly, as long as there are patterns in Rust bindings.


>> +    /// Returns true if auto-negotiation is enabled.
>> +    pub fn is_autoneg_enabled(&mut self) -> bool {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
>> +    }
> 
> Should this maybe be get_autoneg_enabled()? I don't know.

I think that we can leave this name alone since tis_something() names
are used for OS related functions in Rust.