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

[PATCH v2 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.

There is no major changes from v1; build failure fix and function renaming.

v1:
https://lore.kernel.org/netdev/20231002085302.2274260-3-fujita.tomonori@gmail.com/T/

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 ++++++
 init/Kconfig                     |   1 +
 rust/Makefile                    |   1 +
 rust/bindings/bindings_helper.h  |   3 +
 rust/kernel/lib.rs               |   2 +
 rust/kernel/net.rs               |   5 +
 rust/kernel/net/phy.rs           | 706 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 11 files changed, 862 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: b2516f7af9d238ebc391bdbdae01ac9528f1109e
-- 
2.34.1

[PATCH v2 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>
---
 init/Kconfig                    |   1 +
 rust/Makefile                   |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/net.rs              |   5 +
 rust/kernel/net/phy.rs          | 706 ++++++++++++++++++++++++++++++++
 6 files changed, 718 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

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/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..f9883bde4459 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,7 @@
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+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..fbb6d9683012
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..d06a83ad8a8a
--- /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 phy_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 v2 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 1a5b0bda2b05..006e601f9862 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7767,6 +7767,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
@@ -7796,6 +7797,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 v2 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 v2 3/3] net: phy: add Rust Asix PHY driver

[Greg KH]

On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote:
> +config AX88796B_RUST_PHY
> +	bool "Rust reference driver"
> +	depends on RUST && AX88796B_PHY
> +	default n

Nit, "n" is always the default, there is no need for this line.

> +	help
> +	  Uses the Rust version driver for Asix PHYs.

You need more text here please.  Provide a better description of what
hardware is supported and the name of the module if it is built aas a
module.

Also that if you select this one, the C driver will not be built (which
is not expressed in the Kconfig language, why not?

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

This can be expressed in Kconfig, no need to put this here, right?

>  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

No copyright line?  Are you sure?

thanks,

greg k-h

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[Andrew Lunn]

On Fri, Oct 06, 2023 at 06:49:08PM +0900, FUJITA Tomonori wrote:
> 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.

One of the conventions for submitting patches for netdev is to include
the tree in the Subject.

[PATCH net-next v2 1/3] rust: core abstractions for network PHY drivers

This is described here, along with other useful hits for working with
netdev.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

This tag helps patchworks decide which tree to apply your patches to
and then run build tests on it:

https://patchwork.kernel.org/project/netdevbpf/patch/20231006094911.3305152-4-fujita.tomonori@gmail.com/

I don't know if it made the wrong decision based on the missing tag,
or it simply does not know what to do with Rust yet.

There is also the question of how we merge this. Does it all come
through netdev? Do we split the patches, the abstraction merged via
rust and the rest via netdev? Is the Kconfig sufficient that if a tree
only contains patches 2 and 3 it does not allow the driver to be
enabled?

	Andrew
	

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 12:31:59 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote:
>> +config AX88796B_RUST_PHY
>> +	bool "Rust reference driver"
>> +	depends on RUST && AX88796B_PHY
>> +	default n
> 
> Nit, "n" is always the default, there is no need for this line.

Understood, I'll remove this line.

>> +	help
>> +	  Uses the Rust version driver for Asix PHYs.
> 
> You need more text here please.  Provide a better description of what
> hardware is supported and the name of the module if it is built aas a
> module.
> 
> Also that if you select this one, the C driver will not be built (which
> is not expressed in the Kconfig language, why not?

Because the way to load a PHY driver module can't handle multiple
modules with the same phy id (a NIC driver loads a PHY driver module).
There is no machinism to specify which PHY driver module should be
loaded when multiple PHY modules have the same phy id (as far as I know).

The Kconfig file would be like the following. AX88796B_RUST_PHY
depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
I'll add the name of the module.


config AX88796B_PHY
	tristate "Asix PHYs"
	help
	  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
> 
> This can be expressed in Kconfig, no need to put this here, right?

Not sure. Is it possible? If we allow both modules to be built, I
guess it's possible though.


>>  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
> 
> No copyright line?  Are you sure?

Ah, I'll add.

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

On Fri, 6 Oct 2023 14:54:43 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Oct 06, 2023 at 06:49:08PM +0900, FUJITA Tomonori wrote:
>> 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.
> 
> One of the conventions for submitting patches for netdev is to include
> the tree in the Subject.
> 
> [PATCH net-next v2 1/3] rust: core abstractions for network PHY drivers
> 
> This is described here, along with other useful hits for working with
> netdev.
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> This tag helps patchworks decide which tree to apply your patches to
> and then run build tests on it:
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20231006094911.3305152-4-fujita.tomonori@gmail.com/
> 
> I don't know if it made the wrong decision based on the missing tag,
> or it simply does not know what to do with Rust yet.

Thanks, I didn't know how tags and patchworks works.

> There is also the question of how we merge this. Does it all come
> through netdev? Do we split the patches, the abstraction merged via
> rust and the rest via netdev? Is the Kconfig sufficient that if a tree
> only contains patches 2 and 3 it does not allow the driver to be
> enabled?

A tree only that contains patches 2 and 3 allow the driver to be
enabled, I think. The driver depends on CONFIG_RUST, which might
doesn't have PHY bindings support (the first patch).

So I think that merging the patchset through a single tree is easier;
netdev or rust.

Miguel, how do you prefer to merge the patchset?

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

[Greg KH]

On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote:
> On Fri, 6 Oct 2023 12:31:59 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote:
> >> +config AX88796B_RUST_PHY
> >> +	bool "Rust reference driver"
> >> +	depends on RUST && AX88796B_PHY
> >> +	default n
> > 
> > Nit, "n" is always the default, there is no need for this line.
> 
> Understood, I'll remove this line.
> 
> >> +	help
> >> +	  Uses the Rust version driver for Asix PHYs.
> > 
> > You need more text here please.  Provide a better description of what
> > hardware is supported and the name of the module if it is built aas a
> > module.
> > 
> > Also that if you select this one, the C driver will not be built (which
> > is not expressed in the Kconfig language, why not?
> 
> Because the way to load a PHY driver module can't handle multiple
> modules with the same phy id (a NIC driver loads a PHY driver module).
> There is no machinism to specify which PHY driver module should be
> loaded when multiple PHY modules have the same phy id (as far as I know).

Sorry, I know that, I mean I am pretty sure you can express this "one or
the other" type of restriction in Kconfig, no need to encode it in the
Makefile logic.

Try doing "depens on AX88796B_PHY=n" as the dependency for the rust
driver.

> The Kconfig file would be like the following. AX88796B_RUST_PHY
> depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
> I'll add the name of the module.
> 
> 
> config AX88796B_PHY
> 	tristate "Asix PHYs"
> 	help
> 	  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.

"This is the rust version of a driver to support...  It will be
called..."

> 
> >> +
> >>  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
> > 
> > This can be expressed in Kconfig, no need to put this here, right?
> 
> Not sure. Is it possible? If we allow both modules to be built, I
> guess it's possible though.

see above, this is expressed in the Kconfig language and then no ifdef
is needed here at all.

thanks,

greg k-h

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 16:12:24 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote:
>> On Fri, 6 Oct 2023 12:31:59 +0200
>> Greg KH <gregkh@linuxfoundation.org> wrote:
>> 
>> > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote:
>> >> +config AX88796B_RUST_PHY
>> >> +	bool "Rust reference driver"
>> >> +	depends on RUST && AX88796B_PHY
>> >> +	default n
>> > 
>> > Nit, "n" is always the default, there is no need for this line.
>> 
>> Understood, I'll remove this line.
>> 
>> >> +	help
>> >> +	  Uses the Rust version driver for Asix PHYs.
>> > 
>> > You need more text here please.  Provide a better description of what
>> > hardware is supported and the name of the module if it is built aas a
>> > module.
>> > 
>> > Also that if you select this one, the C driver will not be built (which
>> > is not expressed in the Kconfig language, why not?
>> 
>> Because the way to load a PHY driver module can't handle multiple
>> modules with the same phy id (a NIC driver loads a PHY driver module).
>> There is no machinism to specify which PHY driver module should be
>> loaded when multiple PHY modules have the same phy id (as far as I know).
> 
> Sorry, I know that, I mean I am pretty sure you can express this "one or
> the other" type of restriction in Kconfig, no need to encode it in the
> Makefile logic.
> 
> Try doing "depens on AX88796B_PHY=n" as the dependency for the rust
> driver.

You meant the following?

config AX88796B_PHY
	tristate "Asix PHYs"
	help
	  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.


The problem is that there are NIC drivers that `select
AX88796B_PHY`. the Kconfig language doesn't support something like
`select AX88796B_PHY or AX88796B_RUST_PHY`, I guess.

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

[Andrew Lunn]

> The Kconfig file would be like the following. AX88796B_RUST_PHY
> depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
> I'll add the name of the module.
> 
> 
> config AX88796B_PHY
> 	tristate "Asix PHYs"
> 	help
> 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
> 	  AX88796B package.

I _think_ you can add

	depends on !AX88796B_RUST_PHY

> config AX88796B_RUST_PHY
> 	bool "Rust reference driver"
> 	depends on RUST && AX88796B_PHY

And then this becomes

    	depends on RUST && !AX88796B_PHY

> 	default n
> 	help
> 	  Uses the Rust version driver for Asix PHYs.

You then express the mutual exclusion in Kconfig, so that only one of
AX88796B_PHY and AX88796B_RUST_PHY is ever enabled.

I've not actually tried this, so it might not work. Ideally you need
to be able disable both, so that you can enable one.

There is good documentation in

Documentation/kbuild/kconfig-language.rst

> >> +ifdef CONFIG_AX88796B_RUST_PHY
> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
> >> +else
> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
> >> +endif
> > 
> > This can be expressed in Kconfig, no need to put this here, right?
> 
> Not sure. Is it possible? If we allow both modules to be built, I
> guess it's possible though.

If what i suggested above works, you don't need the ifdef, just list
the two drivers are normal and let Kconfig only enable one at most.
Or go back to your idea of using choice. Maybe something like

choice
	tristate "AX88796B PHY driver"

	config CONFIG_AX88796B_PHY
		bool "C driver"

	config CONFIG_AX88796B_RUST_PHY
	        bool "Rust driver"
		depends on RUST
endchoice

totally untested....

	Andrew

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

[Greg KH]

On Fri, Oct 06, 2023 at 11:30:54PM +0900, FUJITA Tomonori wrote:
> On Fri, 6 Oct 2023 16:12:24 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote:
> >> On Fri, 6 Oct 2023 12:31:59 +0200
> >> Greg KH <gregkh@linuxfoundation.org> wrote:
> >> 
> >> > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote:
> >> >> +config AX88796B_RUST_PHY
> >> >> +	bool "Rust reference driver"
> >> >> +	depends on RUST && AX88796B_PHY
> >> >> +	default n
> >> > 
> >> > Nit, "n" is always the default, there is no need for this line.
> >> 
> >> Understood, I'll remove this line.
> >> 
> >> >> +	help
> >> >> +	  Uses the Rust version driver for Asix PHYs.
> >> > 
> >> > You need more text here please.  Provide a better description of what
> >> > hardware is supported and the name of the module if it is built aas a
> >> > module.
> >> > 
> >> > Also that if you select this one, the C driver will not be built (which
> >> > is not expressed in the Kconfig language, why not?
> >> 
> >> Because the way to load a PHY driver module can't handle multiple
> >> modules with the same phy id (a NIC driver loads a PHY driver module).
> >> There is no machinism to specify which PHY driver module should be
> >> loaded when multiple PHY modules have the same phy id (as far as I know).
> > 
> > Sorry, I know that, I mean I am pretty sure you can express this "one or
> > the other" type of restriction in Kconfig, no need to encode it in the
> > Makefile logic.
> > 
> > Try doing "depens on AX88796B_PHY=n" as the dependency for the rust
> > driver.
> 
> You meant the following?
> 
> config AX88796B_PHY
> 	tristate "Asix PHYs"
> 	help
> 	  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.

Nope, that's not going to work.

And your string is wrong :(

> The problem is that there are NIC drivers that `select
> AX88796B_PHY`. the Kconfig language doesn't support something like
> `select AX88796B_PHY or AX88796B_RUST_PHY`, I guess.

You're going to have to figure out which you want to have in the system,
so if a NIC driver selects the C version, live with that.

This is just the tip of the iceburg of the mess that is "duplicate
drivers in the kernel tree" that I keep warning people about.

You all are on your own here now, good luck!

greg k-h

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

[Andrew Lunn]

> config AX88796B_PHY
> 	tristate "Asix PHYs"
> 	help
> 	  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.
> 
> 
> The problem is that there are NIC drivers that `select
> AX88796B_PHY`. the Kconfig language doesn't support something like
> `select AX88796B_PHY or AX88796B_RUST_PHY`, I guess.

So change AX88796B_PHY to mean any driver for that hardware. And then
move the C driver to AX88796B_C_PHY, and add AX88796B_RUST_PHY.

All the MAC drivers really cares about is that there is a PHY
driver. They don't care if it is written in C, Rust, or SNOBOL.

	Andrew

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[Andrew Lunn]

> A tree only that contains patches 2 and 3 allow the driver to be
> enabled, I think. The driver depends on CONFIG_RUST, which might
> doesn't have PHY bindings support (the first patch).

This is part of why i said there should be a Kconfig symbol
CONFIG_RUST_PHYLIB_BINDING or similar. With only patches 2 and 3, that
would not exists, and so you cannot enable the driver. Once all the
patches meet up in linux-next, you have both parts, and you can enable
it.

> So I think that merging the patchset through a single tree is easier;
> netdev or rust.
> 
> Miguel, how do you prefer to merge the patchset?

What are the merge conflicts looking like? What has happened in the
past? Or is this the first driver to actually get this far towards
being merged?

      Andrew

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 16:35:28 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> The Kconfig file would be like the following. AX88796B_RUST_PHY
>> depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
>> I'll add the name of the module.
>> 
>> 
>> config AX88796B_PHY
>> 	tristate "Asix PHYs"
>> 	help
>> 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>> 	  AX88796B package.
> 
> I _think_ you can add
> 
> 	depends on !AX88796B_RUST_PHY
> 
>> config AX88796B_RUST_PHY
>> 	bool "Rust reference driver"
>> 	depends on RUST && AX88796B_PHY
> 
> And then this becomes
> 
>     	depends on RUST && !AX88796B_PHY
> 
>> 	default n
>> 	help
>> 	  Uses the Rust version driver for Asix PHYs.
> 
> You then express the mutual exclusion in Kconfig, so that only one of
> AX88796B_PHY and AX88796B_RUST_PHY is ever enabled.
> 
> I've not actually tried this, so it might not work. Ideally you need
> to be able disable both, so that you can enable one.

This doesn't work.

ubuntu@ip-172-30-47-114:~/git/linux$ make LLVM=1 -j 32 menuconfig
drivers/net/phy/Kconfig:111:error: recursive dependency detected!
drivers/net/phy/Kconfig:111:    symbol AX88796B_RUST_PHY depends on AX88796B_PHY
drivers/net/phy/Kconfig:104:    symbol AX88796B_PHY depends on AX88796B_RUST_P

The following gurantees that only one is built but we hit the `select
AX88796B_PHY` problem in my previous mail.

config AX88796B_PHY
        tristate "Asix PHYs"
        help
          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=n
        help
          Uses the Rust version driver for Asix PHYs.


Greg, Sorry. I messed up copy-and-paste in the previous mail. I think that you meant the above.

> There is good documentation in
> 
> Documentation/kbuild/kconfig-language.rst
> 
>> >> +ifdef CONFIG_AX88796B_RUST_PHY
>> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
>> >> +else
>> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
>> >> +endif
>> > 
>> > This can be expressed in Kconfig, no need to put this here, right?
>> 
>> Not sure. Is it possible? If we allow both modules to be built, I
>> guess it's possible though.
> 
> If what i suggested above works, you don't need the ifdef, just list
> the two drivers are normal and let Kconfig only enable one at most.
> Or go back to your idea of using choice. Maybe something like
> 
> choice
> 	tristate "AX88796B PHY driver"
> 
> 	config CONFIG_AX88796B_PHY
> 		bool "C driver"
> 
> 	config CONFIG_AX88796B_RUST_PHY
> 	        bool "Rust driver"
> 		depends on RUST
> endchoice
> 
> totally untested....

Now I'm thinking that this is the best option. Kconfig would be the following:

config AX88796B_PHY
        tristate "Asix PHYs"
        help
         Currently supports the Asix Electronics PHY found in the X-Surf 100
         AX88796B package.

choice
        prompt "Implementation options"
        depends on AX88796B_PHY
        help
         There are two implementations for a driver for Asix PHYs; C and Rust.
         If not sure, choose C.

config AX88796B_C_PHY
        bool "The C version driver for Asix PHYs"

config AX88796B_RUST_PHY
        bool "The Rust version driver for Asix PHYs"
        depends on RUST

endchoice


No hack in Makefile:

obj-$(CONFIG_AX88796B_C_PHY)    += ax88796b.o
obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o

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

[Andrew Lunn]

> Now I'm thinking that this is the best option. Kconfig would be the following:
> 
> config AX88796B_PHY
>         tristate "Asix PHYs"
>         help
>          Currently supports the Asix Electronics PHY found in the X-Surf 100
>          AX88796B package.
> 
> choice
>         prompt "Implementation options"
>         depends on AX88796B_PHY
>         help
>          There are two implementations for a driver for Asix PHYs; C and Rust.
>          If not sure, choose C.
> 
> config AX88796B_C_PHY
>         bool "The C version driver for Asix PHYs"
> 
> config AX88796B_RUST_PHY
>         bool "The Rust version driver for Asix PHYs"
>         depends on RUST
> 
> endchoice
> 
> 
> No hack in Makefile:
> 
> obj-$(CONFIG_AX88796B_C_PHY)    += ax88796b.o
> obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o

This looks reasonable. Lets use this. But i still think we need some
sort of RUST_PHYLIB_BINDING.

     Andrew

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 17:57:41 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Now I'm thinking that this is the best option. Kconfig would be the following:
>> 
>> config AX88796B_PHY
>>         tristate "Asix PHYs"
>>         help
>>          Currently supports the Asix Electronics PHY found in the X-Surf 100
>>          AX88796B package.
>> 
>> choice
>>         prompt "Implementation options"
>>         depends on AX88796B_PHY
>>         help
>>          There are two implementations for a driver for Asix PHYs; C and Rust.
>>          If not sure, choose C.
>> 
>> config AX88796B_C_PHY
>>         bool "The C version driver for Asix PHYs"
>> 
>> config AX88796B_RUST_PHY
>>         bool "The Rust version driver for Asix PHYs"
>>         depends on RUST
>> 
>> endchoice
>> 
>> 
>> No hack in Makefile:
>> 
>> obj-$(CONFIG_AX88796B_C_PHY)    += ax88796b.o
>> obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o
> 
> This looks reasonable. Lets use this. But i still think we need some
> sort of RUST_PHYLIB_BINDING.

How about adding CONFIG_RUST_PHYLIB to the first patch. Not
selectable, it's just a flag for Rust PHYLIB support.

diff --git a/init/Kconfig b/init/Kconfig
index 4b4e3df1658d..2b6627aeb98c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1889,7 +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 RUST_PHYLIB
 	select CONSTRUCTORS
 	help
 	  Enables Rust support in the kernel.
@@ -1904,6 +1904,10 @@ config RUST
 
 	  If unsure, say N.
 
+config RUST_PHYLIB
+	bool
+
 config RUSTC_VERSION_TEXT
 	string
 	depends on RUST


Then the driver depends on RUST instead of RUST_PHYLIB.

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 82ecfffc276c..e0d7a19ca774 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -119,7 +119,7 @@ config AX88796B_C_PHY
 
 config AX88796B_RUST_PHY
 	bool "The Rust version driver for Asix PHYs"
-	depends on RUST
+	depends on RUST_PHYLIB
 
 endchoice

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

[Andrew Lunn]

> How about adding CONFIG_RUST_PHYLIB to the first patch. Not
> selectable, it's just a flag for Rust PHYLIB support.

We have to be careful with names. To some extent, CONFIG_PHYLIB means
the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the
core of phylib written in rust? I doubt that will ever happen, but we
are setting a naming scheme here which i expect others will blindly
cut/paste. What we actually want is a symbol which represents the Rust
binding onto the phylib core. So i think it should have BINDING, or
WRAPPER or something like that in the name.

> diff --git a/init/Kconfig b/init/Kconfig
> index 4b4e3df1658d..2b6627aeb98c 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1889,7 +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 RUST_PHYLIB

I know the rust build system is rather limited at the moment, but is
this required? Is it possible to build the rust code without the
phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile
target only if it is enabled?

>  	select CONSTRUCTORS
>  	help
>  	  Enables Rust support in the kernel.
> @@ -1904,6 +1904,10 @@ config RUST
>  
>  	  If unsure, say N.
>  
> +config RUST_PHYLIB
> +	bool

This is where the depends on PHYLIB should be. It is the Rust binding
on phylib which has the dependency on phylib, not the core rust code.


What i think the end state should be, once the Rust build system is
better is that in drivers/net/phy/Kconfig we have:

if PHYLIB

config RUST_PHYLIB_BINDING
    bool
    depends on RUST
    help
      Adds support needed for PHY drivers written in Rust. It provides
      a wrapper around the C phlib core.

and the Makefile when uses this to build the binding as a kernel
module.

	Andrew

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[Trevor Gross]

On Fri, Oct 6, 2023 at 10:47 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > So I think that merging the patchset through a single tree is easier;
> > netdev or rust.
> >
> > Miguel, how do you prefer to merge the patchset?
>
> What are the merge conflicts looking like? What has happened in the
> past? [...]

Miguel has said before that if subsystems are comfortable bringing
rust through their trees then they are welcome to do so, which helps
get a better idea of how everything works together. If you prefer not
to, it can come through rust-next with no problem.

There are no serious conflicts on the rust side since there is no net
module yet. I think that most new things will need to touch lib.rs and
the binding helper just to register themselves, but those are trivial
(e.g. same for wq updates coming [1]).

> Or is this the first driver to actually get this far towards
> being merged?
>
>       Andrew

I think that answer is yes :) at least for an actual leaf driver.
Hence some of the build system rough edges.

[1]: https://lore.kernel.org/rust-for-linux/20230828104807.1581592-1-aliceryhl@google.com/

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 18:55:23 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> How about adding CONFIG_RUST_PHYLIB to the first patch. Not
>> selectable, it's just a flag for Rust PHYLIB support.
> 
> We have to be careful with names. To some extent, CONFIG_PHYLIB means
> the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the
> core of phylib written in rust? I doubt that will ever happen, but we
> are setting a naming scheme here which i expect others will blindly
> cut/paste. What we actually want is a symbol which represents the Rust
> binding onto the phylib core. So i think it should have BINDING, or
> WRAPPER or something like that in the name.

Good point. Let's save CONFIG_PHYLIB for someday.


>> diff --git a/init/Kconfig b/init/Kconfig
>> index 4b4e3df1658d..2b6627aeb98c 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1889,7 +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 RUST_PHYLIB
> 
> I know the rust build system is rather limited at the moment, but is
> this required? Is it possible to build the rust code without the
> phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile
> target only if it is enabled?

A short-term solution could work, I think.

config RUST
	bool "Rust support"
	depends on HAVE_RUST
	depends on RUST_IS_AVAILABLE
	depends on !MODVERSIONS
	depends on !GCC_PLUGINS
	depends on !RANDSTRUCT
	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
	select CONSTRUCTORS
	help
	  Enables Rust support in the kernel.

	  This allows other Rust-related options, like drivers written in Rust,
	  to be selected.

	  It is also required to be able to load external kernel modules
	  written in Rust.

	  See Documentation/rust/ for more information.

	  If unsure, say N.

config RUST_PHYLIB_BINDING
	bool "PHYLIB bindings 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 phlib core.


Then we can conditionally build build the PHYLIB bindings.

diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
index fbb6d9683012..33fc1531a6c0 100644
--- a/rust/kernel/net.rs
+++ b/rust/kernel/net.rs
@@ -2,4 +2,5 @@

 //! Networking.

+#[cfg(CONFIG_RUST_BINDINGS_PHYLIB)]
 pub mod phy;


A long-term solution is under discussion.

https://lore.kernel.org/rust-for-linux/20231006155739.246381-1-yakoyoku@gmail.com/

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

[Andrew Lunn]

On Sat, Oct 07, 2023 at 08:54:00AM +0900, FUJITA Tomonori wrote:
> On Fri, 6 Oct 2023 18:55:23 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> How about adding CONFIG_RUST_PHYLIB to the first patch. Not
> >> selectable, it's just a flag for Rust PHYLIB support.
> > 
> > We have to be careful with names. To some extent, CONFIG_PHYLIB means
> > the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the
> > core of phylib written in rust? I doubt that will ever happen, but we
> > are setting a naming scheme here which i expect others will blindly
> > cut/paste. What we actually want is a symbol which represents the Rust
> > binding onto the phylib core. So i think it should have BINDING, or
> > WRAPPER or something like that in the name.
> 
> Good point. Let's save CONFIG_PHYLIB for someday.
> 
> 
> >> diff --git a/init/Kconfig b/init/Kconfig
> >> index 4b4e3df1658d..2b6627aeb98c 100644
> >> --- a/init/Kconfig
> >> +++ b/init/Kconfig
> >> @@ -1889,7 +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 RUST_PHYLIB
> > 
> > I know the rust build system is rather limited at the moment, but is
> > this required? Is it possible to build the rust code without the
> > phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile
> > target only if it is enabled?
> 
> A short-term solution could work, I think.
> 
> config RUST
> 	bool "Rust support"
> 	depends on HAVE_RUST
> 	depends on RUST_IS_AVAILABLE
> 	depends on !MODVERSIONS
> 	depends on !GCC_PLUGINS
> 	depends on !RANDSTRUCT
> 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> 	select CONSTRUCTORS
> 	help
> 	  Enables Rust support in the kernel.
> 
> 	  This allows other Rust-related options, like drivers written in Rust,
> 	  to be selected.
> 
> 	  It is also required to be able to load external kernel modules
> 	  written in Rust.
> 
> 	  See Documentation/rust/ for more information.
> 
> 	  If unsure, say N.
> 
> config RUST_PHYLIB_BINDING
> 	bool "PHYLIB bindings 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 phlib core.
> 
> 
> Then we can conditionally build build the PHYLIB bindings.
> 
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> index fbb6d9683012..33fc1531a6c0 100644
> --- a/rust/kernel/net.rs
> +++ b/rust/kernel/net.rs
> @@ -2,4 +2,5 @@
> 
>  //! Networking.
> 
> +#[cfg(CONFIG_RUST_BINDINGS_PHYLIB)]
>  pub mod phy;

This looks reasonable. If you spin a new version with all these
Kconfig changes, i will do some compile testing.

	Andrew

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[Trevor Gross]

Replying here to a missed followup on rfc v3 [1]

On Mon, Oct 2, 2023 at 3:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
> The kernel is documented using kerneldoc. It would seem odd to me to
> have a second parallel set of Documentation for Rust. Just like Rust
> is integrated into the kernel tree, is configured using Kconfig, built
> using make at the top level, i would also expect it to integrate into
> kerneldoc somehow. I see the Rust API for PHY drivers next to the C
> API for PHY drivers. Its just another API in the kernel, nothing
> special. I just use 'make htmldocs' at the top level and out come the
> HTML documentation in Documentation/output/
>
> But kerneldoc is not my subsystem. MAINTAINERS say:
>
> DOCUMENTATION
> M:      Jonathan Corbet <corbet@lwn.net>
> L:      linux-doc@vger.kernel.org
> S:      Maintained
>
> So this discussion should really have Jonathon Corbet involved, if it
> has not already been done.
>
>     Andrew

Having the documentation in the same place and able to easily
crosslink is a goal, but it's still a work in progress. It won't look
the same of course but I think that the rustdoc output will be under
kerneldoc, with some sort of automated crosslinking between related
modules.

The docs team is in the loop, see [2] which was merged. (I suppose it
must not be getting published still)

- Trevor

[1]: https://lore.kernel.org/rust-for-linux/78da96fc-cf66-4645-a98f-80e404800d3e@lunn.ch/
[2]: https://lore.kernel.org/rust-for-linux/20230718151534.4067460-1-carlos.bilbao@amd.com/

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

On Fri, 6 Oct 2023 19:37:15 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 10:47 AM Andrew Lunn <andrew@lunn.ch> wrote:
>> > So I think that merging the patchset through a single tree is easier;
>> > netdev or rust.
>> >
>> > Miguel, how do you prefer to merge the patchset?
>>
>> What are the merge conflicts looking like? What has happened in the
>> past? [...]
> 
> Miguel has said before that if subsystems are comfortable bringing
> rust through their trees then they are welcome to do so, which helps
> get a better idea of how everything works together. If you prefer not
> to, it can come through rust-next with no problem.

It makes sense because these bindings are maintained by subsystems.

I'll send patches with 'net-next' tag in the next round.

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

[Trevor Gross]

On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);

Can you just add `An instance of a PHY` to the docs for reference?

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

The safety comment here still needs something like

    with the exception of fields that are synchronized via the `lock` mutex

> +    /// Gets the id of the PHY.
> +    pub fn phy_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,
> +        }
> +    }

Could you add a comment like `// FIXME:enum-cast` or something? Then
when we have a better solution for enums handling we can revise this.

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

Since we're taking user input, it probably doesn't hurt to do some
sort of sanity check rather than casting. Maybe warn once then return
the biggest nowrapping value

    let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
        warn_once!("excessive speed {speed}");
        i32::MAX
    })
    unsafe { (*phydev).speed = speed_i32 };

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

Andrew, are there any restrictions about calling phy_init_hw more than
once? Or are there certain things that you are not allowed to do until
you call that function?

If so, maybe a simple typestate would make sense here

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

All of these functions need a `# Safety` doc section, you could
probably just say to follow `Device::from_raw`'s rules. And then you
can update the comments to say caller guarantees preconditions

If you care to, these functions are so similar that you could just use
a macro to make your life easier

    macro_rules! make_phydev_callback{
        ($fn_name:ident, $c_fn_name:ident) => {
            /// ....
            /// # Safety
            /// `phydev` must be valid and registered
            unsafe extern "C" fn $fn_name(
                phydev: *mut ::bindings::phy_device
            ) -> $ret_ty {
                from_result(|| {
                    // SAFETY: Preconditions ensure `phydev` is valid and
                    let dev = unsafe { Device::from_raw(phydev) };
                    T::$c_fn_name(dev)?;
                    Ok(0)
                }
            }
        }
    }

    make_phydev_callback!(get_features_callback, get_features);
    make_phydev_callback!(suspend_callback, suspend);

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

Since your're reading a bus, it probably doesn't hurt to do a quick
check when converting

    let devnum_u8 = u8::try_from(devnum).(|_| {
        warn_once!("devnum {devnum} exceeds u8 limits");
        code::EINVAL
    })?
    // ...


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

Same as above with the conversion errors


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

`.cast_mut()`, just makes the mutability change more clear

I guess the C side could technically be `const char *name`

> +        // 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() }
> +    })
> +}

Btw I double checked and this should be OK to use, hopefully will be
stable in the near future
https://github.com/rust-lang/rust/pull/116218

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

Could you add information about the relationship between drivers and
device_table?

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

I can't find the discussion we had about this, but you said you have
the `type` parameter to be consistent with `module!`, correct?

I think that it is more important to be consistent with C's
`MODULE_PHY_DRIVER` where you don't need to specify anything extra,
since the module doesn't do anything else. And I think it is less
confusing for users if they don't wonder why they need to define a
type they never use.

Why not just remove the field and create an internal type based on
`name` for now? We can always make it an optional field later on if it
turns out there is a use case.

- Trevor

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

[Trevor Gross]

On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:

> 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

Maybe want to link to the C version, just for the crossref?

> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> +        dev.genphy_update_link()?;
> +        if !dev.get_link() {
> +            return Ok(0);
> +        }

Looking at this usage, I think `get_link()` should be renamed to just
`link()`. `get_link` makes me think that it is performing an action
like calling `genphy_update_link`, just `link()` sounds more like a
static accessor.

Or maybe it's worth replacing `get_link` with a `get_updated_link`
that calls `genphy_update_link` and then returns `link`, the user can
store it if they need to reuse it. This seems somewhat less accident
prone than someone calling `.link()`/`.get_link()` repeatedly and
wondering why their phy isn't coming up.

In any case, please make the docs clear about what behavior is
executed and what the preconditions are, it should be clear what's
going to wait for the bus vs. simple field access.

> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> +            dev.set_speed(100);
> +        } else {
> +            dev.set_speed(10);
> +        }

Speed should probably actually be an enum since it has defined values.
Something like

    #[non_exhaustive]
    enum Speed {
        Speed10M,
        Speed100M,
        Speed1000M,
        // 2.5G, 5G, 10G, 25G?
    }

    impl Speed {
        fn as_mb(self) -> u32;
    }


> +        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
> +            phy::DuplexMode::Full
> +        } else {
> +            phy::DuplexMode::Half
> +        };

BMCR_x and MII_x are generated as `u32` but that's just a bindgen
thing. It seems we should reexport them as the correct types so users
don't need to cast all over:

    pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
    pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
    // (I'd just make a macro for this)

But I'm not sure how to handle that since the uapi crate exposes its
bindings directly. We're probably going to run into this issue with
other uapi items at some point, any thoughts Miguel?

> +        dev.genphy_read_lpa()?;

Same question as with the `genphy_update_link`

> +    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();
> +        }
> +    }
> +}

Is it worth doing anything with these errors? I know that the C driver doesn't.

---

The overall driver looks correct to me, most of these comments are
actually about [1/3]

- Trevor

[1]: https://lore.kernel.org/rust-for-linux/baa4cc4b-bcde-4b02-9286-c923404db972@lunn.ch/

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

[FUJITA Tomonori]

On Fri, 6 Oct 2023 17:57:41 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Now I'm thinking that this is the best option. Kconfig would be the following:
>> 
>> config AX88796B_PHY
>>         tristate "Asix PHYs"
>>         help
>>          Currently supports the Asix Electronics PHY found in the X-Surf 100
>>          AX88796B package.
>> 
>> choice
>>         prompt "Implementation options"
>>         depends on AX88796B_PHY
>>         help
>>          There are two implementations for a driver for Asix PHYs; C and Rust.
>>          If not sure, choose C.
>> 
>> config AX88796B_C_PHY
>>         bool "The C version driver for Asix PHYs"
>> 
>> config AX88796B_RUST_PHY
>>         bool "The Rust version driver for Asix PHYs"
>>         depends on RUST
>> 
>> endchoice
>> 
>> 
>> No hack in Makefile:
>> 
>> obj-$(CONFIG_AX88796B_C_PHY)    += ax88796b.o
>> obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o
> 
> This looks reasonable. Lets use this. But i still think we need some
> sort of RUST_PHYLIB_BINDING.

Sorry, I found that it doesn't work well when you try to build
AX88796B_PHY as a module.

With AX88796B_PHY=m, if you choose the C version, then you got
AX88796B_C_PHY=y; the driver will be built-in.

Seems that we need a trick in Makefile in any ways, I'll go with the
original version of this patch; the simplest, I think.

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

[FUJITA Tomonori]

On Sat, 7 Oct 2023 01:06:04 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> +/// Wraps the kernel's `struct phy_device`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
> 
> Can you just add `An instance of a PHY` to the docs for reference?

You meant something like?

/// An instance of a PHY device.
/// 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() }
>> +    }
> 
> The safety comment here still needs something like
> 
>     

You meant the following?

/// 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 with the exception of fields that are
/// synchronized via the `lock` mutex.

What this means? We use Device only when an exclusive access is
gurannteed by device->lock. As discussed before, resume/suspend are
called without device->lock locked but still drivers assume an
exclusive access.


>> +    /// Gets the id of the PHY.
>> +    pub fn phy_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,
>> +        }
>> +    }
> 
> Could you add a comment like `// FIXME:enum-cast` or something? Then
> when we have a better solution for enums handling we can revise this.

Added.

>> +    /// 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 };
>> +    }
> 
> Since we're taking user input, it probably doesn't hurt to do some
> sort of sanity check rather than casting. Maybe warn once then return
> the biggest nowrapping value
> 
>     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
>         warn_once!("excessive speed {speed}");
>         i32::MAX
>     })
>     unsafe { (*phydev).speed = speed_i32 };

warn_once() is available? I was thinking about adding it after the PHY
patchset.

I'll change set_speed to return Result.

>> +    /// 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) })
>> +    }
> 
> Andrew, are there any restrictions about calling phy_init_hw more than
> once? Or are there certain things that you are not allowed to do until
> you call that function?

From quick look, you can call it multiple times.


> If so, maybe a simple typestate would make sense here
> 
>> +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)
>> +        })
>> +    }
> 
> All of these functions need a `# Safety` doc section, you could
> probably just say to follow `Device::from_raw`'s rules. And then you
> can update the comments to say caller guarantees preconditions
> 
> If you care to, these functions are so similar that you could just use
> a macro to make your life easier
> 
>     macro_rules! make_phydev_callback{
>         ($fn_name:ident, $c_fn_name:ident) => {
>             /// ....
>             /// # Safety
>             /// `phydev` must be valid and registered
>             unsafe extern "C" fn $fn_name(
>                 phydev: *mut ::bindings::phy_device
>             ) -> $ret_ty {
>                 from_result(|| {
>                     // SAFETY: Preconditions ensure `phydev` is valid and
>                     let dev = unsafe { Device::from_raw(phydev) };
>                     T::$c_fn_name(dev)?;
>                     Ok(0)
>                 }
>             }
>         }
>     }
> 
>     make_phydev_callback!(get_features_callback, get_features);
>     make_phydev_callback!(suspend_callback, suspend);

Looks nice. I use the following macro.

macro_rules! make_phydev_callback {
    ($fn_name: ident) => {
        ::kernel::macros::paste! {
            /// # Safety
            ///
            /// `phydev` must be passed by callback functions in `phy_driver`.
            unsafe extern "C" fn [<$fn_name _callback>] (
                phydev: *mut bindings::phy_device
            ) -> core::ffi::c_int {
                from_result(|| {
                    // SAFETY: Preconditions ensure `phydev` is valid.
                    let dev = unsafe { Device::from_raw(phydev) };
                    T::$fn_name(dev)?;
                    Ok(0)
                })
            }
        }
    };
}

>> +    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())
>> +        })
>> +    }
> 
> Since your're reading a bus, it probably doesn't hurt to do a quick
> check when converting
> 
>     let devnum_u8 = u8::try_from(devnum).(|_| {
>         warn_once!("devnum {devnum} exceeds u8 limits");
>         code::EINVAL
>     })?
>     // ...

Sure.

>> +    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)
>> +        })
>> +    }
> 
> Same as above with the conversion errors
> 
> 
>> +/// 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,
> 
> `.cast_mut()`, just makes the mutability change more clear

Done.

> I guess the C side could technically be `const char *name`
> 
>> +        // 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() }
>> +    })
>> +}
> 
> Btw I double checked and this should be OK to use, hopefully will be
> stable in the near future
> https://github.com/rust-lang/rust/pull/116218

Thanks!

>> +/// 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.
> 
> Could you add information about the relationship between drivers and
> device_table?

device_table needs to have PHY Ids that one of the drivers can handle.

?

>> +/// # 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",
>> +/// }
>> +/// ```
> 
> I can't find the discussion we had about this, but you said you have
> the `type` parameter to be consistent with `module!`, correct?

No, `driver!` in rust branch, which is used by platform, amba, etc.

https://github.com/Rust-for-Linux/linux/blob/rust/samples/rust/rust_platform.rs

> I think that it is more important to be consistent with C's
> `MODULE_PHY_DRIVER` where you don't need to specify anything extra,
> since the module doesn't do anything else. And I think it is less
> confusing for users if they don't wonder why they need to define a
> type they never use.
> 
> Why not just remove the field and create an internal type based on
> `name` for now? We can always make it an optional field later on if it
> turns out there is a use case.

Sure, I'll try. I have no preference and driver! macro isn't in
upstream.

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

[Greg KH]

On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote:
> > Since we're taking user input, it probably doesn't hurt to do some
> > sort of sanity check rather than casting. Maybe warn once then return
> > the biggest nowrapping value
> > 
> >     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
> >         warn_once!("excessive speed {speed}");

NEVER call WARN() on user input, as you now just rebooted the machine
and caused a DoS (and syzbot will start to spam you with reports.)

Remember, the majority of Linux systems in the world run with
panic-on-warn enabled, so any user path that can cause this, will be
used to cause problems.

thanks,

greg k-h

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

[FUJITA Tomonori]

On Sat, 7 Oct 2023 13:17:13 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote:
>> > Since we're taking user input, it probably doesn't hurt to do some
>> > sort of sanity check rather than casting. Maybe warn once then return
>> > the biggest nowrapping value
>> > 
>> >     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
>> >         warn_once!("excessive speed {speed}");
> 
> NEVER call WARN() on user input, as you now just rebooted the machine
> and caused a DoS (and syzbot will start to spam you with reports.)

Trevor uses `user` as the user of this function, which is a PHY driver.

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

[Greg KH]

On Sat, Oct 07, 2023 at 08:23:24PM +0900, FUJITA Tomonori wrote:
> On Sat, 7 Oct 2023 13:17:13 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote:
> >> > Since we're taking user input, it probably doesn't hurt to do some
> >> > sort of sanity check rather than casting. Maybe warn once then return
> >> > the biggest nowrapping value
> >> > 
> >> >     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
> >> >         warn_once!("excessive speed {speed}");
> > 
> > NEVER call WARN() on user input, as you now just rebooted the machine
> > and caused a DoS (and syzbot will start to spam you with reports.)
> 
> Trevor uses `user` as the user of this function, which is a PHY driver.
> 

Ok, same thing in a way, just do a dev_warn() and return an error, no
need to do a full traceback splat at all.

thanks,

greg k-h

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

[FUJITA Tomonori]

On Sat, 7 Oct 2023 03:19:20 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> 
>> 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
> 
> Maybe want to link to the C version, just for the crossref?

Sure.

>> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
>> +        dev.genphy_update_link()?;
>> +        if !dev.get_link() {
>> +            return Ok(0);
>> +        }
> 
> Looking at this usage, I think `get_link()` should be renamed to just
> `link()`. `get_link` makes me think that it is performing an action
> like calling `genphy_update_link`, just `link()` sounds more like a
> static accessor.

Andrew suggested to rename link() to get_link(), I think.

Then we discussed again last week:

https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/

> Or maybe it's worth replacing `get_link` with a `get_updated_link`
> that calls `genphy_update_link` and then returns `link`, the user can
> store it if they need to reuse it. This seems somewhat less accident
> prone than someone calling `.link()`/`.get_link()` repeatedly and
> wondering why their phy isn't coming up.

Once this is merged, I'll think about APIs. I need to add more
bindings anyway.


> In any case, please make the docs clear about what behavior is
> executed and what the preconditions are, it should be clear what's
> going to wait for the bus vs. simple field access.

Sure.

>> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
>> +            dev.set_speed(100);
>> +        } else {
>> +            dev.set_speed(10);
>> +        }
> 
> Speed should probably actually be an enum since it has defined values.
> Something like
> 
>     #[non_exhaustive]
>     enum Speed {
>         Speed10M,
>         Speed100M,
>         Speed1000M,
>         // 2.5G, 5G, 10G, 25G?
>     }
> 
>     impl Speed {
>         fn as_mb(self) -> u32;
>     }
> 

ethtool.h says:

/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
 * Update drivers/net/phy/phy.c:phy_speed_to_str() and
 * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
 */

I don't know there are drivers that set such values.


>> +        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
>> +            phy::DuplexMode::Full
>> +        } else {
>> +            phy::DuplexMode::Half
>> +        };
> 
> BMCR_x and MII_x are generated as `u32` but that's just a bindgen
> thing. It seems we should reexport them as the correct types so users
> don't need to cast all over:
> 
>     pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
>     pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
>     // (I'd just make a macro for this)
> 
> But I'm not sure how to handle that since the uapi crate exposes its
> bindings directly. We're probably going to run into this issue with
> other uapi items at some point, any thoughts Miguel?

reexporting all the BMCR_ values by hand doesn't sound fun. Can we
automaticall generate such?

>> +        dev.genphy_read_lpa()?;
> 
> Same question as with the `genphy_update_link`
> 
>> +    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();
>> +        }
>> +    }
>> +}
> 
> Is it worth doing anything with these errors? I know that the C driver doesn't.

I'll check out what other drivers do in the similar situations.


> The overall driver looks correct to me, most of these comments are
> actually about [1/3]

Thanks!

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

[Andrew Lunn]

> > +    /// 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 };
> > +    }
> 
> Since we're taking user input, it probably doesn't hurt to do some
> sort of sanity check rather than casting. Maybe warn once then return
> the biggest nowrapping value

After reading the thread, we first have a terminology problem. In the
kernel world, 'user input' generally means from user space. And user
space should never be trusted, but user space should also not be
allowed to bring the system to its knees. Return -EINVAL to userspace
is the correct thing to do and keep going. Don't do a kernel splat
because the user passed 42 as a speed, not 10.

However, what Trevor was meaning is that whoever called set_speed()
passed an invalid value. But what are valid values?

We have this file which devices the KAPI
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1883

says:

/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.

and we also have

#define SPEED_UNKNOWN		-1

and there is a handy little helper:

static inline int ethtool_validate_speed(__u32 speed)
{
	return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;
}

so if you want to validate speed, call this helper.

However, this is a kernel driver, and we generally trust kernel
drivers. There is not much we can do except trust them. And passing an
invalid speed here is very unlikely to cause the kernel to explode
sometime later.

   Andrew

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

[Andrew Lunn]

> The safety comment here still needs something like
> 
>     with the exception of fields that are synchronized via the `lock` mutex

I'm not sure that really adds much useful information. Which values
are protected by the lock? More importantly, which are not protected
by the lock?

As a general rule of thumb, driver writers don't understand
locking. Yes, there are some which do, but many don't. So the
workaround to that is make it so they don't need to understand
locking. All the locking happens in the core.

The exception is suspend and resume, which are called without the
lock. So if i was to add a comment about locking, i would only put a
comment on those two.

> > +    /// 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) })
> > +    }
> 
> Andrew, are there any restrictions about calling phy_init_hw more than
> once? Or are there certain things that you are not allowed to do until
> you call that function?

phy_init_hw can be called multiple times. It used by drivers as a work
around to broken hardware/firmware to get the device back into a good
state. It is also used during resume, since often the PHY looses its
settings when suspended.

> > +    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())
> > +        })
> > +    }
> 
> Since your're reading a bus, it probably doesn't hurt to do a quick
> check when converting
> 
>     let devnum_u8 = u8::try_from(devnum).(|_| {
>         warn_once!("devnum {devnum} exceeds u8 limits");
>         code::EINVAL
>     })?

I would actually say this is the wrong place to do that. Such checks
should happen in the core, so it checks all drivers, not just the
current one Rust driver. Feel free to submit a C patch adding this.

	Andrew

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

[Andrew Lunn]

On Sat, Oct 07, 2023 at 03:19:20AM -0400, Trevor Gross wrote:
> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> 
> > 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
> 
> Maybe want to link to the C version, just for the crossref?
> 
> > +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> > +        dev.genphy_update_link()?;
> > +        if !dev.get_link() {
> > +            return Ok(0);
> > +        }
> 
> Looking at this usage, I think `get_link()` should be renamed to just
> `link()`. `get_link` makes me think that it is performing an action
> like calling `genphy_update_link`, just `link()` sounds more like a
> static accessor.

Naming is hard, and i had the exact opposite understanding.

The rust binding seems to impose getter/setters on members of
phydev. So my opinion was, using get_/set_ makes it clear this is just
a dumb getter/setter, and nothing more.

> Or maybe it's worth replacing `get_link` with a `get_updated_link`
> that calls `genphy_update_link` and then returns `link`, the user can
> store it if they need to reuse it. This seems somewhat less accident
> prone than someone calling `.link()`/`.get_link()` repeatedly and
> wondering why their phy isn't coming up.

You have to be very careful with reading the link state. It is latched
low. Meaning if the link is dropped and then comes back again, the
first read of the link will tell you it went away, and the second read
will give you current status. The core expects the driver to read the
link state only once, when asked what is the state of the link, so it
gets informed about this short link down events.

> In any case, please make the docs clear about what behavior is
> executed and what the preconditions are, it should be clear what's
> going to wait for the bus vs. simple field access.
> 
> > +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> > +            dev.set_speed(100);
> > +        } else {
> > +            dev.set_speed(10);
> > +        }
> 
> Speed should probably actually be an enum since it has defined values.
> Something like
> 
>     #[non_exhaustive]
>     enum Speed {
>         Speed10M,
>         Speed100M,
>         Speed1000M,
>         // 2.5G, 5G, 10G, 25G?
>     }

This beings us back to how do you make use of C #defines. All the
values defined here are theoretically valid:

https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1887

#define SPEED_10		10
#define SPEED_100		100
#define SPEED_1000		1000
#define SPEED_2500		2500
#define SPEED_5000		5000
#define SPEED_10000		10000
#define SPEED_14000		14000
#define SPEED_20000		20000
#define SPEED_25000		25000
#define SPEED_40000		40000
#define SPEED_50000		50000
#define SPEED_56000		56000
#define SPEED_100000		100000
#define SPEED_200000		200000
#define SPEED_400000		400000
#define SPEED_800000		800000

and more speeds keep getting added.

Also, the kAPI actually would allow the value 42, not that any
hardware i know of actually supports that.

> > +    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();
> > +        }
> > +    }
> > +}
> 
> Is it worth doing anything with these errors? I know that the C driver doesn't.

You could do a phydev_err(). But if these fail, the hardware is dead,
and there is not much you can do about that.

    Andrew

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

[Andrew Lunn]

> reexporting all the BMCR_ values by hand doesn't sound fun. Can we
> automaticall generate such?

The C22 address space only have a max of 32, and no more are expected.

C45 address space can have in theory 32 x 65536, although in practice
it is sparsely populated. But new values are added every so often. So
generated at build time from the #defines would be better.

	  Andrew

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

[FUJITA Tomonori]

On Sat, 07 Oct 2023 19:58:57 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

>>> +/// # 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",
>>> +/// }
>>> +/// ```
>> 
>> I can't find the discussion we had about this, but you said you have
>> the `type` parameter to be consistent with `module!`, correct?
> 
> No, `driver!` in rust branch, which is used by platform, amba, etc.
> 
> https://github.com/Rust-for-Linux/linux/blob/rust/samples/rust/rust_platform.rs
> 
>> I think that it is more important to be consistent with C's
>> `MODULE_PHY_DRIVER` where you don't need to specify anything extra,
>> since the module doesn't do anything else. And I think it is less
>> confusing for users if they don't wonder why they need to define a
>> type they never use.
>> 
>> Why not just remove the field and create an internal type based on
>> `name` for now? We can always make it an optional field later on if it
>> turns out there is a use case.
> 
> Sure, I'll try. I have no preference and driver! macro isn't in
> upstream.

To create an internal type based on `name`, we need to unstringify
`name`? I can't find a easy way to do it.

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 6:59 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> > Can you just add `An instance of a PHY` to the docs for reference?
>
> You meant something like?
>
> /// An instance of a PHY device.
> /// 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>);

Seems good to me. I know that largely we want users to refer to the C
docs, but I think a small hint is good.

Fwiw they can be on the same line, Markdown combines them

> >> +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() }
> >> +    }
> >
> > The safety comment here still needs something like
>
> You meant the following?
>
> /// 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 with the exception of fields that are
> /// synchronized via the `lock` mutex.
>
> What this means? We use Device only when an exclusive access is
> gurannteed by device->lock. As discussed before, resume/suspend are
> called without device->lock locked but still drivers assume an
> exclusive access.

This was in follow up to one of the notes on the RFC patches, I'll
reply more to Andrew's comment about this

>
> >> +    /// Sets the speed of the PHY.
> >> +    pub fn set_speed(&mut self, speed: u32) {
> >> +        let phydev = self.0.get();
> >> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> >> +        unsafe { (*phydev).speed = speed as i32 };
> >> +    }
> >
> > Since we're taking user input, it probably doesn't hurt to do some
> > sort of sanity check rather than casting. Maybe warn once then return
> > the biggest nowrapping value
> >
> >     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
> >         warn_once!("excessive speed {speed}");
> >         i32::MAX
> >     })
> >     unsafe { (*phydev).speed = speed_i32 };
>
> warn_once() is available? I was thinking about adding it after the PHY
> patchset.
>
> I'll change set_speed to return Result.

Nevermind, I guess we don't have `warn_once`. Andrew mentioned
something about potentially lossy conversions, I'll follow up there.

> >> +    /// 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) })
> >> +    }
> >
> > Andrew, are there any restrictions about calling phy_init_hw more than
> > once? Or are there certain things that you are not allowed to do until
> > you call that function?
>
> From quick look, you can call it multiple times.

Thanks, no worries in that case


> > If so, maybe a simple typestate would make sense here
> >
> >> +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)
> >> +        })
> >> +    }
> >
> > All of these functions need a `# Safety` doc section, you could
> > probably just say to follow `Device::from_raw`'s rules. And then you
> > can update the comments to say caller guarantees preconditions
> >
> > If you care to, these functions are so similar that you could just use
> > a macro to make your life easier
> >
> >     macro_rules! make_phydev_callback{
> >         ($fn_name:ident, $c_fn_name:ident) => {
> >             /// ....
> >             /// # Safety
> >             /// `phydev` must be valid and registered
> >             unsafe extern "C" fn $fn_name(
> >                 phydev: *mut ::bindings::phy_device
> >             ) -> $ret_ty {
> >                 from_result(|| {
> >                     // SAFETY: Preconditions ensure `phydev` is valid and
> >                     let dev = unsafe { Device::from_raw(phydev) };
> >                     T::$c_fn_name(dev)?;
> >                     Ok(0)
> >                 }
> >             }
> >         }
> >     }
> >
> >     make_phydev_callback!(get_features_callback, get_features);
> >     make_phydev_callback!(suspend_callback, suspend);
>
> Looks nice. I use the following macro.

Miguel mentioned on Zulip that we try to avoid macros (I did not know
this), so I guess it's fine if redundant.

https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.60bool.3A.3Athen.60.20helper/near/395398830

> >> +/// 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.
> >
> > Could you add information about the relationship between drivers and
> > device_table?
>
> device_table needs to have PHY Ids that one of the drivers can handle.
>
> ?

I think something like "Every driver needs an entry in device_table"
is fine, just make it less easy to miss

Thanks for the followup, all of these were very minor

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 10:48 AM Andrew Lunn <andrew@lunn.ch> wrote:
> After reading the thread, we first have a terminology problem. In the
> kernel world, 'user input' generally means from user space. And user
> space should never be trusted, but user space should also not be
> allowed to bring the system to its knees. Return -EINVAL to userspace
> is the correct thing to do and keep going. Don't do a kernel splat
> because the user passed 42 as a speed, not 10.

Yes, sorry about the terminology. I was indeed referring to a user of
this function and am still figuring out the failure modes (thanks also
Greg for the corrections).

> However, what Trevor was meaning is that whoever called set_speed()
> passed an invalid value. But what are valid values?
>
> We have this file which devices the KAPI
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1883
>
> says:
>
> /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>
> and we also have
>
> #define SPEED_UNKNOWN           -1
>
> and there is a handy little helper:
>
> static inline int ethtool_validate_speed(__u32 speed)
> {
>         return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;
> }
>
> so if you want to validate speed, call this helper.
>
> However, this is a kernel driver, and we generally trust kernel
> drivers. There is not much we can do except trust them. And passing an
> invalid speed here is very unlikely to cause the kernel to explode
> sometime later.
>
>    Andrew

I didn't mean the suggestion to be a way of handling untrusted input,
just a means of providing a fallback value with module author feedback
if something > INT_MAX winds up getting passed somehow.

Agreed that this is more than necessary for such a trivial situation,
the original is of course fine.

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > The safety comment here still needs something like
> >
> >     with the exception of fields that are synchronized via the `lock` mutex
>
> I'm not sure that really adds much useful information. Which values
> are protected by the lock? More importantly, which are not protected
> by the lock?
>
> As a general rule of thumb, driver writers don't understand
> locking. Yes, there are some which do, but many don't. So the
> workaround to that is make it so they don't need to understand
> locking. All the locking happens in the core.
>
> The exception is suspend and resume, which are called without the
> lock. So if i was to add a comment about locking, i would only put a
> comment on those two.

This doesn't get used by driver implementations, it's only used within
the abstractions here. I think anyone who needs the details can refer
to the C side, I just suggested to note the locking caveat based on
your second comment at
https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/

Fujita - since this doesn't get exposed, could this be pub(crate)?)

> > Andrew, are there any restrictions about calling phy_init_hw more than
> > once? Or are there certain things that you are not allowed to do until
> > you call that function?
>
> phy_init_hw can be called multiple times. It used by drivers as a work
> around to broken hardware/firmware to get the device back into a good
> state. It is also used during resume, since often the PHY looses its
> settings when suspended.

Great, thank you for the clarification

> > > +    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())
> > > +        })
> > > +    }
> >
> > Since your're reading a bus, it probably doesn't hurt to do a quick
> > check when converting
> >
> >     let devnum_u8 = u8::try_from(devnum).(|_| {
> >         warn_once!("devnum {devnum} exceeds u8 limits");
> >         code::EINVAL
> >     })?
>
> I would actually say this is the wrong place to do that. Such checks
> should happen in the core, so it checks all drivers, not just the
> current one Rust driver. Feel free to submit a C patch adding this.
>
>         Andrew

I guess it does that already:
https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556

Fujita, I think we started doing comments when we know that
lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
but add

    // CAST: the C side verifies devnum < 32

?

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 6:33 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> To create an internal type based on `name`, we need to unstringify
> `name`? I can't find a easy way to do it.

I think you should just be able to do it with `paste!`

    macro_rules! module_phy_driver {
        (name: $name:expr) => {
            paste::paste! {
                #[allow(non_camel_case_types)]
                struct [<$name _ty>];
            }
        }
    }

    // creates struct `demo_driver_ty`
    module_phy_driver! {
        name: "demo_driver"
    }

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 8:10 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> >> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> >> +        dev.genphy_update_link()?;
> >> +        if !dev.get_link() {
> >> +            return Ok(0);
> >> +        }
> >
> > Looking at this usage, I think `get_link()` should be renamed to just
> > `link()`. `get_link` makes me think that it is performing an action
> > like calling `genphy_update_link`, just `link()` sounds more like a
> > static accessor.
>
> Andrew suggested to rename link() to get_link(), I think.
>
> Then we discussed again last week:
>
> https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/

Thanks for the link, in that case LGTM

>
> >> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> >> +            dev.set_speed(100);
> >> +        } else {
> >> +            dev.set_speed(10);
> >> +        }
> >
> > Speed should probably actually be an enum since it has defined values.
> > Something like
> >
> >     #[non_exhaustive]
> >     enum Speed {
> >         Speed10M,
> >         Speed100M,
> >         Speed1000M,
> >         // 2.5G, 5G, 10G, 25G?
> >     }
> >
> >     impl Speed {
> >         fn as_mb(self) -> u32;
> >     }
> >
>
> ethtool.h says:
>
> /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>  * Update drivers/net/phy/phy.c:phy_speed_to_str() and
>  * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
>  */
>
> I don't know there are drivers that set such values.

Andrew replied to this too and an enum wouldn't work. Maybe good to
add uapi/linux/ethtool.h to the bindings and use the SPEED_X defined
there?

> >> +        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
> >> +            phy::DuplexMode::Full
> >> +        } else {
> >> +            phy::DuplexMode::Half
> >> +        };
> >
> > BMCR_x and MII_x are generated as `u32` but that's just a bindgen
> > thing. It seems we should reexport them as the correct types so users
> > don't need to cast all over:
> >
> >     pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
> >     pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
> >     // (I'd just make a macro for this)
> >
> > But I'm not sure how to handle that since the uapi crate exposes its
> > bindings directly. We're probably going to run into this issue with
> > other uapi items at some point, any thoughts Miguel?
>
> reexporting all the BMCR_ values by hand doesn't sound fun. Can we
> automaticall generate such?

Definitely not by hand, I don't think bindgen allows finer control
over what types are created from `#define` yet. I am not sure what our
policy is on build scripts but the below would work:

    # repeat this with a different prefix (BMCR) and type (u16) as needed
    perl -ne 'print if
s/^#define\s+(BMCR\w+)\s+([0-9xX]+)\s+(?:\/\*(.*)\*\/)?/\/\/\/ \3\npub
const \1: u16 = \2;/' include/uapi/linux/mii.h > somefile.rs

That creates outputs

    ///  MSB of Speed (1000)
    pub const BMCR_SPEED1000: u16 = 0x0040;
    ///  Collision test
    pub const BMCR_CTST: u16 = 0x0080;
    ///  Full duplex
    pub const BMCR_FULLDPLX: u16 = 0x0100;

Miguel, any suggestions here?

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

[Trevor Gross]

On Sat, Oct 7, 2023 at 11:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > Looking at this usage, I think `get_link()` should be renamed to just
> > `link()`. `get_link` makes me think that it is performing an action
> > like calling `genphy_update_link`, just `link()` sounds more like a
> > static accessor.
>
> Naming is hard, and i had the exact opposite understanding.
>
> The rust binding seems to impose getter/setters on members of
> phydev. So my opinion was, using get_/set_ makes it clear this is just
> a dumb getter/setter, and nothing more.
>
> > Or maybe it's worth replacing `get_link` with a `get_updated_link`
> > that calls `genphy_update_link` and then returns `link`, the user can
> > store it if they need to reuse it. This seems somewhat less accident
> > prone than someone calling `.link()`/`.get_link()` repeatedly and
> > wondering why their phy isn't coming up.
>
> You have to be very careful with reading the link state. It is latched
> low. Meaning if the link is dropped and then comes back again, the
> first read of the link will tell you it went away, and the second read
> will give you current status. The core expects the driver to read the
> link state only once, when asked what is the state of the link, so it
> gets informed about this short link down events.

Thanks for the clarification, I agree that it makes sense as-is then.

> > In any case, please make the docs clear about what behavior is
> > executed and what the preconditions are, it should be clear what's
> > going to wait for the bus vs. simple field access.
> >
> > > +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> > > +            dev.set_speed(100);
> > > +        } else {
> > > +            dev.set_speed(10);
> > > +        }
> >
> > Speed should probably actually be an enum since it has defined values.
> > Something like
> >
> >     #[non_exhaustive]
> >     enum Speed {
> >         Speed10M,
> >         Speed100M,
> >         Speed1000M,
> >         // 2.5G, 5G, 10G, 25G?
> >     }
>
> This beings us back to how do you make use of C #defines. All the
> values defined here are theoretically valid:
>
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1887
>
> #define SPEED_10                10
> #define SPEED_100               100
> #define SPEED_1000              1000
> #define SPEED_2500              2500
> #define SPEED_5000              5000
> #define SPEED_10000             10000
> #define SPEED_14000             14000
> #define SPEED_20000             20000
> #define SPEED_25000             25000
> #define SPEED_40000             40000
> #define SPEED_50000             50000
> #define SPEED_56000             56000
> #define SPEED_100000            100000
> #define SPEED_200000            200000
> #define SPEED_400000            400000
> #define SPEED_800000            800000
>
> and more speeds keep getting added.
>
> Also, the kAPI actually would allow the value 42, not that any
> hardware i know of actually supports that.

The enum doesn't make sense but maybe we should just generate bindings
for these defines? I suggested that in my reply to Fujita.

- Trevor

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

[FUJITA Tomonori]

On Sun, 8 Oct 2023 02:19:46 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sat, Oct 7, 2023 at 6:33 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> To create an internal type based on `name`, we need to unstringify
>> `name`? I can't find a easy way to do it.
> 
> I think you should just be able to do it with `paste!`
> 
>     macro_rules! module_phy_driver {
>         (name: $name:expr) => {
>             paste::paste! {
>                 #[allow(non_camel_case_types)]
>                 struct [<$name _ty>];
>             }
>         }
>     }
> 
>     // creates struct `demo_driver_ty`
>     module_phy_driver! {
>         name: "demo_driver"
>     }
> 

I realized that we don't need `name`. The name of struct doesn't
matter so I use `Module`. I tried to use `name` for the name of
device_table however the variable name of the table isn't embeded into
the module binary so it doesn't matter.

FYI, I use paste! but got the following error:

= help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier
= note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the
  macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)


#[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:tt, [$($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),+], name: $name:tt, $($f:tt)*) => {
        struct Module {
            _reg: kernel::net::phy::Registration,
        }

        $crate::prelude::module! {
             type: Module,
             name: $name,
             $($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 {
            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,
                })
            }
        }

        $crate::module_phy_driver!(@device_table $name, [$($dev),+]);
    }
}

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

[Trevor Gross]

On Sun, Oct 8, 2023 at 3:49 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> I realized that we don't need `name`. The name of struct doesn't
> matter so I use `Module`. I tried to use `name` for the name of
> device_table however the variable name of the table isn't embeded into
> the module binary so it doesn't matter.
>
> FYI, I use paste! but got the following error:
>
> = help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier
> = note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the
>   macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)

Too bad, this seems to be a limitation of our paste macro compared to
the published one. What you have with `Module` seems fine so I think
you don't need it?

I'll submit a patch to fix paste sometime this week, don't wait on it
though of course.

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

[FUJITA Tomonori]

On Sun, 8 Oct 2023 04:54:52 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sun, Oct 8, 2023 at 3:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> I realized that we don't need `name`. The name of struct doesn't
>> matter so I use `Module`. I tried to use `name` for the name of
>> device_table however the variable name of the table isn't embeded into
>> the module binary so it doesn't matter.
>>
>> FYI, I use paste! but got the following error:
>>
>> = help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier
>> = note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the
>>   macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)
> 
> Too bad, this seems to be a limitation of our paste macro compared to
> the published one. What you have with `Module` seems fine so I think
> you don't need it?

Yeah, now I don't use paste! in PHY bindings.

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

[Trevor Gross]

On Sun, Oct 8, 2023 at 5:03 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Yeah, now I don't use paste! in PHY bindings.

Even better!

The fix was small so I sent it anyway,
https://lore.kernel.org/rust-for-linux/20231008094816.320424-1-tmgross@umich.edu/T/#u

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

[FUJITA Tomonori]

On Sun, 8 Oct 2023 02:07:44 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> > The safety comment here still needs something like
>> >
>> >     with the exception of fields that are synchronized via the `lock` mutex
>>
>> I'm not sure that really adds much useful information. Which values
>> are protected by the lock? More importantly, which are not protected
>> by the lock?
>>
>> As a general rule of thumb, driver writers don't understand
>> locking. Yes, there are some which do, but many don't. So the
>> workaround to that is make it so they don't need to understand
>> locking. All the locking happens in the core.
>>
>> The exception is suspend and resume, which are called without the
>> lock. So if i was to add a comment about locking, i would only put a
>> comment on those two.
> 
> This doesn't get used by driver implementations, it's only used within
> the abstractions here. I think anyone who needs the details can refer
> to the C side, I just suggested to note the locking caveat based on
> your second comment at
> https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/
> 
> Fujita - since this doesn't get exposed, could this be pub(crate)?)

Device? I don't think so. If we make Device pub(crate), we need to
make trait Driver pub(crate) too.


>> > > +    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())
>> > > +        })
>> > > +    }
>> >
>> > Since your're reading a bus, it probably doesn't hurt to do a quick
>> > check when converting
>> >
>> >     let devnum_u8 = u8::try_from(devnum).(|_| {
>> >         warn_once!("devnum {devnum} exceeds u8 limits");
>> >         code::EINVAL
>> >     })?
>>
>> I would actually say this is the wrong place to do that. Such checks
>> should happen in the core, so it checks all drivers, not just the
>> current one Rust driver. Feel free to submit a C patch adding this.
>>
>>         Andrew
> 
> I guess it does that already:
> https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556
> 
> Fujita, I think we started doing comments when we know that
> lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
> but add
> 
>     // CAST: the C side verifies devnum < 32

Ok. As I commented on the RFC reviewing, I don't think that we
need try_from conversion for values from PHYLIB. Implementing bindings
for untrusted stuff doesn't make sense.

https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/

On the other hand, I think that it might worth to use try_from for
set_speed() because its about the bindings and Rust PHY drivers.
However, I leave it alone since likely setting a wrong value doesn't
break anything.

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

[Trevor Gross]

On Sun, Oct 8, 2023 at 10:28 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> > Fujita - since this doesn't get exposed, could this be pub(crate)?)
>
> Device? I don't think so. If we make Device pub(crate), we need to
> make trait Driver pub(crate) too.

Not `Device`, just `pub fn from_raw`. I think it is fine as-is (I see
you sent a new version)

> > Fujita, I think we started doing comments when we know that
> > lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
> > but add
> >
> >     // CAST: the C side verifies devnum < 32
>
> Ok. As I commented on the RFC reviewing, I don't think that we
> need try_from conversion for values from PHYLIB. Implementing bindings
> for untrusted stuff doesn't make sense.
>
> https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/
>
> On the other hand, I think that it might worth to use try_from for
> set_speed() because its about the bindings and Rust PHY drivers.
> However, I leave it alone since likely setting a wrong value doesn't
> break anything.

Agreed, thanks for the followup

Re: [PATCH v2 0/3] Rust abstractions for network PHY drivers

[Miguel Ojeda]

On Fri, Oct 6, 2023 at 2:54 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> This is described here, along with other useful hits for working with
> netdev.
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Off-topic suggestion: the `.rst` file could be linked as the `P:`
field in `MAINTAINERS`, perhaps with some tweaks.

> I don't know if it made the wrong decision based on the missing tag,
> or it simply does not know what to do with Rust yet.

How does it usually determine the tree otherwise (if the tree is not
in the subject)?

> There is also the question of how we merge this. Does it all come
> through netdev? Do we split the patches, the abstraction merged via
> rust and the rest via netdev? Is the Kconfig sufficient that if a tree
> only contains patches 2 and 3 it does not allow the driver to be
> enabled?

Ideally, everything goes through the subsystem's tree whenever they
feel ready to do so. The idea is that maintainers get involved and
handle their Rust code as any other code. Trees like Kbuild, KUnit and
Workqueue have started taking things, for instance, which is great
(and we appreciate it).

Having said that, I would recommend caution -- I would wait until a
few more people from the Rust subsystem give their `Reviewed-by`. In
particular, I would wait until Wedson has given it another look at
least, since he has had the most experience developing networking
abstractions.

I mention this because what we are trying to achieve with the Rust
abstractions is not just functional equivalence to the C side, but
also to make them "sound". In Rust, "sound" means the safe APIs cannot
possibly introduce UB on their own.

Moreover, as I said elsewhere, I do not agree with the
`--rustified-enum` change in the series, given the UB risk (see the
previous paragraph). If we really want to go with that, then we should
be very explicit about the fact that we are placing an assumption on
the C side here.

Cheers,
Miguel