[PATCH net-next v3 0/3] Rust abstractions for network PHY drivers

[PATCH net-next v3 0/3] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

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

The first patch introduces Rust bindings for phylib.

The second patch updates the ETHERNET PHY LIBRARY entry in MAINTAINERS
file; adds the binding file and 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.

v3:
  - changes the base tree to net-next from rust-next
  - makes this feature optional; only enabled with CONFIG_RUST_PHYLIB_BINDINGS=y
  - cosmetic code and comment improvement
  - adds copyright
v2: https://lore.kernel.org/netdev/20231006094911.3305152-2-fujita.tomonori@gmail.com/T/
  - build failure fix
  - function renaming
v1: https://lore.kernel.org/netdev/20231002085302.2274260-3-fujita.tomonori@gmail.com/T/


FUJITA Tomonori (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                     |   8 +
 rust/Makefile                    |   1 +
 rust/bindings/bindings_helper.h  |   3 +
 rust/kernel/lib.rs               |   3 +
 rust/kernel/net.rs               |   6 +
 rust/kernel/net/phy.rs           | 733 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 11 files changed, 899 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: 19537e125cc7cf2da43a606f5bcebbe0c9aea4cc
-- 
2.34.1

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

[FUJITA Tomonori]

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

This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 init/Kconfig                    |   8 +
 rust/Makefile                   |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 733 ++++++++++++++++++++++++++++++++
 6 files changed, 754 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..7ea415c9b144 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1903,6 +1903,14 @@ config RUST
 
 	  If unsure, say N.
 
+config RUST_PHYLIB_BINDINGS
+        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.
+
 config RUSTC_VERSION_TEXT
 	string
 	depends on RUST
diff --git a/rust/Makefile b/rust/Makefile
index 87958e864be0..f67e55945b36 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
       cmd_bindgen = \
 	$(BINDGEN) $< $(bindgen_target_flags) \
 		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
+		--rustified-enum phy_state\
 		--no-debug '.*' \
 		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
 		$(bindgen_target_cflags) $(bindgen_target_extra)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c91a3c24f607..ec4ee09a34ad 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,9 @@
 
 #include <kunit/test.h>
 #include <linux/errname.h>
+#include <linux/ethtool.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e8811700239a..0588422e273c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -14,6 +14,7 @@
 #![no_std]
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
+#![feature(const_maybe_uninit_zeroed)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(receiver_trait)]
@@ -36,6 +37,8 @@
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..cc1de17cd5fa
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_RUST_PHYLIB_BINDINGS)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..f31983bf0460
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,733 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::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,
+}
+
+/// 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() }
+    }
+
+    /// 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 };
+        // FIXME: enum-cast
+        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> {
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: Preconditions ensure `phydev` is valid.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            // CAST: the C side verifies devnum < 32.
+            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
+            Ok(ret.into())
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum as u8, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: Preconditions ensure `phydev` is valid.
+        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().cast_mut(),
+        flags: T::FLAGS,
+        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
+        soft_reset: if T::HAS_SOFT_RESET {
+            Some(Adapter::<T>::soft_reset_callback)
+        } else {
+            None
+        },
+        get_features: if T::HAS_GET_FEATURES {
+            Some(Adapter::<T>::get_features_callback)
+        } else {
+            None
+        },
+        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
+            Some(Adapter::<T>::match_phy_device_callback)
+        } else {
+            None
+        },
+        suspend: if T::HAS_SUSPEND {
+            Some(Adapter::<T>::suspend_callback)
+        } else {
+            None
+        },
+        resume: if T::HAS_RESUME {
+            Some(Adapter::<T>::resume_callback)
+        } else {
+            None
+        },
+        config_aneg: if T::HAS_CONFIG_ANEG {
+            Some(Adapter::<T>::config_aneg_callback)
+        } else {
+            None
+        },
+        read_status: if T::HAS_READ_STATUS {
+            Some(Adapter::<T>::read_status_callback)
+        } else {
+            None
+        },
+        read_mmd: if T::HAS_READ_MMD {
+            Some(Adapter::<T>::read_mmd_callback)
+        } else {
+            None
+        },
+        write_mmd: if T::HAS_WRITE_MMD {
+            Some(Adapter::<T>::write_mmd_callback)
+        } else {
+            None
+        },
+        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
+            Some(Adapter::<T>::link_change_notify_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+    })
+}
+
+/// 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. Every driver needs an entry in 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>()
+///     ],
+///     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 [$($dev:expr),+]) => {
+        #[no_mangle]
+        static __mod_mdio__phydev_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),+], $($f:tt)*) => {
+        struct Module {
+            _reg: kernel::net::phy::Registration,
+        }
+
+        $crate::prelude::module! {
+             type: Module,
+             $($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 [$($dev),+]);
+    }
+}
-- 
2.34.1

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

[Trevor Gross]

On Sun, Oct 8, 2023 at 9:41 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---

Thanks for all the work Fujita, the rust side looks good to me here!

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

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

[Benno Lossin]

On 09.10.23 03:39, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>   init/Kconfig                    |   8 +
>   rust/Makefile                   |   1 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 733 ++++++++++++++++++++++++++++++++
>   6 files changed, 754 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..7ea415c9b144 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1903,6 +1903,14 @@ config RUST
> 
>   	  If unsure, say N.
> 
> +config RUST_PHYLIB_BINDINGS
> +        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.
> +
>   config RUSTC_VERSION_TEXT
>   	string
>   	depends on RUST
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
>         cmd_bindgen = \
>   	$(BINDGEN) $< $(bindgen_target_flags) \
>   		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> +		--rustified-enum phy_state\
>   		--no-debug '.*' \
>   		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
>   		$(bindgen_target_cflags) $(bindgen_target_extra)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
> 
>   #include <kunit/test.h>
>   #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
>   #include <linux/slab.h>
>   #include <linux/refcount.h>
>   #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
>   #![no_std]
>   #![feature(allocator_api)]
>   #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
>   #![feature(dispatch_from_dyn)]
>   #![feature(new_uninit)]
>   #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
>   pub mod ioctl;
>   #[cfg(CONFIG_KUNIT)]
>   pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>   pub mod prelude;
>   pub mod print;
>   mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..cc1de17cd5fa
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_RUST_PHYLIB_BINDINGS)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..f31983bf0460
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,733 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::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,
> +}
> +
> +/// 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() }

Missing `SAFETY` comment.

> +    }
> +
> +    /// 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 };
> +        // FIXME: enum-cast
> +        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 {

I would call this function `is_link_up`.

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

Can you move the call to `link` and the `==` operation out
of the `unsafe` block? They are safe operations. (also do
that below where possible)

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

Just a general question, all of these unsafe calls do not have
additional requirements? So aside from the pointers being
valid, there are no timing/locking/other safety requirements
for calling the functions?

> +        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> {
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn soft_reset_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::soft_reset(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn get_features_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::get_features(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::suspend(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::resume(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn config_aneg_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::config_aneg(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn read_status_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::read_status(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn match_phy_device_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: Preconditions ensure `phydev` is valid.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::match_phy_device(dev) as i32
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn read_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            // CAST: the C side verifies devnum < 32.
> +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> +            Ok(ret.into())
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn write_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +        val: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::write_mmd(dev, devnum as u8, regnum, val)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> +        // SAFETY: Preconditions ensure `phydev` is valid.
> +        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`.

Missing '`'.

> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
> +    Opaque::new(bindings::phy_driver {
> +        name: T::NAME.as_char_ptr().cast_mut(),
> +        flags: T::FLAGS,
> +        phy_id: T::PHY_DEVICE_ID.id,
> +        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
> +        soft_reset: if T::HAS_SOFT_RESET {
> +            Some(Adapter::<T>::soft_reset_callback)
> +        } else {
> +            None
> +        },
> +        get_features: if T::HAS_GET_FEATURES {
> +            Some(Adapter::<T>::get_features_callback)
> +        } else {
> +            None
> +        },
> +        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
> +            Some(Adapter::<T>::match_phy_device_callback)
> +        } else {
> +            None
> +        },
> +        suspend: if T::HAS_SUSPEND {
> +            Some(Adapter::<T>::suspend_callback)
> +        } else {
> +            None
> +        },
> +        resume: if T::HAS_RESUME {
> +            Some(Adapter::<T>::resume_callback)
> +        } else {
> +            None
> +        },
> +        config_aneg: if T::HAS_CONFIG_ANEG {
> +            Some(Adapter::<T>::config_aneg_callback)
> +        } else {
> +            None
> +        },
> +        read_status: if T::HAS_READ_STATUS {
> +            Some(Adapter::<T>::read_status_callback)
> +        } else {
> +            None
> +        },
> +        read_mmd: if T::HAS_READ_MMD {
> +            Some(Adapter::<T>::read_mmd_callback)
> +        } else {
> +            None
> +        },
> +        write_mmd: if T::HAS_WRITE_MMD {
> +            Some(Adapter::<T>::write_mmd_callback)
> +        } else {
> +            None
> +        },
> +        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
> +            Some(Adapter::<T>::link_change_notify_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
> +        // sets `Option<&F>` to be `None`.
> +        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> +    })
> +}
> +
> +/// 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`.

Since it is a reference you do not need to explicitly state
that it points to an array of `struct phy_driver`. Instead I would
suggest the following invariant:

All elements of the `drivers` slice are valid and currently registered
to the kernel via `phy_drivers_register`.

> +pub struct Registration {
> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,

Why is this an `Option`?

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

This `register` function seems to assume that the values of the
`drivers` array are initialized and otherwise also considered valid.
So please change that or make this function `unsafe`.

> +        Ok(Registration {

Please add an `INVARIANT` comment similar to a `SAFETY` comment
that explains why the invariant is upheld.

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

Question: is it ok for two different threads to call `phy_drivers_register`
and `phy_drivers_unregister`? If no, then `Send` must not be implemented.

> +
> +// 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. Every driver needs an entry in 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>()
> +///     ],
> +///     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 [$($dev:expr),+]) => {
> +        #[no_mangle]
> +        static __mod_mdio__phydev_device_table: [

Shouldn't this have a unique name? If we define two different
phy drivers with this macro we would have a symbol collision?

> +            kernel::bindings::mdio_device_id;

Please use absolute paths in macros:
`::kernel::bindings::mdio_device_id` (also below).

> +            $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),+], $($f:tt)*) => {
> +        struct Module {
> +            _reg: kernel::net::phy::Registration,
> +        }
> +
> +        $crate::prelude::module! {
> +             type: Module,
> +             $($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.

In order for this SAFETY comment to be correct, you need to ensure
that nobody else can access the `DRIVERS` static. You can do that by
placing both the `static mut DRIVERS` and the `impl ::kernel::Module
for Module` items inside of a `const _: () = {}`, so like this:

     const _: () = {
         static mut DRIVERS: [...] = ...;
         impl ::kernel::Module for Module { ... }
     };

You can also mention this in the SAFETY comment.

> +                let mut reg = unsafe { kernel::net::phy::Registration::register(module, &DRIVERS) }?;
> +
> +                Ok(Module {
> +                    _reg: reg,
> +                })
> +            }
> +        }
> +
> +        $crate::module_phy_driver!(@device_table [$($dev),+]);
> +    }
> +}
> --
> 2.34.1
> 
> 

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 12:19:54PM +0000, Benno Lossin wrote:
> > +impl Device {
> > +    /// Creates a new [`Device`] instance from a raw pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > +    /// may read or write to the `phy_device` object.
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> > +        unsafe { &mut *ptr.cast() }
> 
> Missing `SAFETY` comment.

Hi Benno

It is normal on Linux kernel mailing lists to trim the text to what is
just relevant to the reply. Comments don't get lost that way.

> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&mut self) -> bool {
> 
> I would call this function `is_link_up`.

Please read the discussion on the previous versions of this patch. If
you still want to change the name, please give a justification.

> > +    /// 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())
> > +        };
> 
> Just a general question, all of these unsafe calls do not have
> additional requirements? So aside from the pointers being
> valid, there are no timing/locking/other safety requirements
> for calling the functions?

Locking has been discussed a number of times already. What do you mean
by timing?

> > +// SAFETY: `Registration` does not expose any of its state across threads.
> > +unsafe impl Send for Registration {}
> 
> Question: is it ok for two different threads to call `phy_drivers_register`
> and `phy_drivers_unregister`? If no, then `Send` must not be implemented.

The core phy_drivers_register() is thread safe. It boils down to a
driver_register() which gets hammered every kernel boot, so locking
issues would soon be found there.

> > +macro_rules! module_phy_driver {
> > +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> > +
> > +    (@count_devices $($x:expr),*) => {
> > +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> > +    };
> > +
> > +    (@device_table [$($dev:expr),+]) => {
> > +        #[no_mangle]
> > +        static __mod_mdio__phydev_device_table: [
> 
> Shouldn't this have a unique name? If we define two different
> phy drivers with this macro we would have a symbol collision?

I assume these are the equivalent of C static. It is not visible
outside the scope of this object file. The kernel has lots of tables
and they are mostly of very limited visibility scope, because only the
method registering/unregistering the table needs to see it.

       Andrew

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

[Benno Lossin]

On 09.10.23 15:02, Andrew Lunn wrote:
> On Mon, Oct 09, 2023 at 12:19:54PM +0000, Benno Lossin wrote:
>>> +impl Device {
>>> +    /// Creates a new [`Device`] instance from a raw pointer.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>>> +    /// may read or write to the `phy_device` object.
>>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> +        unsafe { &mut *ptr.cast() }
>>
>> Missing `SAFETY` comment.
> 
> Hi Benno
> 
> It is normal on Linux kernel mailing lists to trim the text to what is
> just relevant to the reply. Comments don't get lost that way.

Sure, I will keep it in mind.

> 
>>> +    /// Returns true if the link is up.
>>> +    pub fn get_link(&mut self) -> bool {
>>
>> I would call this function `is_link_up`.
> 
> Please read the discussion on the previous versions of this patch. If
> you still want to change the name, please give a justification.

As Fujita wrote in [1], `get_foo` is not really common in Rust.
And here it seems weird to, since the return type is a bool. To
me `get_foo` means "fetch me a foo" and that is not what this
function is doing. Also given the documentation explicitly states
"Returns true if the link is up", I think that `is_link_up` or similar
fits very well.

>>> +    /// 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())
>>> +        };
>>
>> Just a general question, all of these unsafe calls do not have
>> additional requirements? So aside from the pointers being
>> valid, there are no timing/locking/other safety requirements
>> for calling the functions?
> 
> Locking has been discussed a number of times already. What do you mean
> by timing?

A few different things:
- atomic/raw atomic context
- another function has to be called prior

I have limited knowledge of the C side and have cannot sift through
the C code for every `unsafe` function call. Just wanted to ensure
that someone has checked that these safety requirements are exhaustive.

>>> +macro_rules! module_phy_driver {
>>> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
>>> +
>>> +    (@count_devices $($x:expr),*) => {
>>> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>>> +    };
>>> +
>>> +    (@device_table [$($dev:expr),+]) => {
>>> +        #[no_mangle]
>>> +        static __mod_mdio__phydev_device_table: [
>>
>> Shouldn't this have a unique name? If we define two different
>> phy drivers with this macro we would have a symbol collision?
> 
> I assume these are the equivalent of C static. It is not visible
> outside the scope of this object file. The kernel has lots of tables
> and they are mostly of very limited visibility scope, because only the
> method registering/unregistering the table needs to see it.
The `#[no_mangle]` attribute in Rust disables standard symbol name
mangling, see [2]. So if this macro is invoked twice, it will result
in a compile error.

[1]: https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/
[2]: https://doc.rust-lang.org/reference/abi.html#the-no_mangle-attribute

--
Cheers,
Benno

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

[Andrew Lunn]

> > Locking has been discussed a number of times already. What do you mean
> > by timing?
> 
> A few different things:
> - atomic/raw atomic context

PHY drivers need to access a slow MDIO bus, so you are always in
thread context which can sleep. Even the interrupt handler is in
thread context, and the device lock is held.

> >>> +macro_rules! module_phy_driver {
> >>> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> >>> +
> >>> +    (@count_devices $($x:expr),*) => {
> >>> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> >>> +    };
> >>> +
> >>> +    (@device_table [$($dev:expr),+]) => {
> >>> +        #[no_mangle]
> >>> +        static __mod_mdio__phydev_device_table: [
> >>
> >> Shouldn't this have a unique name? If we define two different
> >> phy drivers with this macro we would have a symbol collision?
> > 
> > I assume these are the equivalent of C static. It is not visible
> > outside the scope of this object file. The kernel has lots of tables
> > and they are mostly of very limited visibility scope, because only the
> > method registering/unregistering the table needs to see it.
> The `#[no_mangle]` attribute in Rust disables standard symbol name
> mangling, see [2]. So if this macro is invoked twice, it will result
> in a compile error.

Invoked twice in what context? Within one object file? That i would
say is O.K. In practice, you only every have one table per driver.

As i said, i expect these symbols are static, so not seen outside the
object file. So if it is involved twice by different PHY drivers, that
should not matter, they are not global symbols, so the linker will not
complain about them. Also, in the Linux world, symbols are not visible
outside of a kernel module unless there is an EXPORT_SYMBOL_GPL() on
the symbol. So even if two kernel drivers do have global scope tables
with the same name, they are still invisible to each other when built
into the kernel, or loaded at runtime.

       Andrew

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

[FUJITA Tomonori]

On Mon, 09 Oct 2023 12:19:54 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

I skipped the topics that you've already discussed with Andrew.

> On 09.10.23 03:39, FUJITA Tomonori wrote:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>> 
>> This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>   init/Kconfig                    |   8 +
>>   rust/Makefile                   |   1 +
>>   rust/bindings/bindings_helper.h |   3 +
>>   rust/kernel/lib.rs              |   3 +
>>   rust/kernel/net.rs              |   6 +
>>   rust/kernel/net/phy.rs          | 733 ++++++++++++++++++++++++++++++++
>>   6 files changed, 754 insertions(+)
>>   create mode 100644 rust/kernel/net.rs
>>   create mode 100644 rust/kernel/net/phy.rs

(snip)

>> +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() }
> 
> Missing `SAFETY` comment.

Added:

// SAFETY: The safety requirements guarantee the validity of the dereference, while the
// `Device` type being transparent makes the cast ok.


>> +    /// 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 };
>> +        // FIXME: enum-cast
>> +        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 {
> 
> I would call this function `is_link_up`.
> 
>> +        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 }
> 
> Can you move the call to `link` and the `==` operation out
> of the `unsafe` block? They are safe operations. (also do
> that below where possible)

Sure, fixed.


>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> 
> Missing '`'.

Fixed.


>> +/// 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`.
> 
> Since it is a reference you do not need to explicitly state
> that it points to an array of `struct phy_driver`. Instead I would
> suggest the following invariant:
> 
> All elements of the `drivers` slice are valid and currently registered
> to the kernel via `phy_drivers_register`.

Surely, makes sense. 


>> +pub struct Registration {
>> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> 
> Why is this an `Option`?

Oops, removed; leftover of older version.


>> +}
>> +
>> +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)
>> +        })?;
> 
> This `register` function seems to assume that the values of the
> `drivers` array are initialized and otherwise also considered valid.
> So please change that or make this function `unsafe`.

Understood.


>> +        Ok(Registration {
> 
> Please add an `INVARIANT` comment similar to a `SAFETY` comment
> that explains why the invariant is upheld.

Added.


>> +#[macro_export]
>> +macro_rules! module_phy_driver {
>> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
>> +
>> +    (@count_devices $($x:expr),*) => {
>> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> +    };
>> +
>> +    (@device_table [$($dev:expr),+]) => {
>> +        #[no_mangle]
>> +        static __mod_mdio__phydev_device_table: [
> 
> Shouldn't this have a unique name? If we define two different
> phy drivers with this macro we would have a symbol collision?
> 
>> +            kernel::bindings::mdio_device_id;
> 
> Please use absolute paths in macros:
> `::kernel::bindings::mdio_device_id` (also below).

Updated.


>> +            $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),+], $($f:tt)*) => {
>> +        struct Module {
>> +            _reg: kernel::net::phy::Registration,
>> +        }
>> +
>> +        $crate::prelude::module! {
>> +             type: Module,
>> +             $($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.
> 
> In order for this SAFETY comment to be correct, you need to ensure
> that nobody else can access the `DRIVERS` static. You can do that by
> placing both the `static mut DRIVERS` and the `impl ::kernel::Module
> for Module` items inside of a `const _: () = {}`, so like this:
> 
>      const _: () = {
>          static mut DRIVERS: [...] = ...;
>          impl ::kernel::Module for Module { ... }
>      };
> 
> You can also mention this in the SAFETY comment.

Great, that's exactly what to be needed here. Thanks a lot!

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

[Miguel Ojeda]

Hi Tomonori,

A few nits I noticed. Please note that this is not really a full
review, and that I recommend that other people like Wedson should take
a look again and OK these abstractions before this is merged.

On Mon, Oct 9, 2023 at 3:41 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +config RUST_PHYLIB_BINDINGS

This should be called ABSTRACTIONS. Please see:

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

Also, could this symbol go elsewhere?

> +        bool "PHYLIB bindings support"

Ditto.

> +          a wrapper around the C phlib core.

Typo.

> +               --rustified-enum phy_state\

As I said elsewhere, we should avoid `--rustified-enum` due tot he
risk of UB unless we are explicit on the assumptions we are placing on
the C side.

> +#![feature(const_maybe_uninit_zeroed)]

The patch message should justify this addition and warn about it.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..f31983bf0460
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,733 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>

Newline missing.

> +    /// Full-duplex mode

Please use the style of the rest of the Rust comments.

> +/// An instance of a PHY device.
> +/// Wraps the kernel's `struct phy_device`.

That should be separated.

> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else

Missing Markdown around the lifetime.

> +        // FIXME: enum-cast

Please explain what needs to be fixed.

> +    /// Executes software reset the PHY via BMCR_RESET bit.

Markdown missing (multiple instances).

> +    /// Reads Link partner ability.

Why is "link" capitalized here?

> +/// Creates the kernel's `phy_driver` instance.
> +///
> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.

Broken formatting? Does `rustdoc` complain about it?

> +/// The `drivers` points to an array of `struct phy_driver`, which is
> +/// registered to the kernel via `phy_drivers_register`.

Perhaps "The `drivers` field"?

> +            // SAFETY: The type invariants guarantee that self.drivers is valid.

Markdown.

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

It would be nice to explain why the field is `pub`.

> +    /// Get a mask as u32.

Markdown.

This patch could be split a bit too, but that is up to the maintainers.

> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver` and registers it.

"kernel's" or similar

> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information

Markdown.

> +/// for module loading into the module binary file. Every driver needs an entry in device_table.

Markdown.

> +/// # 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>()
> +///     ],
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +/// ```

Please add an example above with the expansion of the macro so that it
is easy to understand at a glance, see e.g. what Benno did in
`pin-init` (`rust/init*`).

Also, perhaps splitting the patches into a few would help.

Cheers,
Miguel

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 14:59:19 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> A few nits I noticed. Please note that this is not really a full
> review, and that I recommend that other people like Wedson should take
> a look again and OK these abstractions before this is merged.

We have about two weeks before the merge window opens? It would great
if other people could review really soon.

We can improve the abstractions after it's merged. This patchset
doesn't add anything exported to users. This adds only one driver so
the APIs can be fixed anytime.

Once it's merged, multiple people can send patches easily, so more
scalable.

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 3:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> We have about two weeks before the merge window opens? It would great
> if other people could review really soon.
>
> We can improve the abstractions after it's merged. This patchset
> doesn't add anything exported to users. This adds only one driver so
> the APIs can be fixed anytime.
>
> Once it's merged, multiple people can send patches easily, so more
> scalable.

I think it is too soon to merge it unless you get some more reviews.

On the other hand, I agree iterating in-tree is easier.

If you want to merge it very soon, I would suggest
considering/evaluating the following:

  - Please consider marking the driver as a "Rust reference driver"
[1] that is not meant to be used (yet, at least) in production. That
would probably be the best signal, and everybody is clear on the
expectations.

  - Otherwise, please consider marking it as staging/experimental for
the time being. That allows you to iterate the abstractions at your
own pace. Of course, it still risks somebody out-of-tree using them,
but see the next points.

  - Should fixes to the code be considered actual fixes and sent to
stable? If we do one of the above, I guess you could simply say the
code is in development.

  - Similarly, what about Rust unsoundness issues? We do want to
consider those as stable-worthy patches even if they may not be "real"
security issues, and just "potential" ones. We did submit an stable
patch in the past for one of those.

[1] https://lore.kernel.org/ksummit/CANiq72=99VFE=Ve5MNM9ZuSe9M-JSH1evk6pABNSEnNjK7aXYA@mail.gmail.com/

Cheers,
Miguel

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 16:32:42 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Mon, Oct 9, 2023 at 3:49 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> We have about two weeks before the merge window opens? It would great
>> if other people could review really soon.
>>
>> We can improve the abstractions after it's merged. This patchset
>> doesn't add anything exported to users. This adds only one driver so
>> the APIs can be fixed anytime.
>>
>> Once it's merged, multiple people can send patches easily, so more
>> scalable.
> 
> I think it is too soon to merge it unless you get some more reviews.
> 
> On the other hand, I agree iterating in-tree is easier.
> 
> If you want to merge it very soon, I would suggest
> considering/evaluating the following:

It's up to PHY maintainers. I prefer that the patchset are merged very
soon. Much easier to improve the code in tree.


>   - Please consider marking the driver as a "Rust reference driver"
> [1] that is not meant to be used (yet, at least) in production. That
> would probably be the best signal, and everybody is clear on the
> expectations.

Of course. I would be very surprised if someone think that a Rust
driver is ready for production because Rust support is an
experiment.

How I can mark the driver as a "Rust reference driver"? Kconfig
description?


>   - Otherwise, please consider marking it as staging/experimental for
> the time being. That allows you to iterate the abstractions at your
> own pace. Of course, it still risks somebody out-of-tree using them,
> but see the next points.
> 
>   - Should fixes to the code be considered actual fixes and sent to
> stable? If we do one of the above, I guess you could simply say the
> code is in development.
> 
>   - Similarly, what about Rust unsoundness issues? We do want to
> consider those as stable-worthy patches even if they may not be "real"
> security issues, and just "potential" ones. We did submit an stable
> patch in the past for one of those.
> 
> [1] https://lore.kernel.org/ksummit/CANiq72=99VFE=Ve5MNM9ZuSe9M-JSH1evk6pABNSEnNjK7aXYA@mail.gmail.com/

If a driver is marked as a reference driver, we don't need to think
about "stable" stuff, right?

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:15 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> It's up to PHY maintainers. I prefer that the patchset are merged very
> soon. Much easier to improve the code in tree.

Yes, sorry, my message was meant for Andrew.

Cheers,
Miguel

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

[Greg KH]

On Mon, Oct 09, 2023 at 10:49:07PM +0900, FUJITA Tomonori wrote:
> On Mon, 9 Oct 2023 14:59:19 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> > A few nits I noticed. Please note that this is not really a full
> > review, and that I recommend that other people like Wedson should take
> > a look again and OK these abstractions before this is merged.
> 
> We have about two weeks before the merge window opens? It would great
> if other people could review really soon.
> 
> We can improve the abstractions after it's merged. This patchset
> doesn't add anything exported to users. This adds only one driver so
> the APIs can be fixed anytime.

There is no rush, or deadline here.  Take the time to get it in proper
shape first please.

thanks,

greg k-h

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 17:11:51 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 09, 2023 at 10:49:07PM +0900, FUJITA Tomonori wrote:
>> On Mon, 9 Oct 2023 14:59:19 +0200
>> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>> 
>> > A few nits I noticed. Please note that this is not really a full
>> > review, and that I recommend that other people like Wedson should take
>> > a look again and OK these abstractions before this is merged.
>> 
>> We have about two weeks before the merge window opens? It would great
>> if other people could review really soon.
>> 
>> We can improve the abstractions after it's merged. This patchset
>> doesn't add anything exported to users. This adds only one driver so
>> the APIs can be fixed anytime.
> 
> There is no rush, or deadline here.  Take the time to get it in proper
> shape first please.

Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
that we have been discussing the same topics like locking, naming, etc
again and again.

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:24 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
> that we have been discussing the same topics like locking, naming, etc
> again and again.

Well, there was other feedback too, which isn't addressed so far. So
merging this in 2 weeks does seem a bit rushed to me.

And, yes, the discussion on this has been going around for a while,
but that is precisely why we recommended to iterate more on our side
first, because it was not ready.

Cheers,
Miguel

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 17:39:27 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Mon, Oct 9, 2023 at 5:24 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
>> that we have been discussing the same topics like locking, naming, etc
>> again and again.
> 
> Well, there was other feedback too, which isn't addressed so far. So
> merging this in 2 weeks does seem a bit rushed to me.

What feedback? enum stuff? I think that it's a long-term issue. 

> And, yes, the discussion on this has been going around for a while,
> but that is precisely why we recommended to iterate more on our side
> first, because it was not ready.

I'm not sure about it. For example, we reviewed the locking issue
three times. It can't be reviewed only on Rust side. It's mainly about
how the C side works.

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:50 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> What feedback? enum stuff? I think that it's a long-term issue.

Not just that. There has been other feedback, and since this message,
we got new reviews too.

But, yes, the `--rustified-enum` is one of those. I am still
uncomfortable with it. It is not a huge deal for a while, and things
will work, and the risk of UB is low. But why do we want to risk it?
The point of using Rust is precisely to avoid this sort of thing.

Why cannot we use one of the alternatives? If we really want to catch,
right now, the "addition of new variant in the C enum" case, cannot we
add a temporary check for that? e.g. it occurs to me we could make
`bindgen` generate the `--rustified-enum` into a temporary file and
compile a fixed `match` somewhere or something like that, for the
purposes of checking. That way we avoid the UB in the actual code.

But the best would be to work on adding to `bindgen` something like
the `--safe-rustified-enum` I suggested (because we already know the
maintainers find the idea reasonable -- thanks Trevor for creating the
issue!), even if only to validate the idea with a prototype.

In short, what is the rush?

> I'm not sure about it. For example, we reviewed the locking issue
> three times. It can't be reviewed only on Rust side. It's mainly about
> how the C side works.

We have never said it has to be reviewed only on the Rust side. In
fact, our instructions for contributing explain very clearly the
opposite:

    https://rust-for-linux.com/contributing#the-rust-subsystem

The instructions also say that the code must be warning-free and so
on, and yet after several iterations and pushing for merging several
times, there are still "surface-level" things like missing `// SAFETY`
comments and `bindings::` in public APIs; which we consider very
important -- we want to get them enforced by the compiler in the
future.

Not only that, when I saw Wedson mentioning yesterday the
`#[must_use]` bit, I wondered how this was even not being noticed by
the compiler.

So I just took the v3 patches and compiled them and, indeed, Clippy gives you:

    error: this function has an empty `#[must_use]` attribute, but
returns a type already marked as `#[must_use]`
    --> rust/kernel/net/phy.rs:547:5
        |
    547 | /     pub fn register(
    548 | |         module: &'static crate::ThisModule,
    549 | |         drivers: &'static [Opaque<bindings::phy_driver>],
    550 | |     ) -> Result<Self> {
        | |_____________________^
        |
        = help: either add some descriptive text or remove the attribute
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#double_must_use
        = note: `-D clippy::double-must-use` implied by `-D clippy::style`

    error: length comparison to zero
    --> rust/kernel/net/phy.rs:551:12
        |
    551 |         if drivers.len() == 0 {
        |            ^^^^^^^^^^^^^^^^^^ help: using `is_empty` is
clearer and more explicit: `drivers.is_empty()`
        |
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
        = note: `-D clippy::len-zero` implied by `-D clippy::style`

    error: methods called `as_*` usually take `self` by reference or
`self` by mutable reference
    --> rust/kernel/net/phy.rs:642:21
        |
    642 |     const fn as_int(self) -> u32 {
        |                     ^^^^
        |
        = help: consider choosing a less ambiguous name
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
        = note: `-D clippy::wrong-self-convention` implied by `-D clippy::style`

And from `rustdoc`:

    error: unresolved link to `module_phy_driver`
    --> rust/kernel/net/phy.rs:408:23
        |
    408 | /// This is used by [`module_phy_driver`] macro to create a
static array of phy_driver`.
        |                       ^^^^^^^^^^^^^^^^^ no item named
`module_phy_driver` in scope
        |
        = note: `macro_rules` named `module_phy_driver` exists in this
crate, but it is not in scope at this link's location
        = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`

    error: unresolved link to `PHY_DEVICE_ID`
    --> rust/kernel/net/phy.rs:494:52
        |
    494 |     /// If not implemented, matching is based on [`PHY_DEVICE_ID`].
        |
^^^^^^^^^^^^^ no item named `PHY_DEVICE_ID` in scope
        |
        = help: to escape `[` and `]` characters, add '\' before them
like `\[` or `\]`

So, no, it is not ready for merge. Yes, those things above are
trivial, but fixing them is also trivial, and after several revisions
it has not been done. And this sort of thing should be done before
even submitting the very first version.

Cheers,
Miguel

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

[FUJITA Tomonori]

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

> On Mon, Oct 9, 2023 at 5:50 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> What feedback? enum stuff? I think that it's a long-term issue.
> 
> Not just that. There has been other feedback, and since this message,
> we got new reviews too.
> 
> But, yes, the `--rustified-enum` is one of those. I am still
> uncomfortable with it. It is not a huge deal for a while, and things
> will work, and the risk of UB is low. But why do we want to risk it?
> The point of using Rust is precisely to avoid this sort of thing.
>
> Why cannot we use one of the alternatives? If we really want to catch,
> right now, the "addition of new variant in the C enum" case, cannot we
> add a temporary check for that? e.g. it occurs to me we could make

IIRC, Andrew prefers to avoid creating a temporary rust variant (Greg
does too, I understand). I guess that only solusion that both Rust and
C devs would be happy with is generating safe Rust code from C. The
solution is still a prototype and I don't know when it will be
available (someone knows?).

I think that unlikely PHYLIB's state machine would be broken, so I
chose that approach with the code commented.


>> I'm not sure about it. For example, we reviewed the locking issue
>> three times. It can't be reviewed only on Rust side. It's mainly about
>> how the C side works.
> 
> We have never said it has to be reviewed only on the Rust side. In
> fact, our instructions for contributing explain very clearly the
> opposite:
> 
>     https://rust-for-linux.com/contributing#the-rust-subsystem
> 
> The instructions also say that the code must be warning-free and so
> on, and yet after several iterations and pushing for merging several
> times, there are still "surface-level" things like missing `// SAFETY`
> comments and `bindings::` in public APIs; which we consider very
> important -- we want to get them enforced by the compiler in the
> future.
> 
> Not only that, when I saw Wedson mentioning yesterday the
> `#[must_use]` bit, I wondered how this was even not being noticed by
> the compiler.
> 
> So I just took the v3 patches and compiled them and, indeed, Clippy gives you:

Sorry, there's no excuse. I should have done better. I'll make sure
that the code is warning-free.

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

[Miguel Ojeda]

On Thu, Oct 12, 2023 at 1:18 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> IIRC, Andrew prefers to avoid creating a temporary rust variant (Greg
> does too, I understand). I guess that only solusion that both Rust and
> C devs would be happy with is generating safe Rust code from C. The

As far as I understand, the workaround I just suggested in the
previous reply was not discussed so far. I am not sure which of the
alternatives you mean by the "temporary rust variant", so I may be
misunderstanding your message.

> solution is still a prototype and I don't know when it will be
> available (someone knows?).

If no alternative is good enough, and you do not have time to
implement one of the better solutions, then we need to wait until one
of us (or somebody else) implements it. I understand that can be
frustrating, but we cannot really agree to start using
`--rustified-enum` or, in general, ways to introduce UB where we
already have known solutions.

Instead, we prefer to spend some time iterating on this sort of
problem. It is also not the first time at all we have done this, e.g.
see `pin-init`. It is all about trying to avoid compromising, unless
the solution is really far away.

Having said that, to try to unblock things, I spent some time
prototyping the workaround I suggested, see below [1]. That catches
the "new C variant added" desync between Rust and C.

For instance, if I add a `PHY_NEW` variant, then I get:

    error[E0005]: refutable pattern in function argument
         --> rust/bindings/bindings_enum_check.rs:29:6
          |
    29    |       (phy_state::PHY_DOWN
          |  ______^
    30    | |     | phy_state::PHY_READY
    31    | |     | phy_state::PHY_HALTED
    32    | |     | phy_state::PHY_ERROR
    ...     |
    35    | |     | phy_state::PHY_NOLINK
    36    | |     | phy_state::PHY_CABLETEST): phy_state,
          | |______________________________^ pattern
`phy_state::PHY_NEW` not covered
          |
    note: `phy_state` defined here
         --> rust/bindings/bindings_generated_enum_check.rs:60739:10
          |
    60739 | pub enum phy_state {
          |          ^^^^^^^^^
    ...
    60745 |     PHY_NEW = 5,
          |     ------- not covered
          = note: the matched value is of type `phy_state`

It seems to work fine and would allow us to use the wildcard `_`
without risk of desync, and without needing changes on the C enum.

> Sorry, there's no excuse. I should have done better. I'll make sure
> that the code is warning-free.

No problem at all -- it is all fine. I hope the workaround is suitable
and unblocks you. Please let me know and I can send it as a patch.

However, I would still prefer that it is done in `bindgen` -- when we
talked about that possibility in the weekly meeting a couple days ago,
we thought it could be doable to have it ready for the next kernel
version if somebody steps up to do it now. I could do it, but not
before LPC.

Cheers,
Miguel

[1]

diff --git a/rust/.gitignore b/rust/.gitignore
index d3829ffab80b..1a76ad0d6603 100644
--- a/rust/.gitignore
+++ b/rust/.gitignore
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0

 bindings_generated.rs
+bindings_generated_enum_check.rs
 bindings_helpers_generated.rs
 doctests_kernel_generated.rs
 doctests_kernel_generated_kunit.c
diff --git a/rust/Makefile b/rust/Makefile
index 87958e864be0..4a1c7a48dfad 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -15,6 +15,7 @@ always-$(CONFIG_RUST) += libmacros.so
 no-clean-files += libmacros.so

 always-$(CONFIG_RUST) += bindings/bindings_generated.rs
bindings/bindings_helpers_generated.rs
+always-$(CONFIG_RUST) += bindings/bindings_generated_enum_check.rs
 obj-$(CONFIG_RUST) += alloc.o bindings.o kernel.o
 always-$(CONFIG_RUST) += exports_alloc_generated.h
exports_bindings_generated.h \
     exports_kernel_generated.h
@@ -341,6 +342,19 @@ $(obj)/bindings/bindings_generated.rs:
$(src)/bindings/bindings_helper.h \
     $(src)/bindgen_parameters FORCE
        $(call if_changed_dep,bindgen)

+$(obj)/bindings/bindings_generated_enum_check.rs: private
bindgen_target_flags = \
+    $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters) \
+    --default-enum-style rust
+$(obj)/bindings/bindings_generated_enum_check.rs: private
bindgen_target_extra = ; \
+    OBJTREE=$(abspath $(objtree)) $(RUSTC_OR_CLIPPY) $(rust_flags)
$(rustc_target_flags) \
+        --crate-type rlib -L$(objtree)/$(obj) \
+        --emit=dep-info=$(obj)/bindings/.bindings_enum_check.rs.d \
+        --emit=metadata=$(obj)/bindings/libbindings_enum_check.rmeta \
+        --crate-name enum_check $(obj)/bindings/bindings_enum_check.rs
+$(obj)/bindings/bindings_generated_enum_check.rs:
$(src)/bindings/bindings_helper.h \
+    $(src)/bindings/bindings_enum_check.rs $(src)/bindgen_parameters FORCE
+       $(call if_changed_dep,bindgen)
+
 $(obj)/uapi/uapi_generated.rs: private bindgen_target_flags = \
     $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters)
 $(obj)/uapi/uapi_generated.rs: $(src)/uapi/uapi_helper.h \
diff --git a/rust/bindings/bindings_enum_check.rs
b/rust/bindings/bindings_enum_check.rs
new file mode 100644
index 000000000000..7c62bab12ea1
--- /dev/null
+++ b/rust/bindings/bindings_enum_check.rs
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bindings exhaustiveness enum check.
+//!
+//! Eventually, this should be replaced by a safe version of
`--rustified-enum`, see
+//! https://github.com/rust-lang/rust-bindgen/issues/2646.
+
+#![no_std]
+#![allow(
+    clippy::all,
+    dead_code,
+    missing_docs,
+    non_camel_case_types,
+    non_upper_case_globals,
+    non_snake_case,
+    improper_ctypes,
+    unreachable_pub,
+    unsafe_op_in_unsafe_fn
+)]
+
+include!(concat!(
+    env!("OBJTREE"),
+    "/rust/bindings/bindings_generated_enum_check.rs"
+));
+
+fn check_phy_state(
+    (phy_state::PHY_DOWN
+    | phy_state::PHY_READY
+    | phy_state::PHY_HALTED
+    | phy_state::PHY_ERROR
+    | phy_state::PHY_UP
+    | phy_state::PHY_RUNNING
+    | phy_state::PHY_NOLINK
+    | phy_state::PHY_CABLETEST): phy_state,
+) {
+}

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

[FUJITA Tomonori]

On Fri, 13 Oct 2023 13:59:12 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Oct 12, 2023 at 1:18 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> IIRC, Andrew prefers to avoid creating a temporary rust variant (Greg
>> does too, I understand). I guess that only solusion that both Rust and
>> C devs would be happy with is generating safe Rust code from C. The
> 
> As far as I understand, the workaround I just suggested in the
> previous reply was not discussed so far. I am not sure which of the
> alternatives you mean by the "temporary rust variant", so I may be
> misunderstanding your message.

I meant that defining Rust's enum corresponding to the kernel's enum
phy_state like.

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

Then write match code by hand.

I'll leave it to PHYLIB maintainers. The subsystem maintainers decide
whether they merges the code.

Andrew, what do you think about the status of the abstraction patchset?


> Having said that, to try to unblock things, I spent some time
> prototyping the workaround I suggested, see below [1]. That catches
> the "new C variant added" desync between Rust and C.

Thanks, but we have to maintain the following code by hand? if so,
the maintanace nightmare problem isn't solved?


btw, I can't apply the patch, line wrapping?

> new file mode 100644
> index 000000000000..7c62bab12ea1
> --- /dev/null
> +++ b/rust/bindings/bindings_enum_check.rs
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bindings exhaustiveness enum check.
> +//!
> +//! Eventually, this should be replaced by a safe version of
> `--rustified-enum`, see
> +//! https://github.com/rust-lang/rust-bindgen/issues/2646.
> +
> +#![no_std]
> +#![allow(
> +    clippy::all,
> +    dead_code,
> +    missing_docs,
> +    non_camel_case_types,
> +    non_upper_case_globals,
> +    non_snake_case,
> +    improper_ctypes,
> +    unreachable_pub,
> +    unsafe_op_in_unsafe_fn
> +)]
> +
> +include!(concat!(
> +    env!("OBJTREE"),
> +    "/rust/bindings/bindings_generated_enum_check.rs"
> +));
> +
> +fn check_phy_state(
> +    (phy_state::PHY_DOWN
> +    | phy_state::PHY_READY
> +    | phy_state::PHY_HALTED
> +    | phy_state::PHY_ERROR
> +    | phy_state::PHY_UP
> +    | phy_state::PHY_RUNNING
> +    | phy_state::PHY_NOLINK
> +    | phy_state::PHY_CABLETEST): phy_state,
> +) {
> +}
> 

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

[Miguel Ojeda]

On Fri, Oct 13, 2023 at 5:15 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I meant that defining Rust's enum corresponding to the kernel's enum
> phy_state like.
>
> +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,
> +}
>
> Then write match code by hand.

Yes, but that alone is not enough -- that is what we do normally, but
we can still diverge with the C side. That is what the `bindgen`
proposal would solve (plus better maintenance). The workaround also
solves that, but with more maintenance effort. We could even go
further, but I don't think it is worth it given that we really want to
have it in `bindgen`.

> I'll leave it to PHYLIB maintainers. The subsystem maintainers decide
> whether they merges the code.

Indeed, but the "no `--rustified-enum`" is a whole kernel policy we
want to keep, i.e. we are NAK'ing that small bit because we want to a
solution that does not introduce silent UB if a non-local mistake is
made.

> Thanks, but we have to maintain the following code by hand? if so,
> the maintanace nightmare problem isn't solved?

That is correct, but that requires extra work on `bindgen`. What we
can ensure with he workaround is that it does not get our of sync (in
terms of the variants).

If Andrew prefers to wait for a proper `bindgen` solution, that is
fine with us; i.e. what we are only saying is "no, please" to the
`--rustified-enum` approach.

Also please note that there is still the question about the docs on
the generated `enum`, even with the current `bindgen` proposal in
place.

> btw, I can't apply the patch, line wrapping?

Yes, I just copy pasted it in Gmail to showcase the idea. I have
pushed it here in case you want to play with it:
https://github.com/ojeda/linux/tree/rust-bindgen-workaround

Cheers,
Miguel

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

[FUJITA Tomonori]

On Fri, 13 Oct 2023 20:33:58 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Fri, Oct 13, 2023 at 5:15 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> I meant that defining Rust's enum corresponding to the kernel's enum
>> phy_state like.
>>
>> +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,
>> +}
>>
>> Then write match code by hand.
> 
> Yes, but that alone is not enough -- that is what we do normally, but
> we can still diverge with the C side. That is what the `bindgen`
> proposal would solve (plus better maintenance). The workaround also
> solves that, but with more maintenance effort. We could even go
> further, but I don't think it is worth it given that we really want to
> have it in `bindgen`.

Now there is separate entry for PHYLIB [RUST] in MAINTAINERS so it
makes clear that I have to do more maintenance effort. So hopefully,
it's fine by Andrew.

I'll use your workaround in the next version.

I expect that you don't want a commit introducing UD so I squash your
patch with the explanation to the commit log. If you want to merge
your work to the patchset in a different way, please let me know.

Thanks a lot!

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

[Miguel Ojeda]

On Sat, Oct 14, 2023 at 2:31 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I expect that you don't want a commit introducing UD so I squash your
> patch with the explanation to the commit log. If you want to merge
> your work to the patchset in a different way, please let me know.

No, that is all wrong.

Your commits are not introducing UB (at least regarding this topic /
that we know about), whether you use the workaround or not, and
whether you use `--rustified-enum` or not:

  - Using `--rustified-enum` does not introduce UB on its own. It does
introduce a *risk* of UB, which is why we do not want it.

  - The workaround is meant to avoid desync between the C enum and the
Rust `match`, i.e. forgetting to update the Rust side. It has nothing
to do with UB.

Furthermore, no, you should not squash the code into one of your
commits. It is not OK to do that, and even if it were, you would not
need to do so. Instead, you could put the feature into its own commit
(but the patch is still a WIP, I have not sent it formally yet), if
you want to showcase how it would work.

In any case, we have not even discussed/decided whether to use the
workaround. I sent it mainly for your benefit, so that you can show
the netdev maintainer(s) that it would be possible to keep C and Rust
enums in sync, so that hopefully their concern is gone.

Cheers,
Miguel

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

[FUJITA Tomonori]

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

> On Mon, Oct 9, 2023 at 5:50 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> What feedback? enum stuff? I think that it's a long-term issue.
> 
> Not just that. There has been other feedback, and since this message,
> we got new reviews too.
> 
> But, yes, the `--rustified-enum` is one of those. I am still
> uncomfortable with it. It is not a huge deal for a while, and things
> will work, and the risk of UB is low. But why do we want to risk it?
> The point of using Rust is precisely to avoid this sort of thing.

Possibly, I don't correctly understand what your risk means.

You are talking about the risk of UB, which happens when PHYLIB sets a
random value to the state enum, right? It only happens when PHYLIB has
a bug. If PHYLIB has such bug, likely the NIC doesn't work, the user
would report the system failure.

In such situation, a Rust PHY driver can find that bug if C enum isn't
not used directly. You think that that's what Rust should do?

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

[Trevor Gross]

On Mon, Oct 9, 2023 at 11:24 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
> that we have been discussing the same topics like locking, naming, etc
> again and again.

To be clear: this is ONLY for the rust design, I am not at all
qualified to review the build system integration. I provided a review
with the known caveats that:

1. The current enum handling is fragile, but only to the extent that
we do not handle values not specified in the C-side enum. I am not
sure what we can do better here until bindgen provides better
solutions.
2. Types for #define are not ideal
https://lore.kernel.org/rust-for-linux/CALNs47tnXWM3aVpeNMkuVZAJKc=seWxLAoLgSwqP0Jms+Mfc_A@mail.gmail.com/

These seem to me to be reasonable concessions at this time, but of
course the other reviewers will request further changes or perhaps
have suggestions for these items.

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 05:07:18PM -0400, Trevor Gross wrote:
> On Mon, Oct 9, 2023 at 11:24 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> > Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
> > that we have been discussing the same topics like locking, naming, etc
> > again and again.
> 
> To be clear: this is ONLY for the rust design, I am not at all
> qualified to review the build system integration. I provided a review
> with the known caveats that:

There is this patch going through review at the moment.

https://lore.kernel.org/netdev/fad9a472-ae78-8672-6c93-58ddde1447d9@intel.com/T/

Which says:

+It's safe to assume that the maintainers know the community and the level
+of expertise of the reviewers. The reviewers should not be concerned about
+their comments impeding or derailing the patch flow.

Even though Rust is new to netdev, there has been enough discussion
that we should be able to figure out what reviewers domain of
expertise is. That is part of the job of being a Maintainer for
netdev.

    Andrew

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 17:07:18 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Mon, Oct 9, 2023 at 11:24 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> Trevor gave Reviewed-by. Not perfect but reasonable shape, IMHO. Seems
>> that we have been discussing the same topics like locking, naming, etc
>> again and again.
> 
> To be clear: this is ONLY for the rust design, I am not at all
> qualified to review the build system integration. I provided a review
> with the known caveats that:

I think that it's safe to assume that subsystem maintainers understand that.

I really apprecate your feedback on the patchset.

> 1. The current enum handling is fragile, but only to the extent that
> we do not handle values not specified in the C-side enum. I am not
> sure what we can do better here until bindgen provides better
> solutions.
> 2. Types for #define are not ideal
> https://lore.kernel.org/rust-for-linux/CALNs47tnXWM3aVpeNMkuVZAJKc=seWxLAoLgSwqP0Jms+Mfc_A@mail.gmail.com/
> 
> These seem to me to be reasonable concessions at this time, but of
> course the other reviewers will request further changes or perhaps
> have suggestions for these items.

For me, they are an long-term issue.

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

[Andrew Lunn]

> > +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> 
> Broken formatting? Does `rustdoc` complain about it?

For C code, when you do a kernel build with W=1, it enables the
kerneldoc checker on each file:

e.g:

./scripts/kernel-doc -none   arch/x86/entry/vdso/vdso-image-32.c

Can rustdoc be invoked in a similar way? Perform a check on a file,
issue errors, but don't actually generate any documentation? If it
can, it would be good to extend W=1 with this.

The netdev CI instance builds with W=1, so we get to see problems like
this, and we ask for it to be fixed up before the code is merged.

      Andrew

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 3:54 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Can rustdoc be invoked in a similar way? Perform a check on a file,
> issue errors, but don't actually generate any documentation? If it
> can, it would be good to extend W=1 with this.

The Rust docs (like the Rust code) are supposed to be warning-free
(and should remain like that, at the very least for `defconfig` and so
on -- modulo mistakes, of course).

We were thinking of using `W=1` to enable more Clippy lints that have
some false positives or similar, but otherwise a lot of things are
already checked by default (i.e. while building the code and/or the
docs themselves).

Or did I misunderstand you?

Cheers,
Miguel

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 04:48:34PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 3:54 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Can rustdoc be invoked in a similar way? Perform a check on a file,
> > issue errors, but don't actually generate any documentation? If it
> > can, it would be good to extend W=1 with this.
> 
> The Rust docs (like the Rust code) are supposed to be warning-free
> (and should remain like that, at the very least for `defconfig` and so
> on -- modulo mistakes, of course).

'supposed to' is often not enough.

The netdev CI results can be seen here:

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

It would of been nice if netdev/kdoc test had failed if rustdoc found
problems.

We could add a new test, if rustdoc can be run on individual files.

   Andrew

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

[FUJITA Tomonori]

On Mon, 9 Oct 2023 14:59:19 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Hi Tomonori,
> 
> A few nits I noticed. Please note that this is not really a full
> review, and that I recommend that other people like Wedson should take
> a look again and OK these abstractions before this is merged.
> 
> On Mon, Oct 9, 2023 at 3:41 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +config RUST_PHYLIB_BINDINGS
> 
> This should be called ABSTRACTIONS. Please see:

Fixed.

>     https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings
> 
> Also, could this symbol go elsewhere?

This symbol is used by the third patch. Where do you want this? 


>> +        bool "PHYLIB bindings support"
> 
> Ditto.

Updated.


>> +          a wrapper around the C phlib core.
> 
> Typo.

Oops, sorry.


>> +#![feature(const_maybe_uninit_zeroed)]
> 
> The patch message should justify this addition and warn about it.

I added the following to the commit log.

This patch enables unstable const_maybe_uninit_zeroed feature for
kernel crate to enable unsafe code to handle a constant value with
uninitialized data. With the feature, the abstractions can initialize
a phy_driver structure with zero easily; instead of initializing all
the members by hand.


>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> new file mode 100644
>> index 000000000000..f31983bf0460
>> --- /dev/null
>> +++ b/rust/kernel/net/phy.rs
>> @@ -0,0 +1,733 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Newline missing.

Added.


>> +    /// Full-duplex mode
> 
> Please use the style of the rest of the Rust comments.

I'm not sure what the style should be but something like the
following?

/// Represents duplex mode.
pub enum DuplexMode {
    /// PHY is in full-duplex mode.
    Full,

>> +/// An instance of a PHY device.
>> +/// Wraps the kernel's `struct phy_device`.
> 
> That should be separated.

Added.


>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> 
> Missing Markdown around the lifetime.

Fixed.


>> +        // FIXME: enum-cast
> 
> Please explain what needs to be fixed.

Added.


>> +    /// Executes software reset the PHY via BMCR_RESET bit.
> 
> Markdown missing (multiple instances).

Can you elaborate?


>> +    /// Reads Link partner ability.
> 
> Why is "link" capitalized here?

Fixed.


>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> 
> Broken formatting? Does `rustdoc` complain about it?

Yes, sorry about that.


>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
> 
> Perhaps "The `drivers` field"?

I replaced this with the following comment suggested by Benno.

/// All elements of the `drivers` slice are valid and currently registered
/// to the kernel via `phy_drivers_register`.


>> +            // SAFETY: The type invariants guarantee that self.drivers is valid.
> 
> Markdown.

Fixed.


>> +/// 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,
>> +}
> 
> It would be nice to explain why the field is `pub`.

Added.


>> +    /// Get a mask as u32.
> 
> Markdown.

Fixed.


> This patch could be split a bit too, but that is up to the maintainers.

Yeah.


>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver` and registers it.
> 
> "kernel's" or similar

Added.


>> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> 
> Markdown.

Fixed.

>> +/// for module loading into the module binary file. Every driver needs an entry in device_table.
> 
> Markdown.

Fixed.


>> +/// # 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>()
>> +///     ],
>> +///     name: "rust_asix_phy",
>> +///     author: "Rust for Linux Contributors",
>> +///     description: "Rust Asix PHYs driver",
>> +///     license: "GPL",
>> +/// }
>> +/// ```
> 
> Please add an example above with the expansion of the macro so that it
> is easy to understand at a glance, see e.g. what Benno did in
> `pin-init` (`rust/init*`).

Added.

Thanks a lot!

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

[Trevor Gross]

On Wed, Oct 11, 2023 at 11:59 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> >> +#![feature(const_maybe_uninit_zeroed)]
> >
> > The patch message should justify this addition and warn about it.
>
> I added the following to the commit log.
>
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand.

Maybe also link something about its stability confidence?
https://github.com/rust-lang/rust/pull/116218#issuecomment-1738534665

> >> +    /// Executes software reset the PHY via BMCR_RESET bit.
> >
> > Markdown missing (multiple instances).
>
> Can you elaborate?

BMCR_RESET -> `BMCR_RESET` I believe

> > +/// 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,
> > +}
>
> It would be nice to explain why the field is `pub`.

On this subject, I think it would be good to add

    impl DeviceId {
        #[doc(hidden)] // <- macro use only
        pub const fn as_mdio_device_id(&self) ->
bindings::mdio_device_id { /* ... */ }
    }

That makes more sense when creating the table, and `id` no longer has
to be public.

> > This patch could be split a bit too, but that is up to the maintainers.
>
> Yeah.

Maybe it would make sense to put the macro in its own commit when you
send the next version? That gets some attention on its own.

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

[FUJITA Tomonori]

On Thu, 12 Oct 2023 00:43:00 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Wed, Oct 11, 2023 at 11:59 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> >> +#![feature(const_maybe_uninit_zeroed)]
>> >
>> > The patch message should justify this addition and warn about it.
>>
>> I added the following to the commit log.
>>
>> This patch enables unstable const_maybe_uninit_zeroed feature for
>> kernel crate to enable unsafe code to handle a constant value with
>> uninitialized data. With the feature, the abstractions can initialize
>> a phy_driver structure with zero easily; instead of initializing all
>> the members by hand.
> 
> Maybe also link something about its stability confidence?
> https://github.com/rust-lang/rust/pull/116218#issuecomment-1738534665

Thanks for the pointer. I'll update the commit log.


>> >> +    /// Executes software reset the PHY via BMCR_RESET bit.
>> >
>> > Markdown missing (multiple instances).
>>
>> Can you elaborate?
> 
> BMCR_RESET -> `BMCR_RESET` I believe

Thanks, fixed.


>> > +/// 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,
>> > +}
>>
>> It would be nice to explain why the field is `pub`.
> 
> On this subject, I think it would be good to add
> 
>     impl DeviceId {
>         #[doc(hidden)] // <- macro use only
>         pub const fn as_mdio_device_id(&self) ->
> bindings::mdio_device_id { /* ... */ }
>     }
> 
> That makes more sense when creating the table, and `id` no longer has
> to be public.

Ah, nice.


>> > This patch could be split a bit too, but that is up to the maintainers.
>>
>> Yeah.
> 
> Maybe it would make sense to put the macro in its own commit when you
> send the next version? That gets some attention on its own.

I don't want attention on the macro :) But yeah, I'll do in the next
round.

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

[Boqun Feng]

On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
[...]
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn phy_id(&mut self) -> u32 {

This function doesn't modify the `self`, why does this need to be a
`&mut self` function? Ditto for a few functions in this impl block.

It seems you used `&mut self` for all the functions, which looks like
more design work is required here.

Regards,
Boqun

> +        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 };
> +        // FIXME: enum-cast
> +        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,
> +        }
> +    }
> +
[...]

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 11:29:45 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
> [...]
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> +    /// may read or write to the `phy_device` object.
>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        unsafe { &mut *ptr.cast() }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&mut self) -> u32 {
> 
> This function doesn't modify the `self`, why does this need to be a
> `&mut self` function? Ditto for a few functions in this impl block.
> 
> It seems you used `&mut self` for all the functions, which looks like
> more design work is required here.

Ah, I can drop all the mut here.

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

[Boqun Feng]

On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
> On Wed, 11 Oct 2023 11:29:45 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> +impl Device {
> >> +    /// Creates a new [`Device`] instance from a raw pointer.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >> +    /// may read or write to the `phy_device` object.
> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >> +        unsafe { &mut *ptr.cast() }
> >> +    }
> >> +
> >> +    /// Gets the id of the PHY.
> >> +    pub fn phy_id(&mut self) -> u32 {
> > 
> > This function doesn't modify the `self`, why does this need to be a
> > `&mut self` function? Ditto for a few functions in this impl block.
> > 
> > It seems you used `&mut self` for all the functions, which looks like
> > more design work is required here.
> 
> Ah, I can drop all the mut here.

It may not be that easy... IIUC, most of the functions in the `impl`
block can only be called correctly with phydev->lock held. In other
words, their usage requires exclusive accesses. We should somehow
express this in the type system, otherwise someone may lose track on
this requirement in the future (for example, calling any function
without the lock held).

A simple type trick comes to me is that

impl Device {
    // rename `from_raw` into `assume_locked`
    pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
	...
    }
}

/// LockedDevice is just a new type of Device
pub struct LockedDevice(Device);

impl LockedDevice {
    pub fn phy_id(&self) -> u32 {
        ...
    }
}

Others may have better idea.

Fundamentally, having a mutable method which doesn't modify the object
makes little sense, however we does need a way (other than the `&mut
self`) to express the need of exclusive here..

Regards,
Boqun

> 

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

[FUJITA Tomonori]

On Wed, 11 Oct 2023 23:34:18 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
>> On Wed, 11 Oct 2023 11:29:45 -0700
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
>> > [...]
>> >> +impl Device {
>> >> +    /// Creates a new [`Device`] instance from a raw pointer.
>> >> +    ///
>> >> +    /// # Safety
>> >> +    ///
>> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> >> +    /// may read or write to the `phy_device` object.
>> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> >> +        unsafe { &mut *ptr.cast() }
>> >> +    }
>> >> +
>> >> +    /// Gets the id of the PHY.
>> >> +    pub fn phy_id(&mut self) -> u32 {
>> > 
>> > This function doesn't modify the `self`, why does this need to be a
>> > `&mut self` function? Ditto for a few functions in this impl block.
>> > 
>> > It seems you used `&mut self` for all the functions, which looks like
>> > more design work is required here.
>> 
>> Ah, I can drop all the mut here.
> 
> It may not be that easy... IIUC, most of the functions in the `impl`
> block can only be called correctly with phydev->lock held. In other
> words, their usage requires exclusive accesses. We should somehow
> express this in the type system, otherwise someone may lose track on
> this requirement in the future (for example, calling any function
> without the lock held).
>
> A simple type trick comes to me is that
> 
> impl Device {
>     // rename `from_raw` into `assume_locked`
>     pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
> 	...
>     }
> }

Hmm, the concept of PHYLIB is that a driver never play with a
lock. From the perspective of PHYLIB, this abstraction is a PHY
driver. The abstraction should not touch the lock.

How can someone lose track on this requirement? The abstraction
creates a Device instance only inside the callbacks.

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

[FUJITA Tomonori]

On Thu, 12 Oct 2023 15:44:44 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Wed, 11 Oct 2023 23:34:18 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
>>> On Wed, 11 Oct 2023 11:29:45 -0700
>>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>> 
>>> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
>>> > [...]
>>> >> +impl Device {
>>> >> +    /// Creates a new [`Device`] instance from a raw pointer.
>>> >> +    ///
>>> >> +    /// # Safety
>>> >> +    ///
>>> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>>> >> +    /// may read or write to the `phy_device` object.
>>> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> >> +        unsafe { &mut *ptr.cast() }
>>> >> +    }
>>> >> +
>>> >> +    /// Gets the id of the PHY.
>>> >> +    pub fn phy_id(&mut self) -> u32 {
>>> > 
>>> > This function doesn't modify the `self`, why does this need to be a
>>> > `&mut self` function? Ditto for a few functions in this impl block.
>>> > 
>>> > It seems you used `&mut self` for all the functions, which looks like
>>> > more design work is required here.
>>> 
>>> Ah, I can drop all the mut here.
>> 
>> It may not be that easy... IIUC, most of the functions in the `impl`
>> block can only be called correctly with phydev->lock held. In other
>> words, their usage requires exclusive accesses. We should somehow
>> express this in the type system, otherwise someone may lose track on
>> this requirement in the future (for example, calling any function
>> without the lock held).
>>
>> A simple type trick comes to me is that
>> 
>> impl Device {
>>     // rename `from_raw` into `assume_locked`
>>     pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
>> 	...
>>     }
>> }
> 
> Hmm, the concept of PHYLIB is that a driver never play with a
> lock. From the perspective of PHYLIB, this abstraction is a PHY
> driver. The abstraction should not touch the lock.
> 
> How can someone lose track on this requirement? The abstraction
> creates a Device instance only inside the callbacks.

Note `pub` isn't necessary here. I removed it.

No chance to misuse a Device instance as explained above, but if we
need to express the exclusive here, better to keep `mut`?

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

[Boqun Feng]

On Thu, Oct 12, 2023 at 04:02:46PM +0900, FUJITA Tomonori wrote:
> On Thu, 12 Oct 2023 15:44:44 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> > On Wed, 11 Oct 2023 23:34:18 -0700
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> >> On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
> >>> On Wed, 11 Oct 2023 11:29:45 -0700
> >>> Boqun Feng <boqun.feng@gmail.com> wrote:
> >>> 
> >>> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
> >>> > [...]
> >>> >> +impl Device {
> >>> >> +    /// Creates a new [`Device`] instance from a raw pointer.
> >>> >> +    ///
> >>> >> +    /// # Safety
> >>> >> +    ///
> >>> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >>> >> +    /// may read or write to the `phy_device` object.
> >>> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >>> >> +        unsafe { &mut *ptr.cast() }
> >>> >> +    }
> >>> >> +
> >>> >> +    /// Gets the id of the PHY.
> >>> >> +    pub fn phy_id(&mut self) -> u32 {
> >>> > 
> >>> > This function doesn't modify the `self`, why does this need to be a
> >>> > `&mut self` function? Ditto for a few functions in this impl block.
> >>> > 
> >>> > It seems you used `&mut self` for all the functions, which looks like
> >>> > more design work is required here.
> >>> 
> >>> Ah, I can drop all the mut here.
> >> 
> >> It may not be that easy... IIUC, most of the functions in the `impl`
> >> block can only be called correctly with phydev->lock held. In other
> >> words, their usage requires exclusive accesses. We should somehow
> >> express this in the type system, otherwise someone may lose track on
> >> this requirement in the future (for example, calling any function
> >> without the lock held).
> >>
> >> A simple type trick comes to me is that
> >> 
> >> impl Device {
> >>     // rename `from_raw` into `assume_locked`
> >>     pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
> >> 	...
> >>     }
> >> }
> > 
> > Hmm, the concept of PHYLIB is that a driver never play with a
> > lock. From the perspective of PHYLIB, this abstraction is a PHY
> > driver. The abstraction should not touch the lock.
> > 
> > How can someone lose track on this requirement? The abstraction
> > creates a Device instance only inside the callbacks.
> 
> Note `pub` isn't necessary here. I removed it.
> 
> No chance to misuse a Device instance as explained above, but if we
> need to express the exclusive here, better to keep `mut`?
> 

If `Device::from_raw`'s safety requirement is "only called in callbacks
with phydevice->lock held, etc.", then the exclusive access is
guaranteed by the safety requirement, therefore `mut` can be drop. It's
a matter of the exact semantics of the APIs.

Regards,
Boqun

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

[Trevor Gross]

On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@gmail.com> wrote:

> If `Device::from_raw`'s safety requirement is "only called in callbacks
> with phydevice->lock held, etc.", then the exclusive access is
> guaranteed by the safety requirement, therefore `mut` can be drop. It's
> a matter of the exact semantics of the APIs.
>
> Regards,
> Boqun

That is correct to my understanding, the core handles
locking/unlocking and no driver functions are called if the core
doesn't hold an exclusive lock first. Which also means the wrapper
type can't be `Sync`.

Andrew said a bit about it in the second comment here:
https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/

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

[FUJITA Tomonori]

On Thu, 12 Oct 2023 03:32:44 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> If `Device::from_raw`'s safety requirement is "only called in callbacks
>> with phydevice->lock held, etc.", then the exclusive access is
>> guaranteed by the safety requirement, therefore `mut` can be drop. It's
>> a matter of the exact semantics of the APIs.
>>
>> Regards,
>> Boqun
> 
> That is correct to my understanding, the core handles
> locking/unlocking and no driver functions are called if the core
> doesn't hold an exclusive lock first. Which also means the wrapper
> type can't be `Sync`.
> 
> Andrew said a bit about it in the second comment here:
> https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/

resume/suspend are called without the mutex hold but we don't need the
details. PHYLIB guarantees the exclusive access inside the
callbacks. I updated the comment and drop mut in Device's methods.

   /// Creates a new [`Device`] instance from a raw pointer.
   ///
   /// # Safety
   ///
   /// This function can be called only in the callbacks in `phy_driver`. PHYLIB guarantees
   /// the exclusive access for the duration of the lifetime `'a`.
   unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {

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

[Benno Lossin]

On 12.10.23 09:58, FUJITA Tomonori wrote:
> On Thu, 12 Oct 2023 03:32:44 -0400
> Trevor Gross <tmgross@umich.edu> wrote:
> 
>> On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>>> If `Device::from_raw`'s safety requirement is "only called in callbacks
>>> with phydevice->lock held, etc.", then the exclusive access is
>>> guaranteed by the safety requirement, therefore `mut` can be drop. It's
>>> a matter of the exact semantics of the APIs.
>>>
>>> Regards,
>>> Boqun
>>
>> That is correct to my understanding, the core handles
>> locking/unlocking and no driver functions are called if the core
>> doesn't hold an exclusive lock first. Which also means the wrapper
>> type can't be `Sync`.
>>
>> Andrew said a bit about it in the second comment here:
>> https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/
> 
> resume/suspend are called without the mutex hold but we don't need the
> details. PHYLIB guarantees the exclusive access inside the
> callbacks. I updated the comment and drop mut in Device's methods.

The details about this stuff are _extremely_ important, if there is a
mistake with the way `unsafe` requirements are written/interpreted, then
the Rust guarantee of "memory safety in safe code" flies out the window.
The whole idea is to offload all the dangerous stuff into smaller regions
that can be scrutinized more easily and for that we need all of the details.

What would be really helpful for me, as I have extremely limited
knowledge of the C side, would be an explaining comment in the phy
abstractions that explains the way the C side phy abstractions work. So
for example it would say that locking is handled by the phy core and (at
the moment) neither the Rust abstractions nor the driver code needs to
concern itself with locking. There you could also write that `resume` and
`suspend` are called without the mutex being held. We don't really have a
precedent for this (as there have been no drivers merged), but it would be
really helpful for me. If this exists in some other documentation, feel
free to just link that.

-- 
Cheers,
Benno

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

[Boqun Feng]

On Thu, Oct 12, 2023 at 09:10:41AM +0000, Benno Lossin wrote:
> On 12.10.23 09:58, FUJITA Tomonori wrote:
> > On Thu, 12 Oct 2023 03:32:44 -0400
> > Trevor Gross <tmgross@umich.edu> wrote:
> > 
> >> On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>
> >>> If `Device::from_raw`'s safety requirement is "only called in callbacks
> >>> with phydevice->lock held, etc.", then the exclusive access is
> >>> guaranteed by the safety requirement, therefore `mut` can be drop. It's
> >>> a matter of the exact semantics of the APIs.
> >>>
> >>> Regards,
> >>> Boqun
> >>
> >> That is correct to my understanding, the core handles
> >> locking/unlocking and no driver functions are called if the core
> >> doesn't hold an exclusive lock first. Which also means the wrapper
> >> type can't be `Sync`.
> >>
> >> Andrew said a bit about it in the second comment here:
> >> https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/
> > 
> > resume/suspend are called without the mutex hold but we don't need the
> > details. PHYLIB guarantees the exclusive access inside the
> > callbacks. I updated the comment and drop mut in Device's methods.
> 
> The details about this stuff are _extremely_ important, if there is a
> mistake with the way `unsafe` requirements are written/interpreted, then
> the Rust guarantee of "memory safety in safe code" flies out the window.
> The whole idea is to offload all the dangerous stuff into smaller regions
> that can be scrutinized more easily and for that we need all of the details.
> 

Thank Benno for calling this out.

After re-read my email exchange with Tomo, I realised I need to explain
this a little bit. The minimal requirement of a Rust binding is
soundness: it means if one only uses safe APIs, one cannot introduce
memory/type safety issue (i.e. cannot have an object in an invalid
state), this is a tall task, because you can have zero assumption of the
API users, you can only encode the usage requirement in the type system.

Of course the type system doesn't always work, hence we have unsafe API,
but still the soundness of Rust bindings means using safe APIs +
*correctly* using unsafe APIs cannot introduce memory/type safety
issues.

Tomo, this is why we gave you a hard time here ;-) Unsafe Rust APIs must
be very clear on the correct usage and safe Rust APIs must not assume
how users would call it. Hope this help explain a little bit, we are not
poking random things here, soundness is the team effort from everyone
;-)

Regards,
Boqun

> What would be really helpful for me, as I have extremely limited
> knowledge of the C side, would be an explaining comment in the phy
> abstractions that explains the way the C side phy abstractions work. So
> for example it would say that locking is handled by the phy core and (at
> the moment) neither the Rust abstractions nor the driver code needs to
> concern itself with locking. There you could also write that `resume` and
> `suspend` are called without the mutex being held. We don't really have a
> precedent for this (as there have been no drivers merged), but it would be
> really helpful for me. If this exists in some other documentation, feel
> free to just link that.
> 
> -- 
> Cheers,
> Benno
> 
> 

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

[FUJITA Tomonori]

On Thu, 12 Oct 2023 21:17:14 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> After re-read my email exchange with Tomo, I realised I need to explain
> this a little bit. The minimal requirement of a Rust binding is
> soundness: it means if one only uses safe APIs, one cannot introduce
> memory/type safety issue (i.e. cannot have an object in an invalid
> state), this is a tall task, because you can have zero assumption of the
> API users, you can only encode the usage requirement in the type system.
> 
> Of course the type system doesn't always work, hence we have unsafe API,
> but still the soundness of Rust bindings means using safe APIs +
> *correctly* using unsafe APIs cannot introduce memory/type safety
> issues.
> 
> Tomo, this is why we gave you a hard time here ;-) Unsafe Rust APIs must
> be very clear on the correct usage and safe Rust APIs must not assume
> how users would call it. Hope this help explain a little bit, we are not
> poking random things here, soundness is the team effort from everyone
> ;-)

Understood, so let me know if you still want to improve something in
v4 patchset :) I tried to addressed all the review comments.

btw, what's the purpose of using Rust in linux kernel? Creating sound
Rust abstractions? Making linux kernel more reliable, or something
else?  For me, making linux kernel more reliable is the whole
point. Thus I still can't understand the slogan that Rust abstractions
can't trust subsystems.

Rust abstractions always must check the validity of values that
subsysmtes give because subsysmtes might give an invalid value. Like
the enum state issue, if PHYLIB has a bug then give a random value, so
the abstraction have to prevent the invalid value in Rust with
validity checking. But with such critical bug, likely the system
cannot continue to run anyway. Preventing the invalid state in Rust
aren't useful much for system reliability.

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

[Benno Lossin]

On 13.10.23 07:45, FUJITA Tomonori wrote:
> On Thu, 12 Oct 2023 21:17:14 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> After re-read my email exchange with Tomo, I realised I need to explain
>> this a little bit. The minimal requirement of a Rust binding is
>> soundness: it means if one only uses safe APIs, one cannot introduce
>> memory/type safety issue (i.e. cannot have an object in an invalid
>> state), this is a tall task, because you can have zero assumption of the
>> API users, you can only encode the usage requirement in the type system.
>>
>> Of course the type system doesn't always work, hence we have unsafe API,
>> but still the soundness of Rust bindings means using safe APIs +
>> *correctly* using unsafe APIs cannot introduce memory/type safety
>> issues.
>>
>> Tomo, this is why we gave you a hard time here ;-) Unsafe Rust APIs must
>> be very clear on the correct usage and safe Rust APIs must not assume
>> how users would call it. Hope this help explain a little bit, we are not
>> poking random things here, soundness is the team effort from everyone
>> ;-)
> 
> Understood, so let me know if you still want to improve something in
> v4 patchset :) I tried to addressed all the review comments.
> 
> btw, what's the purpose of using Rust in linux kernel? Creating sound
> Rust abstractions? Making linux kernel more reliable, or something
> else?  For me, making linux kernel more reliable is the whole
> point. Thus I still can't understand the slogan that Rust abstractions
> can't trust subsystems.

For me it is making the Linux kernel more reliable. The Rust abstractions
are just a tool for that goal: we offload the difficult task of handling
the C <-> Rust interactions and other `unsafe` features into those
abstractions. Then driver authors do not need to concern themselves with
that and can freely write drivers in safe Rust. Since there will be a lot
more drivers than abstractions, this will pay off in the end, since we will
have a lot less `unsafe` code than safe code.

Concentrating the difficult/`unsafe` code in the abstractions make it
easier to review (compared to `unsafe` code in every driver) and easier to
maintain, if we find a soundness issue, we only have to fix it in the
abstractions.

> Rust abstractions always must check the validity of values that
> subsysmtes give because subsysmtes might give an invalid value. Like
> the enum state issue, if PHYLIB has a bug then give a random value, so
> the abstraction have to prevent the invalid value in Rust with
> validity checking. But with such critical bug, likely the system
> cannot continue to run anyway. Preventing the invalid state in Rust
> aren't useful much for system reliability.

It's not that we do not trust the subsystems, for example when we register
a callback `foo` and the C side documents that it is ok to sleep within
`foo`, then we will assume so. If we would not trust the C side, then we
would have to disallow sleeping there, since sleeping while holding a
spinlock is UB (and the C side could accidentally be holding a spinlock).

But there are certain things where we do not trust the subsystems, these
are mainly things where we can afford it from a performance and usability
perspective (in the example above we could not afford it from a usability
perspective).

In the enum case it would also be incredibly simple for the C side to just
make a slight mistake and set the integer to a value outside of the
specified range. This strengthens the case for checking validity here.
When an invalid value is given to Rust we have immediate UB. In Rust UB
always means that anything can happen so we must avoid it at all costs.
In this case having a check would not really hurt performance and in terms
of usability it also seems reasonable. If it would be bad for performance,
let us know.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Fri, 13 Oct 2023 07:56:07 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> btw, what's the purpose of using Rust in linux kernel? Creating sound
>> Rust abstractions? Making linux kernel more reliable, or something
>> else?  For me, making linux kernel more reliable is the whole
>> point. Thus I still can't understand the slogan that Rust abstractions
>> can't trust subsystems.
> 
> For me it is making the Linux kernel more reliable. The Rust abstractions
> are just a tool for that goal: we offload the difficult task of handling
> the C <-> Rust interactions and other `unsafe` features into those
> abstractions. Then driver authors do not need to concern themselves with
> that and can freely write drivers in safe Rust. Since there will be a lot
> more drivers than abstractions, this will pay off in the end, since we will
> have a lot less `unsafe` code than safe code.
> 
> Concentrating the difficult/`unsafe` code in the abstractions make it
> easier to review (compared to `unsafe` code in every driver) and easier to
> maintain, if we find a soundness issue, we only have to fix it in the
> abstractions.

Agreed.


>> Rust abstractions always must check the validity of values that
>> subsysmtes give because subsysmtes might give an invalid value. Like
>> the enum state issue, if PHYLIB has a bug then give a random value, so
>> the abstraction have to prevent the invalid value in Rust with
>> validity checking. But with such critical bug, likely the system
>> cannot continue to run anyway. Preventing the invalid state in Rust
>> aren't useful much for system reliability.
> 
> It's not that we do not trust the subsystems, for example when we register
> a callback `foo` and the C side documents that it is ok to sleep within
> `foo`, then we will assume so. If we would not trust the C side, then we
> would have to disallow sleeping there, since sleeping while holding a
> spinlock is UB (and the C side could accidentally be holding a spinlock).
> 
> But there are certain things where we do not trust the subsystems, these
> are mainly things where we can afford it from a performance and usability
> perspective (in the example above we could not afford it from a usability
> perspective).

You need maintenance cost too here. That's exactly the discussion
point during reviewing the enum code, the kinda cut-and-paste from C
code and match code that Andrew and Grek want to avoid.


> In the enum case it would also be incredibly simple for the C side to just
> make a slight mistake and set the integer to a value outside of the
> specified range. This strengthens the case for checking validity here.
> When an invalid value is given to Rust we have immediate UB. In Rust UB
> always means that anything can happen so we must avoid it at all costs.

I'm not sure the general rules in Rust can be applied to linux kernel.

If the C side (PHYLIB) to set in an invalid value to the state,
probably the network doesn't work; already anything can happen in the
system at this point. Then the Rust abstractions get the invalid value
from the C side and detect an error with a check. The abstractions
return an error to a Rust PHY driver. Next what can the Rust PHY
driver do? Stop working? Calling dev_err() to print something and then
selects the state randomly and continue?

What's the practical benefit from the check?


> In this case having a check would not really hurt performance and in terms
> of usability it also seems reasonable. If it would be bad for performance,
> let us know.

Bad for maintenance cost. Please read the discussion in the review on rfc v1.

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

[Benno Lossin]

On 13.10.23 11:53, FUJITA Tomonori wrote:
> On Fri, 13 Oct 2023 07:56:07 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> It's not that we do not trust the subsystems, for example when we register
>> a callback `foo` and the C side documents that it is ok to sleep within
>> `foo`, then we will assume so. If we would not trust the C side, then we
>> would have to disallow sleeping there, since sleeping while holding a
>> spinlock is UB (and the C side could accidentally be holding a spinlock).
>>
>> But there are certain things where we do not trust the subsystems, these
>> are mainly things where we can afford it from a performance and usability
>> perspective (in the example above we could not afford it from a usability
>> perspective).
> 
> You need maintenance cost too here. That's exactly the discussion
> point during reviewing the enum code, the kinda cut-and-paste from C
> code and match code that Andrew and Grek want to avoid.

Indeed, however Trevor already has opened an issue at bindgen [1]
that will fix this maintenance nightmare. It seems to me that the
bindgen developers are willing to implement this. It also seems that
this feature can be implemented rather quickly, so I would not worry
about the ergonomics and choose safety until we can use the new bindgen
feature.

[1]: https://github.com/rust-lang/rust-bindgen/issues/2646

>> In the enum case it would also be incredibly simple for the C side to just
>> make a slight mistake and set the integer to a value outside of the
>> specified range. This strengthens the case for checking validity here.
>> When an invalid value is given to Rust we have immediate UB. In Rust UB
>> always means that anything can happen so we must avoid it at all costs.
> 
> I'm not sure the general rules in Rust can be applied to linux kernel.

Rust UB is still forbidden, it can introduce arbitrary misscompilations.

> If the C side (PHYLIB) to set in an invalid value to the state,
> probably the network doesn't work; already anything can happen in the
> system at this point. Then the Rust abstractions get the invalid value
> from the C side and detect an error with a check. The abstractions
> return an error to a Rust PHY driver. Next what can the Rust PHY
> driver do? Stop working? Calling dev_err() to print something and then
> selects the state randomly and continue?

What if the C side has a bug and gives us a bad value by mistake? It is
not required for the network not working for us to receive an invalid
value. Ideally the PHY driver would not even notice this, the abstractions
should handle this fully. Not exactly sure what to do in the error case,
maybe a warn_once and then choose some sane default state?

> What's the practical benefit from the check?

The practical use of the check is that we do not introduce UB.

>> In this case having a check would not really hurt performance and in terms
>> of usability it also seems reasonable. If it would be bad for performance,
>> let us know.
> 
> Bad for maintenance cost. Please read the discussion in the review on rfc v1.

Since this will only be temporary, I believe it to be fine.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Fri, 13 Oct 2023 10:03:43 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 13.10.23 11:53, FUJITA Tomonori wrote:
>> On Fri, 13 Oct 2023 07:56:07 +0000
>> Benno Lossin <benno.lossin@proton.me> wrote:
>>> It's not that we do not trust the subsystems, for example when we register
>>> a callback `foo` and the C side documents that it is ok to sleep within
>>> `foo`, then we will assume so. If we would not trust the C side, then we
>>> would have to disallow sleeping there, since sleeping while holding a
>>> spinlock is UB (and the C side could accidentally be holding a spinlock).
>>>
>>> But there are certain things where we do not trust the subsystems, these
>>> are mainly things where we can afford it from a performance and usability
>>> perspective (in the example above we could not afford it from a usability
>>> perspective).
>> 
>> You need maintenance cost too here. That's exactly the discussion
>> point during reviewing the enum code, the kinda cut-and-paste from C
>> code and match code that Andrew and Grek want to avoid.
> 
> Indeed, however Trevor already has opened an issue at bindgen [1]
> that will fix this maintenance nightmare. It seems to me that the
> bindgen developers are willing to implement this. It also seems that
> this feature can be implemented rather quickly, so I would not worry
> about the ergonomics and choose safety until we can use the new bindgen
> feature.
> 
> [1]: https://github.com/rust-lang/rust-bindgen/issues/2646

Yeah, I know. I wrote multiple times, let's go with a temporary
solution and will use the better solution when it will be available.


>>> In the enum case it would also be incredibly simple for the C side to just
>>> make a slight mistake and set the integer to a value outside of the
>>> specified range. This strengthens the case for checking validity here.
>>> When an invalid value is given to Rust we have immediate UB. In Rust UB
>>> always means that anything can happen so we must avoid it at all costs.
>> 
>> I'm not sure the general rules in Rust can be applied to linux kernel.
> 
> Rust UB is still forbidden, it can introduce arbitrary misscompilations.

Can you give a pointer on how it can introduce such?


>> If the C side (PHYLIB) to set in an invalid value to the state,
>> probably the network doesn't work; already anything can happen in the
>> system at this point. Then the Rust abstractions get the invalid value
>> from the C side and detect an error with a check. The abstractions
>> return an error to a Rust PHY driver. Next what can the Rust PHY
>> driver do? Stop working? Calling dev_err() to print something and then
>> selects the state randomly and continue?
> 
> What if the C side has a bug and gives us a bad value by mistake? It is
> not required for the network not working for us to receive an invalid
> value. Ideally the PHY driver would not even notice this, the abstractions
> should handle this fully. Not exactly sure what to do in the error case,

Your case is that C side has a good value but somehow gives a bad
value to the abstractions?

The abstractions can't handle this. The abstractions works as the part
of a PHY driver; The abstractions do only what The driver asks.

The PHY driver asks the state from the abstractions then the
abstractions ask the state from PHYLIB. So when the abstractions get a
bad value from PHYLIB, the abstractions must return something to the
PHY driver. As I wrote, the abstractions return a random value or an
error. In either way, probably the system cannot continue.


> maybe a warn_once and then choose some sane default state?

What sane default? PHY_ERROR?


>> What's the practical benefit from the check?
> 
> The practical use of the check is that we do not introduce UB.

hmm.


>>> In this case having a check would not really hurt performance and in terms
>>> of usability it also seems reasonable. If it would be bad for performance,
>>> let us know.
>> 
>> Bad for maintenance cost. Please read the discussion in the review on rfc v1.
> 
> Since this will only be temporary, I believe it to be fine.

Great, if you have other concerns on v4 patchset, please let me
know. I tried to address all your comments.

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

[Benno Lossin]

On 13.10.23 12:53, FUJITA Tomonori wrote:
>>>> In the enum case it would also be incredibly simple for the C side to just
>>>> make a slight mistake and set the integer to a value outside of the
>>>> specified range. This strengthens the case for checking validity here.
>>>> When an invalid value is given to Rust we have immediate UB. In Rust UB
>>>> always means that anything can happen so we must avoid it at all costs.
>>>
>>> I'm not sure the general rules in Rust can be applied to linux kernel.
>>
>> Rust UB is still forbidden, it can introduce arbitrary misscompilations.
> 
> Can you give a pointer on how it can introduce such?

First, I can point you to [1] that is a list of UB that can occur in
Rust. Second, I can give you an example [2] of UB leading to
miscompilations, compare the executions of both release and debug mode.

[1]: https://doc.rust-lang.org/nomicon/what-unsafe-does.html#what-unsafe-rust-can-do
[2]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=856cdd7434350e38d3891162e04424db

>>> If the C side (PHYLIB) to set in an invalid value to the state,
>>> probably the network doesn't work; already anything can happen in the
>>> system at this point. Then the Rust abstractions get the invalid value
>>> from the C side and detect an error with a check. The abstractions
>>> return an error to a Rust PHY driver. Next what can the Rust PHY
>>> driver do? Stop working? Calling dev_err() to print something and then
>>> selects the state randomly and continue?
>>
>> What if the C side has a bug and gives us a bad value by mistake? It is
>> not required for the network not working for us to receive an invalid
>> value. Ideally the PHY driver would not even notice this, the abstractions
>> should handle this fully. Not exactly sure what to do in the error case,
> 
> Your case is that C side has a good value but somehow gives a bad
> value to the abstractions?

Just think of the C side having some weird bug.

> The abstractions can't handle this. The abstractions works as the part
> of a PHY driver; The abstractions do only what The driver asks.
> 
> The PHY driver asks the state from the abstractions then the
> abstractions ask the state from PHYLIB. So when the abstractions get a
> bad value from PHYLIB, the abstractions must return something to the
> PHY driver. As I wrote, the abstractions return a random value or an
> error. In either way, probably the system cannot continue.

Sure then let the system BUG if it cannot continue. I think that
allowing UB is worse than BUGing.

>> maybe a warn_once and then choose some sane default state?
> 
> What sane default? PHY_ERROR?

Sure.

-- 
Cheers,
Benno

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

[Andrew Lunn]

> > The PHY driver asks the state from the abstractions then the
> > abstractions ask the state from PHYLIB. So when the abstractions get a
> > bad value from PHYLIB, the abstractions must return something to the
> > PHY driver. As I wrote, the abstractions return a random value or an
> > error. In either way, probably the system cannot continue.
> 
> Sure then let the system BUG if it cannot continue. I think that
> allowing UB is worse than BUGing.

There is nothing a PHY driver can do which justifies calling BUG().

BUG() indicates the system is totally messed up, and running further
is going to result in more file system corruption, causing more data
loss, so we need to stop the machine immediately.

Anyway, we are talking about this bit of code in the C driver:

        /* Reset PHY, otherwise MII_LPA will provide outdated information.
         * This issue is reproducible only with some link partner PHYs
         */
        if (phydev->state == PHY_NOLINK) {
                phy_init_hw(phydev);
                _phy_start_aneg(phydev);
        }

and what we should do if phydev->state is not one of the values
defined in enum phy_state, but is actually 42. The system will
continue, but it could be that the hardware reports the wrong value
for LPA, the Link Partner Advertisement. That is not critical
information, the link is likely to work, but the debug tool ethtool(1)
will report the wrong value.

Can we turn this UB into DB? I guess you can make the abstraction
accept any value, validate it against the values in enum phy_state, do
a WARN_ON_ONCE() if its not valid so we get a stack trace, and pass
the value on. Life will likely continue, hopefully somebody will
report the stack trace, and we can try to figure out what went wrong.

  Andrew

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

[Benno Lossin]

On 14.10.23 23:55, Andrew Lunn wrote:
>>> The PHY driver asks the state from the abstractions then the
>>> abstractions ask the state from PHYLIB. So when the abstractions get a
>>> bad value from PHYLIB, the abstractions must return something to the
>>> PHY driver. As I wrote, the abstractions return a random value or an
>>> error. In either way, probably the system cannot continue.
>>
>> Sure then let the system BUG if it cannot continue. I think that
>> allowing UB is worse than BUGing.
> 
> There is nothing a PHY driver can do which justifies calling BUG().

I was mostly replying to Tomonori's comment "In either way, probably
the system cannot continue.". I think that defaulting the value to
`PHY_ERROR` when it is out of range to be a lot better way of handling
this.

> BUG() indicates the system is totally messed up, and running further
> is going to result in more file system corruption, causing more data
> loss, so we need to stop the machine immediately.
> 
> Anyway, we are talking about this bit of code in the C driver:
> 
>          /* Reset PHY, otherwise MII_LPA will provide outdated information.
>           * This issue is reproducible only with some link partner PHYs
>           */
>          if (phydev->state == PHY_NOLINK) {
>                  phy_init_hw(phydev);
>                  _phy_start_aneg(phydev);
>          }
> 

I think that we are talking about `Device::state` i.e. the
Rust wrapper function of `phydev->state`.

> and what we should do if phydev->state is not one of the values
> defined in enum phy_state, but is actually 42. The system will
> continue, but it could be that the hardware reports the wrong value
> for LPA, the Link Partner Advertisement. That is not critical
> information, the link is likely to work, but the debug tool ethtool(1)
> will report the wrong value.
> 
> Can we turn this UB into DB? I guess you can make the abstraction
> accept any value, validate it against the values in enum phy_state, do
> a WARN_ON_ONCE() if its not valid so we get a stack trace, and pass
> the value on. Life will likely continue, hopefully somebody will
> report the stack trace, and we can try to figure out what went wrong.

Then we are on the same page, as that would be my preferred solution.

-- 
Cheers,
Benno

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

[Andrew Lunn]

On Sat, Oct 14, 2023 at 10:18:58PM +0000, Benno Lossin wrote:
> On 14.10.23 23:55, Andrew Lunn wrote:
> >>> The PHY driver asks the state from the abstractions then the
> >>> abstractions ask the state from PHYLIB. So when the abstractions get a
> >>> bad value from PHYLIB, the abstractions must return something to the
> >>> PHY driver. As I wrote, the abstractions return a random value or an
> >>> error. In either way, probably the system cannot continue.
> >>
> >> Sure then let the system BUG if it cannot continue. I think that
> >> allowing UB is worse than BUGing.
> > 
> > There is nothing a PHY driver can do which justifies calling BUG().
> 
> I was mostly replying to Tomonori's comment "In either way, probably
> the system cannot continue.". I think that defaulting the value to
> `PHY_ERROR` when it is out of range to be a lot better way of handling
> this.

You could actually call phy_error(phydev); That will set the state to
PHY_ERROR and transition the state machine to put the link down.

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L1213

Its not documented for this use case, its more intended for the
hardware has a problem, and stopped talking to us. But if phylib
itself is messed up, it would seem like a reasonable way to try to
recover. It will dump the stack as well.

	 Andrew

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

[Boqun Feng]

On Fri, Oct 13, 2023 at 06:53:48PM +0900, FUJITA Tomonori wrote:
> On Fri, 13 Oct 2023 07:56:07 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
> >> btw, what's the purpose of using Rust in linux kernel? Creating sound
> >> Rust abstractions? Making linux kernel more reliable, or something
> >> else?  For me, making linux kernel more reliable is the whole
> >> point. Thus I still can't understand the slogan that Rust abstractions
> >> can't trust subsystems.
> > 
> > For me it is making the Linux kernel more reliable. The Rust abstractions
> > are just a tool for that goal: we offload the difficult task of handling
> > the C <-> Rust interactions and other `unsafe` features into those
> > abstractions. Then driver authors do not need to concern themselves with
> > that and can freely write drivers in safe Rust. Since there will be a lot
> > more drivers than abstractions, this will pay off in the end, since we will
> > have a lot less `unsafe` code than safe code.
> > 
> > Concentrating the difficult/`unsafe` code in the abstractions make it
> > easier to review (compared to `unsafe` code in every driver) and easier to
> > maintain, if we find a soundness issue, we only have to fix it in the
> > abstractions.
> 
> Agreed.
> 

Right, so the soundness of the Rust abstraction is the tool for more
reliable kernel. And honestly I haven't found anything that "sound Rust
abstracions" and "more reliable kernel" conflict with either other. If
we provide unsound Rust API, what's the point of using Rust? You can
always provide unsound API rather easily with C or put it in another
way, you can always rely on various implementation details to prove that
nothing is wrong (e.g. "this function is only called under situation A,
B or C, so it's fine"), but these details lost track easily as time
goes. With sound API, hopefully, we can put these details in the type
system and the unsafe requirements, so that these can be helpful (and
compiler can be helpful).

Of course, kernel is compliciated, and I'm pretty there are things that
sound Rust API cannot express (or at least easily). In that case, if
necessary and possible, we should improve the tool, rather than bend the
rules of API soundness, because, as Greg said, there is no deadline
here, we don't need hurry.

Regards,
Boqun

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

[Miguel Ojeda]

On Fri, Oct 13, 2023 at 11:53 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I'm not sure the general rules in Rust can be applied to linux kernel.

Benno and others already replied nicely to this, but I wanted to point
out that this happens with C compilers just the same. It is not a
"Rust thing" and what matters is what compilers do here, in practice.

For instance, you can try to compile this with GCC under -O2, and you
will get a program that returns a 2:

    int main(void) {
        _Bool b;
        char c = 42;
        memcpy(&b, &c, 1);
        if (b)
            return 43;
        return 44;
    }

Similarly, one for Rust where LLVM simply generates `ud2`:

    #[repr(u32)]
    pub enum E {
        A = 0,
        B = 1,
    }

    pub fn main() {
        let e = unsafe { core::mem::transmute::<u32, E>(5) };
        std::process::exit(match e {
            E::A => 42,
            E::B => 43,
        });
    }

The `e` variable is what we can get from C without an unsafe block if
you use `--rustified-enum`, i.e. the case in your abstractions.

The critical bit here is that, in C, it is not UB to have value 5 in
its enum, so we cannot rely on that.

Cheers,
Miguel

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

[Boqun Feng]

On Thu, Oct 12, 2023 at 03:44:44PM +0900, FUJITA Tomonori wrote:
> On Wed, 11 Oct 2023 23:34:18 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
> >> On Wed, 11 Oct 2023 11:29:45 -0700
> >> Boqun Feng <boqun.feng@gmail.com> wrote:
> >> 
> >> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
> >> > [...]
> >> >> +impl Device {
> >> >> +    /// Creates a new [`Device`] instance from a raw pointer.
> >> >> +    ///
> >> >> +    /// # Safety
> >> >> +    ///
> >> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >> >> +    /// may read or write to the `phy_device` object.
> >> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >> >> +        unsafe { &mut *ptr.cast() }
> >> >> +    }
> >> >> +
> >> >> +    /// Gets the id of the PHY.
> >> >> +    pub fn phy_id(&mut self) -> u32 {
> >> > 
> >> > This function doesn't modify the `self`, why does this need to be a
> >> > `&mut self` function? Ditto for a few functions in this impl block.
> >> > 
> >> > It seems you used `&mut self` for all the functions, which looks like
> >> > more design work is required here.
> >> 
> >> Ah, I can drop all the mut here.
> > 
> > It may not be that easy... IIUC, most of the functions in the `impl`
> > block can only be called correctly with phydev->lock held. In other
> > words, their usage requires exclusive accesses. We should somehow
> > express this in the type system, otherwise someone may lose track on
> > this requirement in the future (for example, calling any function
> > without the lock held).
> >
> > A simple type trick comes to me is that
> > 
> > impl Device {
> >     // rename `from_raw` into `assume_locked`
> >     pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
> > 	...
> >     }
> > }
> 
> Hmm, the concept of PHYLIB is that a driver never play with a
> lock. From the perspective of PHYLIB, this abstraction is a PHY
> driver. The abstraction should not touch the lock.
> 

Well, usually we want to describe such a constrait/requirement in the
type system, that's part of the Rust bindings, of course, for some
properties it may be hard, so it may be impossible.

> How can someone lose track on this requirement? The abstraction
> creates a Device instance only inside the callbacks.
> 

Right now, yes. The code in the patch only "creates" a Device inside
the callbacks, but the `Device::from_raw` function doesn't mention any
of this requirement, if the design is only called inside the callbacks,
please add something in the function's `# Safety` requirement, since
voliating this may cause memory safety issue.

Type system and unsafe comments are contracts, if one API has a limited
usage by design, people should be able to find it somewhere in the
contracts.

Regards,
Boqun

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

[FUJITA Tomonori]

Adds me as a maintainer for these Rust bindings too.

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 1bd96045beb8..b6d7e4de6438 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7771,6 +7771,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
@@ -7800,6 +7801,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 net-next v3 3/3] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

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

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/Kconfig          |   7 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 4 files changed, 143 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 421d2b62918f..0317be180ac2 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 version driver for Asix PHYs"
+	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
+	help
+	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
+	  instead of the C version.
+
 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..017f817f6f8d
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Rust Asix PHYs driver
+//!
+//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
+use kernel::c_str;
+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>()
+    ],
+    name: "rust_asix_phy",
+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    description: "Rust Asix PHYs driver",
+    license: "GPL",
+}
+
+// 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(uapi::SPEED_100);
+        } else {
+            dev.set_speed(uapi::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..08f5e9334c9e 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,3 +7,5 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mii.h>
+#include <uapi/linux/ethtool.h>
-- 
2.34.1

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Trevor Gross]

On Sun, Oct 8, 2023 at 9:41 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> 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>
> ---

Thanks for adding the speed bindings. The last thing is that I still
think we need to figure out a better way to generate the correct types
for #define values (discussed around [1]) but we can fix that later.

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

[1]: https://lore.kernel.org/rust-for-linux/CALNs47sxuGVXBwhXZa5NgHQ8F0MH2OoUzsgthAURE+OuTtJ1wQ@mail.gmail.com/

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Jiri Pirko]

Mon, Oct 09, 2023 at 03:39:12AM CEST, fujita.tomonori@gmail.com wrote:
>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          |   2 +
> 4 files changed, 143 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 421d2b62918f..0317be180ac2 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 version driver for Asix PHYs"
>+	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
>+	help
>+	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
>+	  instead of the C version.
>+
> 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..017f817f6f8d
>--- /dev/null
>+++ b/drivers/net/phy/ax88796b_rust.rs
>@@ -0,0 +1,129 @@
>+// SPDX-License-Identifier: GPL-2.0
>+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
>+
>+//! Rust Asix PHYs driver
>+//!
>+//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)

Wait. So you just add rust driver as a duplicate of existing c driver?
What's the point exactly to have 2 drivers for the same thing?



>+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>()
>+    ],
>+    name: "rust_asix_phy",
>+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
>+    description: "Rust Asix PHYs driver",
>+    license: "GPL",
>+}
>+
>+// 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(uapi::SPEED_100);
>+        } else {
>+            dev.set_speed(uapi::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..08f5e9334c9e 100644
>--- a/rust/uapi/uapi_helper.h
>+++ b/rust/uapi/uapi_helper.h
>@@ -7,3 +7,5 @@
>  */
> 
> #include <uapi/asm-generic/ioctl.h>
>+#include <uapi/linux/mii.h>
>+#include <uapi/linux/ethtool.h>

What is exactly the reason to change anything in uapi for phy driver?
Should be just kernel api implementation, no?



>-- 
>2.34.1
>
>

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 9:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wait. So you just add rust driver as a duplicate of existing c driver?
> What's the point exactly to have 2 drivers for the same thing?

Please see https://lore.kernel.org/ksummit/CANiq72=99VFE=Ve5MNM9ZuSe9M-JSH1evk6pABNSEnNjK7aXYA@mail.gmail.com/.

Cheers,
Miguel

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

On Mon, 9 Oct 2023 09:23:04 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

>>diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
>>index 301f5207f023..08f5e9334c9e 100644
>>--- a/rust/uapi/uapi_helper.h
>>+++ b/rust/uapi/uapi_helper.h
>>@@ -7,3 +7,5 @@
>>  */
>> 
>> #include <uapi/asm-generic/ioctl.h>
>>+#include <uapi/linux/mii.h>
>>+#include <uapi/linux/ethtool.h>
> 
> What is exactly the reason to change anything in uapi for phy driver?
> Should be just kernel api implementation, no?

It doesn't change anything. Like the C PHY drivers do, Rust PHY
drivers use the defined values in header files in uapi like MII_*,
BMCR_*, etc.

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

> Wait. So you just add rust driver as a duplicate of existing c driver?
> What's the point exactly to have 2 drivers for the same thing?

To tell the truth, i don't think the driver itself is very
important. The important thing has been the discussion along the way
to get to a driver.

For me as a Maintainer, the discussion about maintainability, how do
we make the build fail when C and Rust diverge is important.  So it
seems C enum are better than #defines for that. Maybe we will need to
replace some #define lists with enum in core code? But we also have a
lot of #defines which it would be good to be able to use.

It took a while to get the code to actually register the driver
instances. But this is something nearly every driver needs to do, so i
hope the ideas and maybe some of the actual code can be used in other
drivers.

It has become much clearer the Rust build system needs work. It is too
monolithic at the moment, and there are no good examples of kconfig
integration. 

Documentation is still an open question for me. I know Rust can
generate documentation from the code, but how do we integrate that
into the existing kernel documentation?

Within netdev, our own tooling is not ready for Rust. Our patchwork
instance did not recognise the patch was for netdev. That has been
fixed now. But i'm pretty sure the latest version of the code was not
built as part of our build testing. Jakub said the machine did not
have a Rust toolchain.  I also think because of the Rust build system
limitations, even if it did have the tools, i don't think with
allmodconfig it would try to built the rust code.

When i build it on my machine, i get a million warnings from i think
the linker. That is not going to be acceptable to Mainline, so we need
to investigate that.

I hope some sort of lessons learned, best practices and TODO list can
be distilled from the experience, to help guide the Rust Experiment.

	Andrew

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 2:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> It has become much clearer the Rust build system needs work. It is too
> monolithic at the moment, and there are no good examples of kconfig
> integration.

I am not sure what you mean. As I said elsewhere, this is well-known,
was done on purpose, and we have discussed it publicly for a long
time.

The Rust experiment does not require the new build system -- the goal
is that developers and maintainers can work on abstractions and
drivers and see whether the whole approach is possible or not.

Being able to compile the abstractions themselves as a module is, of
course, a requirement for leaving the experimentation phase.

> Documentation is still an open question for me. I know Rust can
> generate documentation from the code, but how do we integrate that
> into the existing kernel documentation?

I replied to that in the other thread [1]. The integration that we
really need is getting at least linking in the C to Rust direction;
but I suspect what you are asking for is to replace `rustdoc` or
similar from what you are saying. I don't think that would be a good
idea unless someone can get something substantially better than what
`rustdoc` produces.

[1] https://lore.kernel.org/rust-for-linux/CANiq72n3kHrJKXApx2eZ6MFisdoh==4KQi2qHYkxmDi=TYaHew@mail.gmail.com/

> When i build it on my machine, i get a million warnings from i think
> the linker. That is not going to be acceptable to Mainline, so we need
> to investigate that.

Yes, that is (sadly) expected, and I was at a cross-roads between
restricting it in the config or not, as I mentioned in the other
thread [2]. Given your message, let's take it as an opportunity to
change it now.

[2] https://lore.kernel.org/rust-for-linux/CANiq72m849ebmsVbfrPF3=UrjT=vawEyZ1=nSj6XHqAOEEieMQ@mail.gmail.com/

> I hope some sort of lessons learned, best practices and TODO list can
> be distilled from the experience, to help guide the Rust Experiment.

I appreciate that you are taking the time to have a look at the Rust
support, but please note that most things you are mentioning are not
really "lessons learned" -- they were things that were already known
and/or worked on.

On top of that, please note that we are not the ones that decided that
this driver / patch series was ready. In fact, we recommended
iterating it more before submitting it to the Rust for Linux mailing
list, and even less to the networking one. Mostly because there is
still not a driver merged, and things like this can create confusion
as you have seen (and there are others, way more complex, in the
works, but they have had more internal iterations with other Rust
subsystem people, which we appreciated).

Finally, yes, many things are not ready. That is expected, and the
Rust support is still experimental. We are still in the process of
merging things that we were working on in the past years (on the side
of the abstractions) and, on the infrastructure side, there is also a
lot of work to be done.

We never claimed this is ready for production and that we cover every
feature that C has. This includes other kernel subsystems, but also
tooling and external projects: there are things to do in `pahole`
(thanks Arnaldo for working on that), perhaps in `objtool` too (thanks
Josh for being open to work on that), in the Documentation (thanks
Jonathan and Carlos), in Coccinelle (Coccinelle for Rust has just been
publicly published, thanks Julia and Tathagata), in KernelCI (thanks
Guillaume et al. for getting the Rust builds working again last week),
in `rustc_codegen_gcc` (thanks Antoni for having made it work without
source-level patches in the kernel last month), GCC Rust (getting
closer), LKP/Intel 0-day, in the Rust compiler itself (e.g. recent
mitigations), in `bindgen` (e.g. the `enum` thing, the lack of
non-trivial constants support, the always-`u32`-incorrect C type for
`#define`s)...

What I mean to say with all this (and sorry to those I didn't list --
it was just a sample) is: yes, we are getting there, but it will still
take a while. We cannot really do everything at once, and there are
lots of things going on.

I would encourage others to join the weekly meetings to get up to
speed with the status and help getting everything ready. For
"external" projects, I track things in the lists linked at the top of
https://github.com/Rust-for-Linux/linux/issues/2.

Cheers,
Miguel

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

> > I hope some sort of lessons learned, best practices and TODO list can
> > be distilled from the experience, to help guide the Rust Experiment.
> 
> I appreciate that you are taking the time to have a look at the Rust
> support, but please note that most things you are mentioning are not
> really "lessons learned" -- they were things that were already known
> and/or worked on.

We are at the intersect of two worlds here. Maybe these issues are
well known in the linux for rust world, but they are not really known
to the netdev world, and to some extend the kernel developers /
maintainers world. We need to spread knowledge between each world.

So maybe this "lessons learned" is not really for the Rust people, but
for the netdev community, and kernel developers and Maintainers in
general?

	Andrew

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 4:31 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> We are at the intersect of two worlds here. Maybe these issues are
> well known in the linux for rust world, but they are not really known
> to the netdev world, and to some extend the kernel developers /
> maintainers world. We need to spread knowledge between each world.

Indeed!

We are presenting in netdevconf 0x17 in a few weeks a tutorial [1] to
try to cover a bit of that with a tutorial on Rust & networking
(thanks Jamal for inviting us). Hopefully that helps a bit...

[1] https://netdevconf.info/0x17/sessions/tutorial/rust-for-linux-networking-tutorial.html

And, of course, there is LPC & Kangrejos (the latter already happened
3 weeks ago -- we had most of the slides already up at
https://kangrejos.com). We also had some LF Live: Mentorship Series
done in the past.

> So maybe this "lessons learned" is not really for the Rust people, but
> for the netdev community, and kernel developers and Maintainers in
> general?

I apologize -- I didn't mean to say that you should know about those
things. Just that, we are trying to do our best to get things ready in
the best way possible, while letting people "play" meanwhile with the
abstractions and so on.

Cheers,
Miguel

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:27 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I apologize -- I didn't mean to say that you should know about those
> things. Just that, we are trying to do our best to get things ready in
> the best way possible, while letting people "play" meanwhile with the
> abstractions and so on.

I forgot to mention: we have been planning a "networking call" for a
while for everybody interested in it in e.g. Google Meet. Initially it
was going to be tomorrow, but we are rescheduling now.

The goal of the call is to get the different parties involved, since
there are quite a few trying to upstream different bits and pieces
around networking. In particular, we want to discuss having
a`rust-net` branch where everybody can work together on the networking
abstractions and iterate there.

So that is another alternative. Of course, the `rust-net` branch could
be in the networking tree instead.

Please feel free to join, you would be very welcome (and anybody else
from netdev, of course).

Cheers,
Miguel

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

> The goal of the call is to get the different parties involved, since
> there are quite a few trying to upstream different bits and pieces
> around networking. In particular, we want to discuss having
> a`rust-net` branch where everybody can work together on the networking
> abstractions and iterate there.
> 
> So that is another alternative. Of course, the `rust-net` branch could
> be in the networking tree instead.

My experience is, you need to use the netdev mailing list. Anything
which is not developed in full few of netdev is very likely to get
ripped to shreds and has no chance of being merged. As an extension to
that, you should be targeting net-next.

The mailing list has multiple purposes, one of which is education. The
netdev community needs to learn about Rust, in the same way the Rust
community needs to learn about netdev. If this experiment is a
success, i expect Rust code to be no different to C code. It gets
posted to netdev, it gets run through the netdev CI, and eventually
merged via net-next.

	Andrew

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Greg KH]

On Mon, Oct 09, 2023 at 10:39:12AM +0900, FUJITA Tomonori wrote:
> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
> features are equivalent. You can choose C or Rust 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          |   2 +
>  4 files changed, 143 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 421d2b62918f..0317be180ac2 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 version driver for Asix PHYs"
> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
> +	help
> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
> +	  instead of the C version.

This does not properly describe what hardware this driver supports.  And
that's an odd way to describe the module name, but I see none of the
other entries in this file do that either, so maybe the PHY subsystm
doesn't require that?

thanks,

greg k-h

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

On Mon, 9 Oct 2023 12:10:11 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 09, 2023 at 10:39:12AM +0900, FUJITA Tomonori wrote:
>> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
>> features are equivalent. You can choose C or Rust 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          |   2 +
>>  4 files changed, 143 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 421d2b62918f..0317be180ac2 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 version driver for Asix PHYs"
>> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
>> +	help
>> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
>> +	  instead of the C version.
> 
> This does not properly describe what hardware this driver supports.  And

I'll add (by copying the description of the C driver).


> that's an odd way to describe the module name, but I see none of the
> other entries in this file do that either, so maybe the PHY subsystm
> doesn't require that?

I leave it to Andrew. This is the first driver in Rust so I thought
that users had no idea the relationship between the source file name
and the module file name.

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Benno Lossin]

On 09.10.23 03:39, FUJITA Tomonori wrote:
> 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>
> ---

One small nit below, you can decide what to do with that.

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

>   drivers/net/phy/Kconfig          |   7 ++
>   drivers/net/phy/Makefile         |   6 +-
>   drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
>   rust/uapi/uapi_helper.h          |   2 +
>   4 files changed, 143 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 421d2b62918f..0317be180ac2 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 version driver for Asix PHYs"
> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
> +	help
> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
> +	  instead of the C version.
> +
>   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..017f817f6f8d
> --- /dev/null
> +++ b/drivers/net/phy/ax88796b_rust.rs
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! Rust Asix PHYs driver
> +//!
> +//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
> +use kernel::c_str;
> +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>()
> +    ],
> +    name: "rust_asix_phy",
> +    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
> +    description: "Rust Asix PHYs driver",
> +    license: "GPL",
> +}
> +
> +// 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(uapi::SPEED_100);
> +        } else {
> +            dev.set_speed(uapi::SPEED_10);
> +        }

Maybe refactor to only have one `dev.set_speed` call?

> +
> +        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..08f5e9334c9e 100644
> --- a/rust/uapi/uapi_helper.h
> +++ b/rust/uapi/uapi_helper.h
> @@ -7,3 +7,5 @@
>    */
> 
>   #include <uapi/asm-generic/ioctl.h>
> +#include <uapi/linux/mii.h>
> +#include <uapi/linux/ethtool.h>
> --
> 2.34.1
> 
> 

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

> > +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> > +            dev.set_speed(uapi::SPEED_100);
> > +        } else {
> > +            dev.set_speed(uapi::SPEED_10);
> > +        }
> 
> Maybe refactor to only have one `dev.set_speed` call?

This is a common pattern in the C code. This is basically a
re-implementation of

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432

because this PHY is broken. Being one of the maintainers of the PHY
subsystem, it helps me review this code if it happens to look like the
existing code it is adding a workaround to.

Is there a Rust reason to only have one call?

   Andrew

Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

[Benno Lossin]

On 09.10.23 15:15, Andrew Lunn wrote:
>>> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
>>> +            dev.set_speed(uapi::SPEED_100);
>>> +        } else {
>>> +            dev.set_speed(uapi::SPEED_10);
>>> +        }
>>
>> Maybe refactor to only have one `dev.set_speed` call?
> 
> This is a common pattern in the C code. This is basically a
> re-implementation of
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432
> 
> because this PHY is broken. Being one of the maintainers of the PHY
> subsystem, it helps me review this code if it happens to look like the
> existing code it is adding a workaround to.
> 
> Is there a Rust reason to only have one call?

My reason was consistency, since the call to `set_duplex`
below that was changed to only have one call:

+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };
+        dev.set_duplex(duplex);

I think it should be consistent, I chose to reduce the number of
function calls, since it is immediately obvious that only the argument
is depending on the condition. But if you think it should mirror the C
side, then maybe change the duplex back to calling twice?

-- 
Cheers,
Benno

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 10:39:09AM +0900, FUJITA Tomonori wrote:
> This patchset adds Rust abstractions for phylib. It doesn't fully
> cover the C APIs yet but I think that it's already useful. I implement
> two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems
> they work well with real hardware.
> 
> The first patch introduces Rust bindings for phylib.
> 
> The second patch updates the ETHERNET PHY LIBRARY entry in MAINTAINERS
> file; adds the binding file and 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.

I at last got around to installing a rust tool chain and tried to
build the code. I get what looks like linker errors.

linux$ make LLVM=1 rustavailable
Rust is available!

dpkg says:

+++-==============-============-============-=================================
ii  llvm           1:16.0-57    amd64        Low-Level Virtual Machine (LLVM)

I build with

make LLVM=1

and get lots of warnings like:

vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeANtNtNtB4_5ascii10ascii_char9AsciiCharja_EB4_+0x0: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeFUPuENtNtNtB4_4task4wake8RawWakerEB4_+0x0: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeINtNtB4_6option6OptionINtNtNtNtB4_4iter8adapters7flatten7FlattenINtBJ_8IntoIterNtNtB4_4char11EscapeDebugEEEEB4_+0x0: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeNtNtB4_3fmt5ErrorEB4_+0x0: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placesEB4_+0x0: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f8classify+0x5a: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f16partial_classify+0x1f: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f13classify_bits+0x28: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f7next_up+0x32: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f9next_down+0x34: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f8midpoint+0xc3: 'naked' return found in RETHUNK build
vmlinux.o: warning: objtool: _RNvNvMNtCs4KbNGwnAC5t_4core3f32f7to_bits13ct_f32_to_u32+0x4a: 'naked' return found in RETHUNK build

Any ideas?

    Andrew

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Any ideas?

That is `RETHUNK` and `X86_KERNEL_IBT`.

Since this will keep confusing people, I will make it a `depends on !`
as discussed in the past. I hope it is OK for e.g. Andrea.

Cheers,
Miguel

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

[Greg KH]

On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Any ideas?
> 
> That is `RETHUNK` and `X86_KERNEL_IBT`.
> 
> Since this will keep confusing people, I will make it a `depends on !`
> as discussed in the past. I hope it is OK for e.g. Andrea.

That's not ok as you want that option enabled on systems that have those
broken processors which need this option for proper security.  You would
be forcing people to disable this to enable Rust support?

confused,

greg k-h

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 3:46 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> That's not ok as you want that option enabled on systems that have those
> broken processors which need this option for proper security.  You would
> be forcing people to disable this to enable Rust support?

Yes, that is what would happen. But if we want to avoid the warnings
and be proper (even if there are no real users of Rust yet), until the
Rust compiler supports it and we wire it up, the only way is that, no?

I think PeterZ preferred that we restricted it, and at this point I
think it is a good idea to do so now in case somebody thinks this
works.

Cheers,
Miguel

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

[Greg KH]

On Mon, Oct 09, 2023 at 04:13:02PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 3:46 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > That's not ok as you want that option enabled on systems that have those
> > broken processors which need this option for proper security.  You would
> > be forcing people to disable this to enable Rust support?
> 
> Yes, that is what would happen. But if we want to avoid the warnings
> and be proper (even if there are no real users of Rust yet), until the
> Rust compiler supports it and we wire it up, the only way is that, no?

Then the main CONFIG_HAVE_RUST should have that dependency, don't force
it on each individual driver.

But note, that is probably not a good marketing statement as you are
forced to make your system more insecure in order to use the "secure"
language :(

thanks,

greg k-h

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 4:52 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Then the main CONFIG_HAVE_RUST should have that dependency, don't force
> it on each individual driver.

Yes, that is what I meant (well, `CONFIG_RUST` is where we have the
other restrictions).

> But note, that is probably not a good marketing statement as you are
> forced to make your system more insecure in order to use the "secure"
> language :(

Indeed, but until we catch up on that, it is what it is; i.e. it is
not something that we want to keep there, it has to go away to make it
viable.

The other option we discussed back then was to print a big banner or
something at runtime, but that is also not great (and people would
still see warnings at build time -- for good reason).

Cheers,
Miguel

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

[Greg KH]

On Mon, Oct 09, 2023 at 05:06:45PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 4:52 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > Then the main CONFIG_HAVE_RUST should have that dependency, don't force
> > it on each individual driver.
> 
> Yes, that is what I meant (well, `CONFIG_RUST` is where we have the
> other restrictions).

Oops, yes, add it there please.

> > But note, that is probably not a good marketing statement as you are
> > forced to make your system more insecure in order to use the "secure"
> > language :(
> 
> Indeed, but until we catch up on that, it is what it is; i.e. it is
> not something that we want to keep there, it has to go away to make it
> viable.

Is anyone working on the needed compiler changes for this to work
properly on x86?

> The other option we discussed back then was to print a big banner or
> something at runtime, but that is also not great (and people would
> still see warnings at build time -- for good reason).

No, please don't do that, you would be making systems insecure and the
mix of a kernel image with, and without, RET statements in it is going
to be a huge mess.  Just disable CONFIG_RUST for now until proper
retbleed support is added to the compiler.

thanks,

greg k-h

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Is anyone working on the needed compiler changes for this to work
> properly on x86?

I don't know, I will ask.

Cheers,
Miguel

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Any ideas?
> 
> That is `RETHUNK` and `X86_KERNEL_IBT`.
> 
> Since this will keep confusing people, I will make it a `depends on !`
> as discussed in the past. I hope it is OK for e.g. Andrea.

I really do suggest you work on your kconfig. The expectation is any
configuration that kconfig is happy with will build. People like Arnd
Bergmann do lots of randconfig builds. We don't want his work upset by
Rust code.

And as a Rust beginning, i find this pretty unfriendly, in that i
followed https://docs.kernel.org/rust/quick-start.html but did not get
a working build.

    Andrew

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 3:25 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I really do suggest you work on your kconfig. The expectation is any
> configuration that kconfig is happy with will build. People like Arnd
> Bergmann do lots of randconfig builds. We don't want his work upset by
> Rust code.
>
> And as a Rust beginning, i find this pretty unfriendly, in that i
> followed https://docs.kernel.org/rust/quick-start.html but did not get
> a working build.

It is a "working" build: some people are actually using it as-is on
purpose (with the warnings, I mean).

Yes, it is bad, and we did not like it when they told us their build
had those warnings and still used it, but it means it will make it
harder for them when we restrict it.

But your message is the perfect excuse for me to send the patch to
restrict it, so thanks :)

Cheers,
Miguel

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

[Andrea Righi]

On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Any ideas?
> 
> That is `RETHUNK` and `X86_KERNEL_IBT`.
> 
> Since this will keep confusing people, I will make it a `depends on !`
> as discussed in the past. I hope it is OK for e.g. Andrea.

Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
If that constraint is introduced we either need to revert that patch
in the Ubuntu kernel or disable Rust support.

It would be nice to have a least something like
CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have
`RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`.

-Andrea

> 
> Cheers,
> Miguel

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 4:21 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
> If that constraint is introduced we either need to revert that patch
> in the Ubuntu kernel or disable Rust support.

Yeah, I was expecting that you would disable the Rust support, of
course (not that you disable the security option! :).

Cheers,
Miguel

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

[Andrew Lunn]

On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote:
> On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > Any ideas?
> > 
> > That is `RETHUNK` and `X86_KERNEL_IBT`.
> > 
> > Since this will keep confusing people, I will make it a `depends on !`
> > as discussed in the past. I hope it is OK for e.g. Andrea.
> 
> Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
> If that constraint is introduced we either need to revert that patch
> in the Ubuntu kernel or disable Rust support.
> 
> It would be nice to have a least something like
> CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have
> `RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`.

Should this actually be CONFIG_RUST_IS_BROKEN_ON_X86_BUT_IM_HAPPY ?

At lest for networking, the code is architecture independent. For a
driver to be useful, it needs to compile for most architectures. So i
hope Rust will quickly make it to other architectures. And for PHY
drivers, ARM and MIPS are probably more important than x86.

	Andrew

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

[Greg KH]

On Mon, Oct 09, 2023 at 04:56:36PM +0200, Andrew Lunn wrote:
> On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote:
> > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > Any ideas?
> > > 
> > > That is `RETHUNK` and `X86_KERNEL_IBT`.
> > > 
> > > Since this will keep confusing people, I will make it a `depends on !`
> > > as discussed in the past. I hope it is OK for e.g. Andrea.
> > 
> > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
> > If that constraint is introduced we either need to revert that patch
> > in the Ubuntu kernel or disable Rust support.
> > 
> > It would be nice to have a least something like
> > CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have
> > `RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`.
> 
> Should this actually be CONFIG_RUST_IS_BROKEN_ON_X86_BUT_IM_HAPPY ?

Just do the proper dependency on RETHUNK and you should be fine, it will
be able to be enabled on arches that do not require RETHUNK for proper
security.

> At lest for networking, the code is architecture independent. For a
> driver to be useful, it needs to compile for most architectures. So i
> hope Rust will quickly make it to other architectures. And for PHY
> drivers, ARM and MIPS are probably more important than x86.

Is MIPS a proper target for rust yet?

thanks,

greg k-h

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Is MIPS a proper target for rust yet?

The compiler has support for it
(https://github.com/Rust-for-Linux/linux/issues/107), but I didn't do
mips pre-merge.

The ones I tried (and that we had in the CI back then pre-merge) were:
arm, arm64, ppc64le, riscv64 and x86_64.

Cheers,
Miguel

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

[Miguel Ojeda]

On Mon, Oct 9, 2023 at 5:10 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> The compiler has support for it
> (https://github.com/Rust-for-Linux/linux/issues/107), but I didn't do
> mips pre-merge.
>
> The ones I tried (and that we had in the CI back then pre-merge) were:
> arm, arm64, ppc64le, riscv64 and x86_64.

By "in the CI" here means: booted in QEMU with a given kernel config.

Also, I should have said that Michael Ellerman was the one that added
the ppc64le one.

Cheers,
Miguel

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

[Greg KH]

On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote:
> On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > Any ideas?
> > 
> > That is `RETHUNK` and `X86_KERNEL_IBT`.
> > 
> > Since this will keep confusing people, I will make it a `depends on !`
> > as discussed in the past. I hope it is OK for e.g. Andrea.
> 
> Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
> If that constraint is introduced we either need to revert that patch
> in the Ubuntu kernel or disable Rust support.

Why is rust enabled in the Ubuntu kernel as there is no in-kernel
support for any real functionality?  Or do you have out-of-tree rust
drivers added to your kernel already?

thanks,

greg k-h

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

[Andrea Righi]

On Mon, Oct 09, 2023 at 04:56:47PM +0200, Greg KH wrote:
> On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote:
> > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote:
> > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > Any ideas?
> > > 
> > > That is `RETHUNK` and `X86_KERNEL_IBT`.
> > > 
> > > Since this will keep confusing people, I will make it a `depends on !`
> > > as discussed in the past. I hope it is OK for e.g. Andrea.
> > 
> > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel.
> > If that constraint is introduced we either need to revert that patch
> > in the Ubuntu kernel or disable Rust support.
> 
> Why is rust enabled in the Ubuntu kernel as there is no in-kernel
> support for any real functionality?  Or do you have out-of-tree rust
> drivers added to your kernel already?

Rust in the Ubuntu kernel is just a "technology preview", enabled in the
development release only. The idea is to provide all the toolchain,
dependencies, headers, etc. in the generic distro kernel, so those who
are willing to do experiments with Rust can do that without installing a
custom kernel.

And as soon as new Rust abstractions will be merged upstream we already
have the infrastructure that would allow anybody to use them with all
the components provided by the distro.

So, we really don't have any out-of-tree module/driver that requires
Rust at the moment.

-Andrea

> 
> thanks,
> 
> greg k-h