[PATCH net-next v7 0/5] Rust abstractions for network PHY drivers

[PATCH net-next v7 0/5] 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 add a macro to declare a kernel module for PHYs
drivers.

The third patch add Miguel's work to make sure that the C's enum is
sync with Rust sides. If not, compiling fails. Note that this is a
temporary solution. It will be replaced with bindgen when it supports
generating the enum conversion code.

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

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

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


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

Miguel Ojeda (1):
  rust: add second `bindgen` pass for enum exhaustiveness checking

 MAINTAINERS                          |  16 +
 drivers/net/phy/Kconfig              |  16 +
 drivers/net/phy/Makefile             |   6 +-
 drivers/net/phy/ax88796b_rust.rs     | 129 ++++
 rust/.gitignore                      |   1 +
 rust/Makefile                        |  14 +
 rust/bindings/bindings_enum_check.rs |  36 ++
 rust/bindings/bindings_helper.h      |   3 +
 rust/kernel/lib.rs                   |   3 +
 rust/kernel/net.rs                   |   6 +
 rust/kernel/net/phy.rs               | 868 +++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h              |   2 +
 12 files changed, 1099 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs
 create mode 100644 rust/bindings/bindings_enum_check.rs
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs


base-commit: 8846f9a04b10b7f61214425409838d764df7080d
-- 
2.34.1

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

[FUJITA Tomonori]

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

This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.

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

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

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

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 421d2b62918f..0faebdb184ca 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -60,6 +60,14 @@ config FIXED_PHY
 
 	  Currently tested with mpc866ads and mpc8349e-mitx.
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "PHYLIB abstractions support"
+        depends on RUST
+        depends on PHYLIB=y
+        help
+          Adds support needed for PHY drivers written in Rust. It provides
+          a wrapper around the C phylib core.
+
 config SFP
 	tristate "SFP cage support"
 	depends on I2C && PHYLINK
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 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..fe415cb369d3
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..61223f638bc6
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,725 @@
+// 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, Pin},
+    str::CStr,
+    types::Opaque,
+};
+use core::marker::PhantomData;
+
+/// PHY state machine states.
+///
+/// Corresponds to the kernel's [`enum phy_state`].
+///
+/// Some of PHY drivers access to the state of PHY's software state machine.
+///
+/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
+#[derive(PartialEq)]
+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+}
+
+/// A mode of Ethernet communication.
+///
+/// PHY drivers get duplex information from hardware and update the current state.
+pub enum DuplexMode {
+    /// PHY is in full-duplex mode.
+    Full,
+    /// PHY is in half-duplex mode.
+    Half,
+    /// PHY is in unknown duplex mode.
+    Unknown,
+}
+
+/// An instance of a PHY device.
+///
+/// Wraps the kernel's [`struct phy_device`].
+///
+/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
+/// executes [`Driver`]'s methods during the callback.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+///
+/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
+// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
+// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
+// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
+// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
+// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
+// to the instance.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// This function must only be called from the callbacks in `phy_driver`.
+    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
+        let ptr = ptr.cast::<Self>();
+        // SAFETY: by the function requirements the pointer is valid and we have unique access for
+        // the duration of `'a`.
+        unsafe { &mut *ptr }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn phy_id(&self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Gets the state of PHY state machine states.
+    pub fn state(&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 };
+        // TODO: this conversion code will be replaced with automatically generated code by bindgen
+        // when it becomes possible.
+        // better to call WARN_ONCE() when the state is out-of-range.
+        match state {
+            bindings::phy_state_PHY_DOWN => DeviceState::Down,
+            bindings::phy_state_PHY_READY => DeviceState::Ready,
+            bindings::phy_state_PHY_HALTED => DeviceState::Halted,
+            bindings::phy_state_PHY_ERROR => DeviceState::Error,
+            bindings::phy_state_PHY_UP => DeviceState::Up,
+            bindings::phy_state_PHY_RUNNING => DeviceState::Running,
+            bindings::phy_state_PHY_NOLINK => DeviceState::NoLink,
+            bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest,
+            _ => DeviceState::Error,
+        }
+    }
+
+    /// Gets the current link state.
+    ///
+    /// It returns true if the link is up.
+    pub fn is_link_up(&self) -> bool {
+        const LINK_IS_UP: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.link() == LINK_IS_UP
+    }
+
+    /// Gets the current auto-negotiation configuration.
+    ///
+    /// It returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&self) -> bool {
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.autoneg() == bindings::AUTONEG_ENABLE
+    }
+
+    /// Gets the current auto-negotiation state.
+    ///
+    /// It returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&self) -> bool {
+        const AUTONEG_COMPLETED: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        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.
+    // This function reads a hardware register and updates the stats so takes `&mut self`.
+    pub fn read(&mut self, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Writes a given C22 PHY register.
+    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
+        })
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Resolves the advertisements into PHY settings.
+    pub fn resolve_aneg_linkmode(&mut self) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
+    }
+
+    /// Executes software reset the PHY via `BMCR_RESET` bit.
+    pub fn genphy_soft_reset(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // so an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_init_hw(phydev) })
+    }
+
+    /// Starts auto-negotiation.
+    pub fn start_aneg(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_start_aneg(phydev) })
+    }
+
+    /// Resumes the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_resume(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_resume(phydev) })
+    }
+
+    /// Suspends the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_suspend(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_suspend(phydev) })
+    }
+
+    /// Checks the link status and updates current link state.
+    pub fn genphy_read_status(&mut self) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Updates the link status.
+    pub fn genphy_update_link(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_update_link(phydev) })
+    }
+
+    /// Reads link partner ability.
+    pub fn genphy_read_lpa(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
+    }
+
+    /// Reads PHY abilities.
+    pub fn genphy_read_abilities(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
+    }
+}
+
+/// Defines certain other features this PHY supports (like interrupts).
+///
+/// These flag values are used in [`Driver::FLAGS`].
+pub mod flags {
+    /// PHY is internal.
+    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
+    /// PHY needs to be reset after the refclk is enabled.
+    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
+    /// Polling is used to detect PHY status changes.
+    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
+    /// Don't suspend.
+    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
+}
+
+/// An adapter for the registration of a PHY driver.
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: 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);
+    }
+}
+
+/// Driver structure for a particular PHY type.
+///
+/// Wraps the kernel's [`struct phy_driver`].
+/// This is used to register a driver for a particular PHY type with the kernel.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+///
+/// [`struct phy_driver`]: ../../../../../../../include/linux/phy.h
+#[repr(transparent)]
+pub struct DriverVTable(Opaque<bindings::phy_driver>);
+
+/// Creates a [`DriverVTable`] instance from [`Driver`].
+///
+/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
+///
+/// [`module_phy_driver`]: crate::module_phy_driver
+pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
+    // INVARIANT: All the fields of `struct phy_driver` are initialized properly.
+    DriverVTable(Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr().cast_mut(),
+        flags: T::FLAGS,
+        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
+        soft_reset: if T::HAS_SOFT_RESET {
+            Some(Adapter::<T>::soft_reset_callback)
+        } else {
+            None
+        },
+        get_features: if T::HAS_GET_FEATURES {
+            Some(Adapter::<T>::get_features_callback)
+        } else {
+            None
+        },
+        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
+            Some(Adapter::<T>::match_phy_device_callback)
+        } else {
+            None
+        },
+        suspend: if T::HAS_SUSPEND {
+            Some(Adapter::<T>::suspend_callback)
+        } else {
+            None
+        },
+        resume: if T::HAS_RESUME {
+            Some(Adapter::<T>::resume_callback)
+        } else {
+            None
+        },
+        config_aneg: if T::HAS_CONFIG_ANEG {
+            Some(Adapter::<T>::config_aneg_callback)
+        } else {
+            None
+        },
+        read_status: if T::HAS_READ_STATUS {
+            Some(Adapter::<T>::read_status_callback)
+        } else {
+            None
+        },
+        read_mmd: if T::HAS_READ_MMD {
+            Some(Adapter::<T>::read_mmd_callback)
+        } else {
+            None
+        },
+        write_mmd: if T::HAS_WRITE_MMD {
+            Some(Adapter::<T>::write_mmd_callback)
+        } else {
+            None
+        },
+        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
+            Some(Adapter::<T>::link_change_notify_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+    }))
+}
+
+/// Driver implementation for a particular PHY type.
+///
+/// This trait is used to create a [`DriverVTable`].
+#[vtable]
+pub trait Driver {
+    /// Defines certain other features this PHY supports.
+    /// It is a combination of the flags in the [`flags`] module.
+    const FLAGS: u32 = 0;
+
+    /// The friendly name of this PHY type.
+    const NAME: &'static CStr;
+
+    /// This driver only works for PHYs with IDs which match this field.
+    /// The default id and mask are zero.
+    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
+
+    /// Issues a PHY software reset.
+    fn soft_reset(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Probes the hardware to determine what abilities it has.
+    fn get_features(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Returns true if this is a suitable driver for the given phydev.
+    /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
+    fn match_phy_device(_dev: &Device) -> bool {
+        false
+    }
+
+    /// Configures the advertisement and resets auto-negotiation
+    /// if auto-negotiation is enabled.
+    fn config_aneg(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Determines the negotiated speed and duplex.
+    fn read_status(_dev: &mut Device) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Suspends the hardware, saving state if needed.
+    fn suspend(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Resumes the hardware, restoring state if needed.
+    fn resume(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD read function for reading a MMD register.
+    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD write function for writing a MMD register.
+    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Callback for notification of link change.
+    fn link_change_notify(_dev: &mut Device) {}
+}
+
+/// Registration structure for PHY drivers.
+///
+/// Registers [`DriverVTable`] instances with the kernel. They will be unregistered when dropped.
+///
+/// # Invariants
+///
+/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: Pin<&'static mut [DriverVTable]>,
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    pub fn register(
+        module: &'static crate::ThisModule,
+        drivers: Pin<&'static mut [DriverVTable]>,
+    ) -> Result<Self> {
+        if drivers.is_empty() {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of
+        // the `drivers` slice are initialized properly. `drivers` will not be moved.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
+        })?;
+        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
+        Ok(Registration { drivers })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `self.drivers` is valid.
+        // So an FFI call with a valid pointer.
+        unsafe {
+            bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32)
+        };
+    }
+}
+
+// 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 {}
+
+/// An identifier for PHY devices on an MDIO/MII bus.
+///
+/// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate
+/// PHY driver.
+pub struct DeviceId {
+    id: u32,
+    mask: DeviceMask,
+}
+
+impl DeviceId {
+    /// Creates a new instance with the exact match mask.
+    pub const fn new_with_exact_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Exact,
+        }
+    }
+
+    /// Creates a new instance with the model match mask.
+    pub const fn new_with_model_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Model,
+        }
+    }
+
+    /// Creates a new instance with the vendor match mask.
+    pub const fn new_with_vendor_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Vendor,
+        }
+    }
+
+    /// Creates a new instance with a custom match mask.
+    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Custom(mask),
+        }
+    }
+
+    /// Creates a new instance from [`Driver`].
+    pub const fn new_with_driver<T: Driver>() -> Self {
+        T::PHY_DEVICE_ID
+    }
+
+    /// Get a `mask` as u32.
+    pub const fn mask_as_int(&self) -> u32 {
+        self.mask.as_int()
+    }
+
+    // macro use only
+    #[doc(hidden)]
+    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
+        bindings::mdio_device_id {
+            phy_id: self.id,
+            phy_id_mask: self.mask.as_int(),
+        }
+    }
+}
+
+enum DeviceMask {
+    Exact,
+    Model,
+    Vendor,
+    Custom(u32),
+}
+
+impl DeviceMask {
+    const MASK_EXACT: u32 = !0;
+    const MASK_MODEL: u32 = !0 << 4;
+    const MASK_VENDOR: u32 = !0 << 10;
+
+    const fn as_int(&self) -> u32 {
+        match self {
+            DeviceMask::Exact => Self::MASK_EXACT,
+            DeviceMask::Model => Self::MASK_MODEL,
+            DeviceMask::Vendor => Self::MASK_VENDOR,
+            DeviceMask::Custom(mask) => *mask,
+        }
+    }
+}
-- 
2.34.1

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

[Boqun Feng]

On Thu, Oct 26, 2023 at 09:10:46AM +0900, FUJITA Tomonori wrote:
[...]
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "PHYLIB abstractions support"

bool "Rust PHYLIB abstractions support"

maybe? Make it easier for menuconfig users to know this is the option
to enable Rust support.

Regards,
Boqun

> +        depends on RUST
> +        depends on PHYLIB=y
> +        help
> +          Adds support needed for PHY drivers written in Rust. It provides
> +          a wrapper around the C phylib core.
> +
[...]

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

[FUJITA Tomonori]

On Fri, 27 Oct 2023 12:09:41 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Thu, Oct 26, 2023 at 09:10:46AM +0900, FUJITA Tomonori wrote:
> [...]
>> +config RUST_PHYLIB_ABSTRACTIONS
>> +        bool "PHYLIB abstractions support"
> 
> bool "Rust PHYLIB abstractions support"
> 
> maybe? Make it easier for menuconfig users to know this is the option
> to enable Rust support.

Surely, updated.

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 11b18370a05b..1fa84a188bcc 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -61,7 +61,7 @@ config FIXED_PHY
 	  Currently tested with mpc866ads and mpc8349e-mitx.
 
 config RUST_PHYLIB_ABSTRACTIONS
-        bool "PHYLIB abstractions support"
+        bool "Rust PHYLIB abstractions support"
         depends on RUST
         depends on PHYLIB=y
         help

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

[Boqun Feng]

On Thu, Oct 26, 2023 at 09:10:46AM +0900, FUJITA Tomonori wrote:
[...]
> +    /// Gets the current link state.
> +    ///
> +    /// It returns true if the link is up.
> +    pub fn is_link_up(&self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };

Tomo, FWIW, the above line means *copying* the content pointed by
`self.0.get()` into `phydev`, i.e. `phydev` is the semantically a copy
of the `phy_device` instead of an alias. In C code, it means you did:

	struct phy_device phydev = *ptr;

Sure, both compilers can figure this out, therefore no extra copy is
done, but still it's better to avoid this copy semantics by doing:

	let phydev = unsafe { &*self.0.get() };

it's equal to C code:

	struct phy_device *phydev = ptr;

Ditto for is_autoneg_enabled() and is_autoneg_completed().

Regards,
Boqun

> +        phydev.link() == LINK_IS_UP
> +    }
> +
> +    /// Gets the current auto-negotiation configuration.
> +    ///
> +    /// It returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&self) -> bool {
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg() == bindings::AUTONEG_ENABLE
> +    }
> +
> +    /// Gets the current auto-negotiation state.
> +    ///
> +    /// It returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
[...]

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

[Benno Lossin]

On 10/27/23 21:59, Boqun Feng wrote:
> On Thu, Oct 26, 2023 at 09:10:46AM +0900, FUJITA Tomonori wrote:
> [...]
>> +    /// Gets the current link state.
>> +    ///
>> +    /// It returns true if the link is up.
>> +    pub fn is_link_up(&self) -> bool {
>> +        const LINK_IS_UP: u32 = 1;
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let phydev = unsafe { *self.0.get() };
> 
> Tomo, FWIW, the above line means *copying* the content pointed by
> `self.0.get()` into `phydev`, i.e. `phydev` is the semantically a copy
> of the `phy_device` instead of an alias. In C code, it means you did:

Good catch. `phy_device` is rather large (did not look at the exact
size) and this will not be optimized on debug builds, so it could lead
to stackoverflows.

> 	struct phy_device phydev = *ptr;
> 
> Sure, both compilers can figure this out, therefore no extra copy is
> done, but still it's better to avoid this copy semantics by doing:
> 
> 	let phydev = unsafe { &*self.0.get() };

We need to be careful here, since doing this creates a reference
`&bindings::phy_device` which asserts that it is immutable. That is not
the case, since the C side might change it at any point (this is the
reason we wrap things in `Opaque`, since that allows mutatation even
through sharde references).

I did not notice this before, but this means we cannot use the `link`
function from bindgen, since that takes `&self`. We would need a
function that takes `*const Self` instead.

-- 
Cheers,
Benno

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

[Boqun Feng]

On Fri, Oct 27, 2023 at 09:19:38PM +0000, Benno Lossin wrote:
[...]
> 
> Good catch. `phy_device` is rather large (did not look at the exact
> size) and this will not be optimized on debug builds, so it could lead
> to stackoverflows.
> 

IIRC, kernel only supports O2 build, but yes, if we don't want the copy
here, we should avoid the copy semantics.

> > 	struct phy_device phydev = *ptr;
> > 
> > Sure, both compilers can figure this out, therefore no extra copy is
> > done, but still it's better to avoid this copy semantics by doing:
> > 
> > 	let phydev = unsafe { &*self.0.get() };
> 
> We need to be careful here, since doing this creates a reference
> `&bindings::phy_device` which asserts that it is immutable. That is not
> the case, since the C side might change it at any point (this is the
> reason we wrap things in `Opaque`, since that allows mutatation even
> through sharde references).
> 
> I did not notice this before, but this means we cannot use the `link`
> function from bindgen, since that takes `&self`. We would need a
> function that takes `*const Self` instead.
> 

Hmm... but does it mean even `set_speed()` has the similar issue?

	let phydev: *mut phy_device = self.0.get();
	unsafe { (*phydev).speed = ...; }

The `(*phydev)` creates a `&mut` IIUC. So we need the following maybe?

	let phydev: *mut phy_device = self.0.get();
	unsafe { *addr_mut_of!((*phydev).speed) = ...; }

because at least from phylib core guarantee, we know no one accessing
`speed` in the same time. However, yes, bit fields are tricky...

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 

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

[Andrew Lunn]

> Hmm... but does it mean even `set_speed()` has the similar issue?
> 
> 	let phydev: *mut phy_device = self.0.get();
> 	unsafe { (*phydev).speed = ...; }
> 
> The `(*phydev)` creates a `&mut` IIUC. So we need the following maybe?
> 
> 	let phydev: *mut phy_device = self.0.get();
> 	unsafe { *addr_mut_of!((*phydev).speed) = ...; }
> 
> because at least from phylib core guarantee, we know no one accessing
> `speed` in the same time. However, yes, bit fields are tricky...

Speed is not a bitfield. Its a plain boring int. link is however a bit
field.

	Andrew

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

[Benno Lossin]

On 10/28/23 00:21, Boqun Feng wrote:
> On Fri, Oct 27, 2023 at 09:19:38PM +0000, Benno Lossin wrote:
>> We need to be careful here, since doing this creates a reference
>> `&bindings::phy_device` which asserts that it is immutable. That is not
>> the case, since the C side might change it at any point (this is the
>> reason we wrap things in `Opaque`, since that allows mutatation even
>> through sharde references).
>>
>> I did not notice this before, but this means we cannot use the `link`
>> function from bindgen, since that takes `&self`. We would need a
>> function that takes `*const Self` instead.
>>
> 
> Hmm... but does it mean even `set_speed()` has the similar issue?
> 
> 	let phydev: *mut phy_device = self.0.get();
> 	unsafe { (*phydev).speed = ...; }

No that should be fine, take a look at the MIR output of the following 
code [1]:

    struct Foo {
        a: usize,
        b: usize,
    }
    
    fn foo(ptr: *mut Foo) {
        unsafe { (*ptr).b = 0; }
    }
    
    fn bar(ptr: *mut Foo) {
        unsafe { (&mut *ptr).b = 0; }
    }

Aside from some alignment checking, foo's MIR looks like this:

    bb1: {
        ((*_1).1: usize) = const 0_usize;
        return;
    }

And bar's MIR like this:

    bb1: {
        _2 = &mut (*_1);
        ((*_2).1: usize) = const 0_usize;
        return;
    }

[1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f7c4d87bf29a64af0acc09ff75d3716d

So I think that is fine, but maybe Gary has something else to say about it.

> The `(*phydev)` creates a `&mut` IIUC. So we need the following maybe?
> 
> 	let phydev: *mut phy_device = self.0.get();
> 	unsafe { *addr_mut_of!((*phydev).speed) = ...; }
> 
> because at least from phylib core guarantee, we know no one accessing
> `speed` in the same time. However, yes, bit fields are tricky...

-- 
Cheers,
Benno

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

[Boqun Feng]

On Fri, Oct 27, 2023 at 10:50:45PM +0000, Benno Lossin wrote:
[...]
> > 
> > Hmm... but does it mean even `set_speed()` has the similar issue?
> > 
> > 	let phydev: *mut phy_device = self.0.get();
> > 	unsafe { (*phydev).speed = ...; }
> 
> No that should be fine, take a look at the MIR output of the following 
> code [1]:
> 
>     struct Foo {
>         a: usize,
>         b: usize,
>     }
>     
>     fn foo(ptr: *mut Foo) {
>         unsafe { (*ptr).b = 0; }
>     }
>     
>     fn bar(ptr: *mut Foo) {
>         unsafe { (&mut *ptr).b = 0; }
>     }
> 
> Aside from some alignment checking, foo's MIR looks like this:
> 
>     bb1: {
>         ((*_1).1: usize) = const 0_usize;
>         return;
>     }
> 
> And bar's MIR like this:
> 
>     bb1: {
>         _2 = &mut (*_1);
>         ((*_2).1: usize) = const 0_usize;
>         return;
>     }
> 
> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f7c4d87bf29a64af0acc09ff75d3716d
> 
> So I think that is fine, but maybe Gary has something else to say about it.
> 

Well when `-C opt-level=2`, they are the same:

	https://godbolt.org/z/hxxo75YYh

Regards,
Boqun

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

[Boqun Feng]

[...]
> > [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f7c4d87bf29a64af0acc09ff75d3716d
> > 
> > So I think that is fine, but maybe Gary has something else to say about it.
> > 
> 
> Well when `-C opt-level=2`, they are the same:
> 
> 	https://godbolt.org/z/hxxo75YYh

But maybe you are right, no temporary reference in this case.

Regards,
Boqun

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

[Benno Lossin]

On 28.10.23 01:26, Boqun Feng wrote:
> On Fri, Oct 27, 2023 at 10:50:45PM +0000, Benno Lossin wrote:
> [...]
>>>
>>> Hmm... but does it mean even `set_speed()` has the similar issue?
>>>
>>> 	let phydev: *mut phy_device = self.0.get();
>>> 	unsafe { (*phydev).speed = ...; }
>>
>> No that should be fine, take a look at the MIR output of the following
>> code [1]:
>>
>>      struct Foo {
>>          a: usize,
>>          b: usize,
>>      }
>>
>>      fn foo(ptr: *mut Foo) {
>>          unsafe { (*ptr).b = 0; }
>>      }
>>
>>      fn bar(ptr: *mut Foo) {
>>          unsafe { (&mut *ptr).b = 0; }
>>      }
>>
>> Aside from some alignment checking, foo's MIR looks like this:
>>
>>      bb1: {
>>          ((*_1).1: usize) = const 0_usize;
>>          return;
>>      }
>>
>> And bar's MIR like this:
>>
>>      bb1: {
>>          _2 = &mut (*_1);
>>          ((*_2).1: usize) = const 0_usize;
>>          return;
>>      }
>>
>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f7c4d87bf29a64af0acc09ff75d3716d
>>
>> So I think that is fine, but maybe Gary has something else to say about it.
>>
> 
> Well when `-C opt-level=2`, they are the same:
> 
> 	https://godbolt.org/z/hxxo75YYh

It doesn't matter what the optimizer does, `bar` is unsound in our use-case
and `foo` is fine (I think).

-- 
Cheers,
Benno

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

[Andrew Lunn]

> I did not notice this before, but this means we cannot use the `link`
> function from bindgen, since that takes `&self`. We would need a
> function that takes `*const Self` instead.

After the discussion about mutability, i took a look at the C code,
and started adding const to functions which take phydev, but don't
modify it. Does bindgen look for such const attributes? Does it make a
difference to be binding?

	Andrew

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

[Miguel Ojeda]

On Sat, Oct 28, 2023 at 12:40 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> After the discussion about mutability, i took a look at the C code,
> and started adding const to functions which take phydev, but don't
> modify it. Does bindgen look for such const attributes? Does it make a
> difference to be binding?

I think you are referring to the `const` type qualifier, not the
attribute (which the kernel uses too).

But yes, it makes a difference in the output it generates (if we are
talking about the pointed-to), e.g.

    void f(struct S *);       // *mut S
    void f(const struct S *); // *const S

Being const-correct would be a good change for C in any case.

Cheers,
Miguel

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

[Andrew Lunn]

> But yes, it makes a difference in the output it generates (if we are
> talking about the pointed-to), e.g.
> 
>     void f(struct S *);       // *mut S
>     void f(const struct S *); // *const S
> 
> Being const-correct would be a good change for C in any case.

I have a patchset which changes a few functions to use const struct
*phydev. I will post them next cycle. They are a bit invasive, so i
don't want to do it now, this close to the merge window.

      Andrew

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

[FUJITA Tomonori]

On Fri, 27 Oct 2023 21:19:38 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 10/27/23 21:59, Boqun Feng wrote:
>> On Thu, Oct 26, 2023 at 09:10:46AM +0900, FUJITA Tomonori wrote:
>> [...]
>>> +    /// Gets the current link state.
>>> +    ///
>>> +    /// It returns true if the link is up.
>>> +    pub fn is_link_up(&self) -> bool {
>>> +        const LINK_IS_UP: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>> 
>> Tomo, FWIW, the above line means *copying* the content pointed by
>> `self.0.get()` into `phydev`, i.e. `phydev` is the semantically a copy
>> of the `phy_device` instead of an alias. In C code, it means you did:
> 
> Good catch. `phy_device` is rather large (did not look at the exact
> size) and this will not be optimized on debug builds, so it could lead
> to stackoverflows.
> 
>> 	struct phy_device phydev = *ptr;
>> 
>> Sure, both compilers can figure this out, therefore no extra copy is
>> done, but still it's better to avoid this copy semantics by doing:
>> 
>> 	let phydev = unsafe { &*self.0.get() };
> 
> We need to be careful here, since doing this creates a reference
> `&bindings::phy_device` which asserts that it is immutable. That is not
> the case, since the C side might change it at any point (this is the
> reason we wrap things in `Opaque`, since that allows mutatation even
> through sharde references).

You meant that the C code might modify it independently anytime, not
the C code called the Rust abstractions might modify it, right?


> I did not notice this before, but this means we cannot use the `link`
> function from bindgen, since that takes `&self`. We would need a
> function that takes `*const Self` instead.

Implementing functions to access to a bitfield looks tricky so we need
to add such feature to bindgen or we add getters to the C side?

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

[Andrew Lunn]

> > We need to be careful here, since doing this creates a reference
> > `&bindings::phy_device` which asserts that it is immutable. That is not
> > the case, since the C side might change it at any point (this is the
> > reason we wrap things in `Opaque`, since that allows mutatation even
> > through sharde references).
> 
> You meant that the C code might modify it independently anytime, not
> the C code called the Rust abstractions might modify it, right?

The whole locking model is base around that not happening. Things
should only change with the lock held. I you make a call into the C
side, then yes, it can and will change it. So you should not cache a
value over a C call.
 
 	Andrew

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

[FUJITA Tomonori]

On Sat, 28 Oct 2023 16:53:30 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> > We need to be careful here, since doing this creates a reference
>> > `&bindings::phy_device` which asserts that it is immutable. That is not
>> > the case, since the C side might change it at any point (this is the
>> > reason we wrap things in `Opaque`, since that allows mutatation even
>> > through sharde references).
>> 
>> You meant that the C code might modify it independently anytime, not
>> the C code called the Rust abstractions might modify it, right?
> 
> The whole locking model is base around that not happening. Things
> should only change with the lock held. I you make a call into the C
> side, then yes, it can and will change it. So you should not cache a
> value over a C call.

Yeah, I understand that. But if I understand Benno correctly, from
Rust perspective, such might happen.

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

[Benno Lossin]

On 28.10.23 18:09, FUJITA Tomonori wrote:
> On Sat, 28 Oct 2023 16:53:30 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>>> We need to be careful here, since doing this creates a reference
>>>> `&bindings::phy_device` which asserts that it is immutable. That is not
>>>> the case, since the C side might change it at any point (this is the
>>>> reason we wrap things in `Opaque`, since that allows mutatation even
>>>> through sharde references).
>>>
>>> You meant that the C code might modify it independently anytime, not
>>> the C code called the Rust abstractions might modify it, right?
>>
>> The whole locking model is base around that not happening. Things
>> should only change with the lock held. I you make a call into the C
>> side, then yes, it can and will change it. So you should not cache a
>> value over a C call.
> 
> Yeah, I understand that. But if I understand Benno correctly, from
> Rust perspective, such might happen.

Yes, that is what I meant. Sure the C side might never modify the
value, but this is not good enough for Rust. It must somehow be ensured
that it never is modified, in order for us to rely on it.

-- 
Cheers,
Benno

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

[Boqun Feng]

On Sat, Oct 28, 2023 at 04:39:08PM +0000, Benno Lossin wrote:
> On 28.10.23 18:09, FUJITA Tomonori wrote:
> > On Sat, 28 Oct 2023 16:53:30 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> >>>> We need to be careful here, since doing this creates a reference
> >>>> `&bindings::phy_device` which asserts that it is immutable. That is not
> >>>> the case, since the C side might change it at any point (this is the
> >>>> reason we wrap things in `Opaque`, since that allows mutatation even
> >>>> through sharde references).
> >>>
> >>> You meant that the C code might modify it independently anytime, not
> >>> the C code called the Rust abstractions might modify it, right?
> >>
> >> The whole locking model is base around that not happening. Things
> >> should only change with the lock held. I you make a call into the C

Here is my understanding, I treat references in Rust as a special
pointer, so having a `&bindings::phy_device` means the *entire* object
is immutable unless the fields are interior mutable, for example,
behind a `UnsafeCell` or `Opaque`, examples of interior mutable types
are atomic and locks.

Now since C doesn't have the "interior mutable" concept or these types,
so when bindgen generates the `bindings::phy_device`, none of the fields
of a lock or atomic is marked as `UnsafeCell` or `Opaque`. That's why
`Opaque` is needed for defining `Device`:

	pub struct Device(Opaque<bindings::phy_device);

`Opaque` basically does two things:

1)	tell compilers that the underlying content may be modified (of
	course in some sort of serialization) when there exists a
	`&Device` or `&mut Device`.

2)	only provide `*mut bindings::phy_device`, so accessing the
	underlying object has to use unsafe.

Now let's look back into struct phy_device, it does have a few locks
in it, and at least even with phydev->lock held, the content of
phydev->lock itself can be changed (e.g tick locks), hence it breaks the
requirement of the existence of a `&bindings::phy_device`.

Of course, if we can define our own `bindings::phy_device` or ask
bindgen to automatically add `Opaque` to certain types, then
`&bindings::phy_device` is still possible to use.

If we are OK to not use `&bindings::phy_device` then Benno's proposal in
bindgen is one way to work with this.

Regards,
Boqun

> >> side, then yes, it can and will change it. So you should not cache a
> >> value over a C call.
> > 
> > Yeah, I understand that. But if I understand Benno correctly, from
> > Rust perspective, such might happen.
> 
> Yes, that is what I meant. Sure the C side might never modify the
> value, but this is not good enough for Rust. It must somehow be ensured
> that it never is modified, in order for us to rely on it.
> 
> -- 
> Cheers,
> Benno
> 
> 

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

[Andrew Lunn]

> Now let's look back into struct phy_device, it does have a few locks
> in it, and at least even with phydev->lock held, the content of
> phydev->lock itself can be changed (e.g tick locks), hence it breaks the
> requirement of the existence of a `&bindings::phy_device`.

tick locks appear to be a Rust thing, so are unlikely to appear in a C
structure. However, kernel C mutex does have a linked list of other
threads waiting for the mutex. So phydev->lock can change at any time,
even when held.

	Andrew

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

[Boqun Feng]

On Sat, Oct 28, 2023 at 09:23:25PM +0200, Andrew Lunn wrote:
> > Now let's look back into struct phy_device, it does have a few locks
> > in it, and at least even with phydev->lock held, the content of
> > phydev->lock itself can be changed (e.g tick locks), hence it breaks the
> > requirement of the existence of a `&bindings::phy_device`.
> 
> tick locks appear to be a Rust thing, so are unlikely to appear in a C

Ah, I meant ticket locks... same here a mutex has a wait_lock which can
be implemented by ticket locks or queued spin locks, so the u32 lock
field may change even with lock held.

Regards,
Boqun

> structure. However, kernel C mutex does have a linked list of other
> threads waiting for the mutex. So phydev->lock can change at any time,
> even when held.
> 
> 	Andrew

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

[Benno Lossin]

On 28.10.23 11:27, FUJITA Tomonori wrote:
> On Fri, 27 Oct 2023 21:19:38 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> I did not notice this before, but this means we cannot use the `link`
>> function from bindgen, since that takes `&self`. We would need a
>> function that takes `*const Self` instead.
> 
> Implementing functions to access to a bitfield looks tricky so we need
> to add such feature to bindgen or we add getters to the C side?

Indeed, I just opened an issue [1] on the bindgen repo.

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

-- 
Cheers,
Benno

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

[Andrew Lunn]

On Sat, Oct 28, 2023 at 04:37:53PM +0000, Benno Lossin wrote:
> On 28.10.23 11:27, FUJITA Tomonori wrote:
> > On Fri, 27 Oct 2023 21:19:38 +0000
> > Benno Lossin <benno.lossin@proton.me> wrote:
> >> I did not notice this before, but this means we cannot use the `link`
> >> function from bindgen, since that takes `&self`. We would need a
> >> function that takes `*const Self` instead.
> > 
> > Implementing functions to access to a bitfield looks tricky so we need
> > to add such feature to bindgen or we add getters to the C side?
> 
> Indeed, I just opened an issue [1] on the bindgen repo.
> 
> [1]: https://github.com/rust-lang/rust-bindgen/issues/2674

Please could you help me understand the consequences here. Are you
saying the rust toolchain is fatally broken here, it cannot generate
valid code at the moment? As a result we need to wait for a new
version of bindgen?

	Andrew

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

[Benno Lossin]

On 28.10.23 20:23, Andrew Lunn wrote:
> On Sat, Oct 28, 2023 at 04:37:53PM +0000, Benno Lossin wrote:
>> On 28.10.23 11:27, FUJITA Tomonori wrote:
>>> On Fri, 27 Oct 2023 21:19:38 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>> I did not notice this before, but this means we cannot use the `link`
>>>> function from bindgen, since that takes `&self`. We would need a
>>>> function that takes `*const Self` instead.
>>>
>>> Implementing functions to access to a bitfield looks tricky so we need
>>> to add such feature to bindgen or we add getters to the C side?
>>
>> Indeed, I just opened an issue [1] on the bindgen repo.
>>
>> [1]: https://github.com/rust-lang/rust-bindgen/issues/2674
> 
> Please could you help me understand the consequences here. Are you
> saying the rust toolchain is fatally broken here, it cannot generate
> valid code at the moment? As a result we need to wait for a new
> version of bindgen?
This only affects bitfields, since they require special accessor functions
generated by bindgen, so I would not say that the toolchain is fatally broken.
It also is theoretically possible to manually access the bitfields in a correct
manner, but that is error prone (which is why we use the accessor functions
provided by bindgen).

In this particular case we have three options:
1. wait until bindgen provides a raw accessor function that allows to use
    only raw pointers.
2. create some C helper functions for the bitfield access that will be replaced
    by the bindgen functions once bindgen has updated.
3. Since for the `phy_device` bindings, we only ever call functions while holding
    the `phy_device.lock` lock (at least I think that this is correct) we might be
    able to get away with creating a reference to the object and use the current
    accessor functions anyway.

But for point 3 I will have to consult the others.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Sat, 28 Oct 2023 18:45:40 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 28.10.23 20:23, Andrew Lunn wrote:
>> On Sat, Oct 28, 2023 at 04:37:53PM +0000, Benno Lossin wrote:
>>> On 28.10.23 11:27, FUJITA Tomonori wrote:
>>>> On Fri, 27 Oct 2023 21:19:38 +0000
>>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>> I did not notice this before, but this means we cannot use the `link`
>>>>> function from bindgen, since that takes `&self`. We would need a
>>>>> function that takes `*const Self` instead.
>>>>
>>>> Implementing functions to access to a bitfield looks tricky so we need
>>>> to add such feature to bindgen or we add getters to the C side?
>>>
>>> Indeed, I just opened an issue [1] on the bindgen repo.
>>>
>>> [1]: https://github.com/rust-lang/rust-bindgen/issues/2674
>> 
>> Please could you help me understand the consequences here. Are you
>> saying the rust toolchain is fatally broken here, it cannot generate
>> valid code at the moment? As a result we need to wait for a new
>> version of bindgen?
> This only affects bitfields, since they require special accessor functions
> generated by bindgen, so I would not say that the toolchain is fatally broken.
> It also is theoretically possible to manually access the bitfields in a correct
> manner, but that is error prone (which is why we use the accessor functions
> provided by bindgen).
> 
> In this particular case we have three options:
> 1. wait until bindgen provides a raw accessor function that allows to use
>     only raw pointers.
> 2. create some C helper functions for the bitfield access that will be replaced
>     by the bindgen functions once bindgen has updated.
> 3. Since for the `phy_device` bindings, we only ever call functions while holding
>     the `phy_device.lock` lock (at least I think that this is correct) we might be
>     able to get away with creating a reference to the object and use the current
>     accessor functions anyway.
> 
> But for point 3 I will have to consult the others.

The current code is fine from Rust perspective because the current
code copies phy_driver on stack and makes a reference to the copy, if
I undertand correctly.

It's not nice to create an 500-bytes object on stack. It turned out
that it's not so simple to avoid it.

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

[Boqun Feng]

On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
[...]
> 
> The current code is fine from Rust perspective because the current
> code copies phy_driver on stack and makes a reference to the copy, if
> I undertand correctly.
> 

I had the same thought Benno brought the issue on `&`, but unfortunately
it's not true ;-) In the following code:

	let phydev = unsafe { *self.0.get() };

, semantically the *whole* `bindings::phy_device` is being read, so if
there is any modification (i.e. write) that may happen in the meanwhile,
it's data race, and data races are UB (even in C).

So both implementations have the problem because of the same cause.

> It's not nice to create an 500-bytes object on stack. It turned out
> that it's not so simple to avoid it.

As you can see, copying is not the way to work around this.

Regards,
Boqun

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

[Boqun Feng]

On Sun, Oct 29, 2023 at 09:48:41AM -0700, Boqun Feng wrote:
> On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> [...]
> > 
> > The current code is fine from Rust perspective because the current
> > code copies phy_driver on stack and makes a reference to the copy, if
> > I undertand correctly.
> > 
> 
> I had the same thought Benno brought the issue on `&`, but unfortunately
> it's not true ;-) In the following code:
> 
> 	let phydev = unsafe { *self.0.get() };
> 
> , semantically the *whole* `bindings::phy_device` is being read, so if
> there is any modification (i.e. write) that may happen in the meanwhile,
> it's data race, and data races are UB (even in C).
> 
> So both implementations have the problem because of the same cause.
> 
> > It's not nice to create an 500-bytes object on stack. It turned out
> > that it's not so simple to avoid it.
> 
> As you can see, copying is not the way to work around this.
> 

An temporary solution is doing the #2 option from Benno, but in Rust and
open code it, like the following:

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 145d0407fe31..f5230ac48014 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -121,10 +121,10 @@ pub fn state(&self) -> DeviceState {
     ///
     /// It returns true if the link is up.
     pub fn is_link_up(&self) -> bool {
-        const LINK_IS_UP: u32 = 1;
+        const LINK_IS_UP: u64 = 1;
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.link() == LINK_IS_UP
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(14, 1) == LINK_IS_UP
     }
 
     /// Gets the current auto-negotiation configuration.
@@ -132,18 +132,18 @@ pub fn is_link_up(&self) -> bool {
     /// It returns true if auto-negotiation is enabled.
     pub fn is_autoneg_enabled(&self) -> bool {
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.autoneg() == bindings::AUTONEG_ENABLE
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
     pub fn is_autoneg_completed(&self) -> bool {
-        const AUTONEG_COMPLETED: u32 = 1;
+        const AUTONEG_COMPLETED: u64 = 1;
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.autoneg_complete() == AUTONEG_COMPLETED
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(15, 1) == AUTONEG_COMPLETED
     }
 
     /// Sets the speed of the PHY.


Of course, it's not maintainable in longer term since it relies on
hard-coding the bit offset of these bit fields. But I think it's best we
can do from Linux kernel side. It's up to Andrew and Miguel whether this
temporary solution is OK.

Regards,
Boqun

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

[Boqun Feng]

On Sun, Oct 29, 2023 at 11:09:17AM -0700, Boqun Feng wrote:
[...]
> Of course, it's not maintainable in longer term since it relies on
> hard-coding the bit offset of these bit fields. But I think it's best we
> can do from Linux kernel side.

Hmm.. I guess I should have added "other than creating EXPORTed C
accessors for these bit fields".

Regards,
Boqun

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

[Andrew Lunn]

> Of course, it's not maintainable in longer term since it relies on
> hard-coding the bit offset of these bit fields. But I think it's best we
> can do from Linux kernel side. It's up to Andrew and Miguel whether this
> temporary solution is OK.

What is the likely time scale for getting a new version of bindgen
with the necessary bit field support?

We have probably missed the merge window for v6.7. So there is around
9 weeks to the next merge window. If a working bindgen is likely to be
available by then, we should not bother with a workaround, just wait.

If you think bindgen is going to take longer than that, then we should
consider workarounds. But we have no rush, its still 9 weeks before we
need working code.

Zooming out a bit. Is there any other in mainline Rust code, or about
to be submitted for this merge window code, using bit fields? I assume
that is equally broken?

	Andrew

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

[Miguel Ojeda]

On Sun, Oct 29, 2023 at 8:39 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> We have probably missed the merge window for v6.7. So there is around

Definitely missed. We aim to send our PRs in advance of the merge
window, e.g. this cycle it has already been sent.

But even if it wasn't, for `rust-next`, feature patches should be sent
early in the cycle anyway.

> 9 weeks to the next merge window. If a working bindgen is likely to be
> available by then, we should not bother with a workaround, just wait.

In general, it is better to avoid workarounds if something is going to
be implemented properly. However, `bindgen` is a third-party project
which we do not control.

In the case of the exhaustiveness check, they looked interested in the
feature, which is why I wanted to avoid the workaround if possible.

In this instance though, we don't even know yet what they think about
the feature we requested. We will give them some time to see what they
think, and then decide based on that.

Cheers,
Miguel

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

[Andrew Lunn]

On Mon, Oct 30, 2023 at 01:07:07PM +0100, Miguel Ojeda wrote:
> On Sun, Oct 29, 2023 at 8:39 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > We have probably missed the merge window for v6.7. So there is around
> 
> Definitely missed. We aim to send our PRs in advance of the merge
> window, e.g. this cycle it has already been sent.

Netdev works a bit different. Patches can be merged into net-next even
a couple of days into the merge window. A lot depends on the riskiness
of a patch. A new driver has very low risk, since it is generally self
contained. Even if its broken, there is another 8 weeks to fix it up.

However, for this cycle, the PR for netdev has already been sent,
because a lot of the Maintainers are travelling to the netdev
conference.

	Andrew

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

[FUJITA Tomonori]

On Sun, 29 Oct 2023 09:48:41 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> [...]
>> 
>> The current code is fine from Rust perspective because the current
>> code copies phy_driver on stack and makes a reference to the copy, if
>> I undertand correctly.
>> 
> 
> I had the same thought Benno brought the issue on `&`, but unfortunately
> it's not true ;-) In the following code:
> 
> 	let phydev = unsafe { *self.0.get() };
> 
> , semantically the *whole* `bindings::phy_device` is being read, so if
> there is any modification (i.e. write) that may happen in the meanwhile,
> it's data race, and data races are UB (even in C).

Benno said so? I'm not sure about the logic (whole v.s. partial). Even
if you read partially, the part might be modified by the C side during
reading.

For me, the issue is that creating &T for an object that might be
modified.

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

[Boqun Feng]

On Mon, Oct 30, 2023 at 07:58:52AM +0900, FUJITA Tomonori wrote:
> On Sun, 29 Oct 2023 09:48:41 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> > [...]
> >> 
> >> The current code is fine from Rust perspective because the current
> >> code copies phy_driver on stack and makes a reference to the copy, if
> >> I undertand correctly.
> >> 
> > 
> > I had the same thought Benno brought the issue on `&`, but unfortunately
> > it's not true ;-) In the following code:
> > 
> > 	let phydev = unsafe { *self.0.get() };
> > 
> > , semantically the *whole* `bindings::phy_device` is being read, so if
> > there is any modification (i.e. write) that may happen in the meanwhile,
> > it's data race, and data races are UB (even in C).
> 
> Benno said so? I'm not sure about the logic (whole v.s. partial). Even

We can wait for Benno's response, but there is an example where Miri
says it's data race:

	https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=c7097644aa5f02a0a436e5b8b8624824

> if you read partially, the part might be modified by the C side during
> reading.

If you read the part protected by phy_device->lock, C side shouldn't
modify it, but the case here is not all fields in phy_device stay
unchanged when phy_device->lock (and Rust side doesn't mark them
interior mutable), see the discussion drom Andrew and me.

> 
> For me, the issue is that creating &T for an object that might be
> modified.

The reason a `&phy_device` cannot be created here is because concurrent
writes may cause a invalid phy_device (i.e. data race), the same applies
to a copy.

Regards,
Boqun

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

[Benno Lossin]

On 30.10.23 01:19, Boqun Feng wrote:
> On Mon, Oct 30, 2023 at 07:58:52AM +0900, FUJITA Tomonori wrote:
>> if you read partially, the part might be modified by the C side during
>> reading.
> 
> If you read the part protected by phy_device->lock, C side shouldn't
> modify it, but the case here is not all fields in phy_device stay
> unchanged when phy_device->lock (and Rust side doesn't mark them
> interior mutable), see the discussion drom Andrew and me.
> 
>>
>> For me, the issue is that creating &T for an object that might be
>> modified.
> 
> The reason a `&phy_device` cannot be created here is because concurrent
> writes may cause a invalid phy_device (i.e. data race), the same applies
> to a copy.

Both comments by Boqun above are correct. Additionally even if the write
would not have a data race with the read, it would still be UB. (For example
when the write is by the same thread)

If you just read the field itself then it should be fine, since it is
protected by a lock, see Boqun's patch for manually accessing the bitfields.

But I would wait until we see a response from the bindgen devs on the issue.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Mon, 30 Oct 2023 08:34:46 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 30.10.23 01:19, Boqun Feng wrote:
>> On Mon, Oct 30, 2023 at 07:58:52AM +0900, FUJITA Tomonori wrote:
>>> if you read partially, the part might be modified by the C side during
>>> reading.
>> 
>> If you read the part protected by phy_device->lock, C side shouldn't
>> modify it, but the case here is not all fields in phy_device stay
>> unchanged when phy_device->lock (and Rust side doesn't mark them
>> interior mutable), see the discussion drom Andrew and me.
>> 
>>>
>>> For me, the issue is that creating &T for an object that might be
>>> modified.
>> 
>> The reason a `&phy_device` cannot be created here is because concurrent
>> writes may cause a invalid phy_device (i.e. data race), the same applies
>> to a copy.
> 
> Both comments by Boqun above are correct. Additionally even if the write
> would not have a data race with the read, it would still be UB. (For example
> when the write is by the same thread)
> 
> If you just read the field itself then it should be fine, since it is
> protected by a lock, see Boqun's patch for manually accessing the bitfields.

The rust code can access to only fields in phy_device that the C side
doesn't modify; these fields are protected by a lock or in other ways
(resume/suspend cases).

Right?

> But I would wait until we see a response from the bindgen devs on the issue.

You meant that they might have a different option on this?

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

[Benno Lossin]

On 30.10.23 13:49, FUJITA Tomonori wrote:
> On Mon, 30 Oct 2023 08:34:46 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> Both comments by Boqun above are correct. Additionally even if the write
>> would not have a data race with the read, it would still be UB. (For example
>> when the write is by the same thread)
>>
>> If you just read the field itself then it should be fine, since it is
>> protected by a lock, see Boqun's patch for manually accessing the bitfields.
> 
> The rust code can access to only fields in phy_device that the C side
> doesn't modify; these fields are protected by a lock or in other ways
> (resume/suspend cases).

No it could access all fields in `phy_device`, which means it could also
access `phy_device.lock`. Since that is a mutex, it might change at any
time even if we hold the lock.

>> But I would wait until we see a response from the bindgen devs on the issue.
> 
> You meant that they might have a different option on this?

No, before you implement the workaround that Boqun posted you
should wait until the bindgen devs say how long/if they will
implement it.

-- 
Cheers,
Benno

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

[FUJITA Tomonori]

On Mon, 30 Oct 2023 16:45:38 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>>> But I would wait until we see a response from the bindgen devs on the issue.
>> 
>> You meant that they might have a different option on this?
> 
> No, before you implement the workaround that Boqun posted you
> should wait until the bindgen devs say how long/if they will
> implement it.

It has been 10 days but no response from bindgen developpers. I guess
that unlikely bindgen will support the feature until the next merge
window.

I prefer adding accessors in the C side rather than the workaround if
it's fine by Andrew because we have no idea when bindgen will support
the feature.

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

[Andrew Lunn]

On Wed, Nov 08, 2023 at 07:46:47PM +0900, FUJITA Tomonori wrote:
> On Mon, 30 Oct 2023 16:45:38 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
> >>> But I would wait until we see a response from the bindgen devs on the issue.
> >> 
> >> You meant that they might have a different option on this?
> > 
> > No, before you implement the workaround that Boqun posted you
> > should wait until the bindgen devs say how long/if they will
> > implement it.
> 
> It has been 10 days but no response from bindgen developpers. I guess
> that unlikely bindgen will support the feature until the next merge
> window.

I just took a look around the kernel includes. Here are a few examples
i found, there are many many more.

include/target/iscsi/iscsi_target_core.h:       unsigned int            conn_tx_reset_cpumask:1;
include/media/videobuf2-core.h: unsigned int            synced:1;
include/media/v4l2-ctrls.h:     unsigned int done:1;
include/drm/bridge/samsung-dsim.h:      unsigned int has_freqband:1;
include/net/sock_reuseport.h:   unsigned int            bind_inany:1;
include/net/sock.h:     unsigned char           skc_ipv6only:1;
include/sound/simple_card_utils.h:      unsigned int force_dpcm:1;
include/sound/soc.h:    unsigned int autodisable:1;
include/linux/pci.h:    unsigned int    imm_ready:1;    /* Supports Immediate Readiness */
include/linux/regmap.h: unsigned int use_ack:1;
include/linux/cpuidle.h:        unsigned int            registered:1;
include/linux/regulator/gpio-regulator.h:       unsigned enabled_at_boot:1;
include/linux/phy.h:    unsigned link:1;
include/linux/pm.h:     unsigned int            irq_safe:1;
include/linux/nfs_xdr.h:        unsigned char                   renew:1;
include/linux/iio/iio.h:        unsigned                output:1;
include/linux/drbd.h:           unsigned user_isp:1 ;
include/linux/sched.h:  unsigned                        sched_migrated:1;
include/linux/writeback.h:      unsigned no_cgroup_owner:1;
include/linux/tty_port.h:       unsigned char           console:1;
include/linux/mpi.h:                    unsigned int two_inv_p:1;
include/linux/mmc/host.h:       unsigned int            use_spi_crc:1;
include/linux/netdevice.h:      unsigned                wol_enabled:1;
include/linux/serial_8250.h:    unsigned int            tx_stopped:1;
include/linux/usb.h:    unsigned can_submit:1;
include/linux/firewire.h:       unsigned is_local:1;
include/linux/phylink.h:        unsigned int link:1;
include/linux/gpio_keys.h:      unsigned int rep:1;
include/linux/spi/spi.h:        unsigned        cs_change:1;
include/linux/i2c-mux.h:        unsigned int arbitrator:1;
include/linux/kobject.h:        unsigned int state_in_sysfs:1;
include/linux/kbd_kern.h:       unsigned char ledmode:1;
include/linux/bpf_verifier.h:   unsigned int fit_for_inline:1;
include/linux/rtc.h:    unsigned int uie_irq_active:1;
include/linux/usb/usbnet.h:     unsigned                can_dma_sg:1;
include/linux/usb/serial.h:     unsigned char                   attached:1;
include/linux/usb/gadget.h:     unsigned dir_out:1;
include/linux/comedi/comedidev.h:       unsigned int attached:1;
include/scsi/sg.h:    unsigned int twelve_byte:1;
include/scsi/scsi_host.h:       unsigned emulated:1;
include/scsi/scsi_device.h:     unsigned removable:1;
include/rdma/ib_verbs.h:        unsigned int            ip_gids:1;

And thats just bitfields used as binary values. There are more with
:2, :3, :4, :8, :16.

The point i'm trying to make is that they are used in many
subsystems. Not having bindgen support for them seems like its going
to be a problem.

Isn't this also an unsoundness problem? Is there existing Rust code
which people think is sound but is actually not?

> I prefer adding accessors in the C side rather than the workaround if
> it's fine by Andrew because we have no idea when bindgen will support
> the feature.

Maybe we need a better understanding of the kernel wide implications
of this. It could be this is actually a big issue, and rust-for-linux
can then apply either pressure, or human resources, to get it
implemented.

    Andrew

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

[Andrew Lunn]

> The current code is fine from Rust perspective because the current
> code copies phy_driver on stack and makes a reference to the copy, if
> I undertand correctly.
> 
> It's not nice to create an 500-bytes object on stack. It turned out
> that it's not so simple to avoid it.

Does it also copy the stack version over the 'real' version before
exiting? If not, it is broken, since modifying state in phy_device is
often why the driver is called. But copying the stack version is also
broken, since another thread taking the phydev->lock is going to get
lost from the linked list of waiters.

Taking a copy of the C structure does seem very odd, to me as a C
programmer.

     Andrew

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

[Benno Lossin]

On 29.10.23 18:32, Andrew Lunn wrote:
>> The current code is fine from Rust perspective because the current
>> code copies phy_driver on stack and makes a reference to the copy, if
>> I undertand correctly.
>>
>> It's not nice to create an 500-bytes object on stack. It turned out
>> that it's not so simple to avoid it.
> 
> Does it also copy the stack version over the 'real' version before
> exiting? If not, it is broken, since modifying state in phy_device is
> often why the driver is called. But copying the stack version is also
> broken, since another thread taking the phydev->lock is going to get
> lost from the linked list of waiters.

It does not copy the stack version over the original. Since we only read
the `link` field in the discussed function and we hold the lock, it should
not get modified, right? So from a functional viewpoint there is no issue.
(Aside from increased stack size and the data race issue etc.)

> Taking a copy of the C structure does seem very odd, to me as a C
> programmer.

It is also odd in Rust.

-- 
Cheers,
Benno

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

[Miguel Ojeda]

On Sat, Oct 28, 2023 at 8:24 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Please could you help me understand the consequences here. Are you
> saying the rust toolchain is fatally broken here, it cannot generate
> valid code at the moment? As a result we need to wait for a new
> version of bindgen?

Benno has already replied, but to be extra clear: no, it is not
"fatally broken".

`bindgen` is just a tool to automate writing some things you would
otherwise need to manually write. It currently generates some methods
that take a reference, but we should avoid creating references in this
case, so we would like methods that take a pointer instead. That's it.

In other words, we could simply write the methods ourselves. That
would be "Option 0" (which would be like Benno's 1, but manually; or
like Benno's 2, but in Rust).

Cheers,
Miguel

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

[Alice Ryhl]

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

I promised Andrew to take a look at these patches at Plumbers. This
email contains the first part of my review.

In this email, I will bring up the question of how the safety comments
should be worded. I know that you've probably discussed this before, but
my opinion was asked for, and this is the main area where I think
there's room for improvement.

> +    /// # Safety
> +    ///
> +    /// This function must only be called from the callbacks in `phy_driver`.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {

This kind of safety comment where you say "must only be used by internal
code and nothing else" isn't great. It doesn't really help with checking
the correctness. It's usually better to document what is actually
required here, even if it shouldn't be called by non-internal code. I
recommend something along the lines of:

	# Safety
	
	For the duration of 'a, the pointer must point at a valid `phy_device`,
	and the caller must hold the X mutex.

Then in methods like this: (which are missing justification for why
there's no data race!)
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).phy_id }
you instead say:

	SAFETY: By the struct invariants, `phydev` points at a valid
	`phy_device`, and we hold the X lock, which gives us access to
	the `phy_id` field.

And you would also update the struct invariant accordingly:

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);





> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
> +// to the instance.

I used "X mutex" as an example for the synchronization mechanism in the
above snippets, but it sounds like its more complicated than that? Here
are some possible alternatives I could come up with:

Maybe we don't need synchronization when some operations can't happen?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held, or that there are no concurrent operations of type Y.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Maybe we have a separate case for when the device is being initialized
and nobody else has access yet?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held, or that the reference has exclusive access to the
/// entire `phy_device`.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Maybe it is easier to just list the fields we need access to?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts exclusive
/// access to the following fields: phy_id, state, speed, duplex. And
/// read access to the following fields: link, autoneg_complete,
/// autoneg.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Perhaps we want to avoid duplication with some existing C documentation?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the user
/// is inside a Y scope as defined in Documentation/foo/bar.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

But I don't know how these things are actually synchronized. Maybe
it is some sixth option. I would be happy to help draft these safety
comments once the actual synchronization mechanism is clear to me.

Or maybe you prefer to not do it this way, or to punt it for a later
patch series. I prefer to document these things in the above way, but
ultimately it is not up to me.

Alice

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

[Andrew Lunn]

> And you would also update the struct invariant accordingly:
> 
> /// # Invariants
> ///
> /// Referencing a `phy_device` using this struct asserts that the X
> /// mutex is held.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);
> 
> 
> 
> 
> 
> > +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> > +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> > +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
> > +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
> > +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
> > +// to the instance.
> 
> I used "X mutex" as an example for the synchronization mechanism in the
> above snippets, but it sounds like its more complicated than that? Here
> are some possible alternatives I could come up with:

So X would be phy_device->lock.

Suspend and resume don't have this lock held. I don't actually
remember the details, but there is an email discussion with Russell
King which does explain the problem we had, and why we think it is
safe to not hold the lock.

> /// # Invariants
> ///
> /// Referencing a `phy_device` using this struct asserts that the X
> /// mutex is held, or that the reference has exclusive access to the
> /// entire `phy_device`.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);

You can never have exclusive access to the entire phy_device, because
it contains a mutex. Other threads can block on that mutex, which
involves changing the linked list in the mutex.

But that is also a pretty common pattern, put the mutex inside the
structure it protects. So when you say 'exclusive access to the entire
`phy_device`' you actually mean excluding mutex, spinlocks, atomic
variables, etc?

> /// Referencing a `phy_device` using this struct asserts exclusive
> /// access to the following fields: phy_id, state, speed, duplex. And
> /// read access to the following fields: link, autoneg_complete,
> /// autoneg.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);

For the Rust code, you can maybe do this. But the Rust code calls into
C helpers to get the real work done, and they expect to have access to
pretty much everything in phy_device.

> /// # Invariants
> ///
> /// Referencing a `phy_device` using this struct asserts that the user
> /// is inside a Y scope as defined in Documentation/foo/bar.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);

There is no such documentation that i know of, except it does get
repeated again and again on the mailling lists. Its tribal knowledge.

> But I don't know how these things are actually synchronized. Maybe
> it is some sixth option. I would be happy to help draft these safety
> comments once the actual synchronization mechanism is clear to me.

The model is: synchronisation is not the drivers problem.

With a few years of experience reviewing drivers, you notice that
there are a number of driver writers who have no idea about
locking. They don't even think about it. So where possible, its best
to hide all the locking from them in the core. The core is in theory
developed by developers who do mostly understand locking and have a
better chance of getting it right. Its also exercised a lot more,
since its shared by all drivers.

My experience with this one Rust driver so far is that Rust developers
have problems accepting that its not the drivers problem.

	Andrew

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

[Alice Ryhl]

Andrew Lunn <andrew@lunn.ch> writes:
>> I used "X mutex" as an example for the synchronization mechanism in the
>> above snippets, but it sounds like its more complicated than that? Here
>> are some possible alternatives I could come up with:
> 
> So X would be phy_device->lock.
> 
> Suspend and resume don't have this lock held. I don't actually
> remember the details, but there is an email discussion with Russell
> King which does explain the problem we had, and why we think it is
> safe to not hold the lock.

So, what I would prefer to see as the struct invariant would be:

 * Either phy_device->lock is held,
 * or, we are in whatever situation you are in when you call suspend and
   resume.

The five suggestions I gave were my guesses as to what that situation
might be.

>> /// # Invariants
>> ///
>> /// Referencing a `phy_device` using this struct asserts that the X
>> /// mutex is held, or that the reference has exclusive access to the
>> /// entire `phy_device`.
>> #[repr(transparent)]
>> pub struct Device(Opaque<bindings::phy_device>);
> 
> You can never have exclusive access to the entire phy_device, because
> it contains a mutex. Other threads can block on that mutex, which
> involves changing the linked list in the mutex.
> 
> But that is also a pretty common pattern, put the mutex inside the
> structure it protects. So when you say 'exclusive access to the entire
> `phy_device`' you actually mean excluding mutex, spinlocks, atomic
> variables, etc?

No, I really meant exclusive access to everything. This suggestion is
where I guessed that the situation might be "we just created the
phy_device, and haven't yet shared it with anyone, so it's okay to
access it without the lock". But it sounds like that's not the case.

>> /// Referencing a `phy_device` using this struct asserts exclusive
>> /// access to the following fields: phy_id, state, speed, duplex. And
>> /// read access to the following fields: link, autoneg_complete,
>> /// autoneg.
>> #[repr(transparent)]
>> pub struct Device(Opaque<bindings::phy_device>);
> 
> For the Rust code, you can maybe do this. But the Rust code calls into
> C helpers to get the real work done, and they expect to have access to
> pretty much everything in phy_device.

Yeah, agreed, this is probably the suggestion I disliked the most.

>> /// # Invariants
>> ///
>> /// Referencing a `phy_device` using this struct asserts that the user
>> /// is inside a Y scope as defined in Documentation/foo/bar.
>> #[repr(transparent)]
>> pub struct Device(Opaque<bindings::phy_device>);
> 
> There is no such documentation that i know of, except it does get
> repeated again and again on the mailling lists. Its tribal knowledge.

Then, my suggestion would be to write down that tribal knowledge in the
safety comments.

>> But I don't know how these things are actually synchronized. Maybe
>> it is some sixth option. I would be happy to help draft these safety
>> comments once the actual synchronization mechanism is clear to me.
> 
> The model is: synchronisation is not the drivers problem.
> 
> With a few years of experience reviewing drivers, you notice that
> there are a number of driver writers who have no idea about
> locking. They don't even think about it. So where possible, its best
> to hide all the locking from them in the core. The core is in theory
> developed by developers who do mostly understand locking and have a
> better chance of getting it right. Its also exercised a lot more,
> since its shared by all drivers.
> 
> My experience with this one Rust driver so far is that Rust developers
> have problems accepting that its not the drivers problem.

I agree that locking should not be the driver's problem. If there's any
comment about locking in patch 5 of this patchset, then something has
gone wrong.

However, I don't see this file as part of the driver. It's a wrapper
around the core, which makes it part of the core. Like the C core, the
Rust abstractions will be shared by all Rust drivers. The correctness of
the unsafe code here is what ensures that drivers are not able to mess
up the locking in safe code.


Anyway. If you don't want to write down the tribal knowledge here, then
I suggest this simpler wording:

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that you are in
/// a context where all methods defined on this struct are safe to call.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

This invariant is much less precise, but at least it is correct.

Other safety comments may then be:

	/// Gets the id of the PHY.
	pub fn phy_id(&self) -> u32 {
	    let phydev = self.0.get();
	    // SAFETY: The struct invariant ensures that we may access
	    // this field without additional synchronization.
	    unsafe { (*phydev).phy_id }
	}

And:

	unsafe extern "C" fn soft_reset_callback(
	    phydev: *mut bindings::phy_device,
	) -> core::ffi::c_int {
	    from_result(|| {
	        // SAFETY: This callback is called only in contexts
		// where we hold `phy_device->lock`, so the accessors on
		// `Device` are okay to call.
	        let dev = unsafe { Device::from_raw(phydev) };
	        T::soft_reset(dev)?;
	        Ok(0)
	    })
	}

And:

	unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
	    from_result(|| {
	        // SAFETY: The C core code ensures that the accessors on
		// `Device` are okay to call even though `phy_device->lock`
		// might not be held.
	        let dev = unsafe { Device::from_raw(phydev) };
	        T::resume(dev)?;
	        Ok(0)
	    })
	}

Alice

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

[Andrew Lunn]

> >> /// # Invariants
> >> ///
> >> /// Referencing a `phy_device` using this struct asserts that the X
> >> /// mutex is held, or that the reference has exclusive access to the
> >> /// entire `phy_device`.
> >> #[repr(transparent)]
> >> pub struct Device(Opaque<bindings::phy_device>);
> > 
> > You can never have exclusive access to the entire phy_device, because
> > it contains a mutex. Other threads can block on that mutex, which
> > involves changing the linked list in the mutex.
> > 
> > But that is also a pretty common pattern, put the mutex inside the
> > structure it protects. So when you say 'exclusive access to the entire
> > `phy_device`' you actually mean excluding mutex, spinlocks, atomic
> > variables, etc?
> 
> No, I really meant exclusive access to everything. This suggestion is
> where I guessed that the situation might be "we just created the
> phy_device, and haven't yet shared it with anyone, so it's okay to
> access it without the lock". But it sounds like that's not the case.

It is pretty unusual for a linux driver to actually create a
device. Some level of core code generally creates a basic device
structure and passes it to the probe function. The probe can then
setup members in the device, maybe allocate memory and assign it to
the device->priv member etc.

However, in the probe method, it should be safe to assume its not
globally visible yet, so you can be more relaxed about locking.

> >> /// # Invariants
> >> ///
> >> /// Referencing a `phy_device` using this struct asserts that the user
> >> /// is inside a Y scope as defined in Documentation/foo/bar.
> >> #[repr(transparent)]
> >> pub struct Device(Opaque<bindings::phy_device>);
> > 
> > There is no such documentation that i know of, except it does get
> > repeated again and again on the mailling lists. Its tribal knowledge.
> 
> Then, my suggestion would be to write down that tribal knowledge in the
> safety comments.

O.K, we can do that.

     Andrew

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

[Alice Ryhl]

On 11/17/23 17:28, Andrew Lunn wrote:>>>> /// # Invariants
>>>> ///
>>>> /// Referencing a `phy_device` using this struct asserts that the user
>>>> /// is inside a Y scope as defined in Documentation/foo/bar.
>>>> #[repr(transparent)]
>>>> pub struct Device(Opaque<bindings::phy_device>);
>>>
>>> There is no such documentation that i know of, except it does get
>>> repeated again and again on the mailling lists. Its tribal knowledge.
>>
>> Then, my suggestion would be to write down that tribal knowledge in the
>> safety comments.
> 
> O.K, we can do that.

Do you have a link to one of those email threads that you mentioned?

Alice

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

[FUJITA Tomonori]

On Fri, 17 Nov 2023 15:42:46 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Anyway. If you don't want to write down the tribal knowledge here, then
> I suggest this simpler wording:
> 
> /// # Invariants
> ///
> /// Referencing a `phy_device` using this struct asserts that you are in
> /// a context where all methods defined on this struct are safe to call.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);
> 
> This invariant is much less precise, but at least it is correct.
> 
> Other safety comments may then be:
> 
> 	/// Gets the id of the PHY.
> 	pub fn phy_id(&self) -> u32 {
> 	    let phydev = self.0.get();
> 	    // SAFETY: The struct invariant ensures that we may access
> 	    // this field without additional synchronization.
> 	    unsafe { (*phydev).phy_id }
> 	}
> 
> And:
> 
> 	unsafe extern "C" fn soft_reset_callback(
> 	    phydev: *mut bindings::phy_device,
> 	) -> core::ffi::c_int {
> 	    from_result(|| {
> 	        // SAFETY: This callback is called only in contexts
> 		// where we hold `phy_device->lock`, so the accessors on
> 		// `Device` are okay to call.
> 	        let dev = unsafe { Device::from_raw(phydev) };
> 	        T::soft_reset(dev)?;
> 	        Ok(0)
> 	    })
> 	}
> 
> And:
> 
> 	unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> 	    from_result(|| {
> 	        // SAFETY: The C core code ensures that the accessors on
> 		// `Device` are okay to call even though `phy_device->lock`
> 		// might not be held.
> 	        let dev = unsafe { Device::from_raw(phydev) };
> 	        T::resume(dev)?;
> 	        Ok(0)
> 	    })
> 	}

With these comments, What I should write on from_raw() function as a
safety comment?

/// # Safety
///
/// For the duration of 'a, the pointer must point at a valid
/// `phy_device`, and the caller must ensure that an user of this struct
/// in a context where all methods defined on this struct are safe to
/// call.
unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {

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

[Alice Ryhl]

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

In this reply, I go through my minor nits:

> +use crate::{
> +    prelude::{vtable, Pin},
> +};

Normally, if you're importing specific prelude items by name instead of
using prelude::*, then you would import them using their non-prelude
path.

> +#[derive(PartialEq)]
> +pub enum DeviceState {

If you add PartialEq and you can add Eq, then also add Eq.

> +/// An adapter for the registration of a PHY driver.
> +struct Adapter<T: Driver> {
> +    _p: PhantomData<T>,
> +}

You don't need this struct. The methods can be top-level functions.

But I know that others have used the same style of defining a useless
struct, so it's fine with me.

> +    /// Defines certain other features this PHY supports.
> +    /// It is a combination of the flags in the [`flags`] module.
> +    const FLAGS: u32 = 0;

You need an empty line between the two lines if you intend for them to
be separate lines in rendered documentation.

> +#[vtable]
> +pub trait Driver {
> +    /// Issues a PHY software reset.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
>      [...]
> +}

I believe that the guidance for what to put in optional vtable-trait
methods was changed in:

https://lore.kernel.org/all/20231026201855.1497680-1-benno.lossin@proton.me/

> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}

I would change this to "it's okay to call phy_drivers_unregister from a
different thread than the one in which phy_drivers_register was called".

> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}

Here, you can say "Registration has no &self methods, so immutable
references to it are useless".

> +    // macro use only
> +    #[doc(hidden)]
> +    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
> +        bindings::mdio_device_id {
> +            phy_id: self.id,
> +            phy_id_mask: self.mask.as_int(),
> +        }
> +    }

This is fine, but I probably would just expose it for everyone. It's not
like it hurts if non-macro code can call this method, even if it doesn't
have a reason to do so.

Alice

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

[Andrew Lunn]

> I would change this to "it's okay to call phy_drivers_unregister from a
> different thread than the one in which phy_drivers_register was called".

This got me thinking about 'threads'. For register and unregister, we
are talking abut the kernel modules module_init() and module_exit()
function. module_init() can be called before user space is even
started, but it could also be called by insmod. module_exit() could be
called by rmmod, but it could also be the kernel, after user space has
gone away on shutdown. We are always in a context which can block, but
i never really think of this being threads. You can start a kernel
thread, and have some data structure exclusively used by that kernel
thread, but that is pretty unusual.

So i would probably turn this commenting around. Only comment like
this in the special case that a kernel thread exists, and it is
expected to have exclusive access.

	 Andrew

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

[Greg KH]

On Fri, Nov 17, 2023 at 02:53:44PM +0100, Andrew Lunn wrote:
> > I would change this to "it's okay to call phy_drivers_unregister from a
> > different thread than the one in which phy_drivers_register was called".
> 
> This got me thinking about 'threads'. For register and unregister, we
> are talking abut the kernel modules module_init() and module_exit()
> function. module_init() can be called before user space is even
> started, but it could also be called by insmod. module_exit() could be
> called by rmmod, but it could also be the kernel, after user space has
> gone away on shutdown.

The kernel will not call module_exit() on shutdown.  Or has something
changed recently?

> We are always in a context which can block, but
> i never really think of this being threads. You can start a kernel
> thread, and have some data structure exclusively used by that kernel
> thread, but that is pretty unusual.
> 
> So i would probably turn this commenting around. Only comment like
> this in the special case that a kernel thread exists, and it is
> expected to have exclusive access.

With the driver model, you can be sure that your probe/release functions
for the bus will never be called at the same time, and module_init/exit
can never be called at the same time, so perhaps this isn't an issue?

thanks,

greg k-h

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

[Boqun Feng]

On Fri, Nov 17, 2023 at 02:50:45PM -0500, Greg KH wrote:
> On Fri, Nov 17, 2023 at 02:53:44PM +0100, Andrew Lunn wrote:
> > > I would change this to "it's okay to call phy_drivers_unregister from a
> > > different thread than the one in which phy_drivers_register was called".
> > 
> > This got me thinking about 'threads'. For register and unregister, we

Just to make things clear for discussion, the "thread" here (when we are
talking about trait `Send` and `Sync`) means "contexts" (thread/process
contexts, irq contexts, etc).

When we say a type is `Send`, it means the object of that type can be
created in one context, passed to another context (could be the same
type of context, e.g. sending between two thread/process contexts) and
dropped there. For example, if you have a work_struct embedded type, you
can create it in the driver code and pass it to workqueue, and when the
work is done, you can free it in the workqueue context.

One example of not `Send` type (or `!Send`) is spinlock guard:

	let guard: Guard<..> = some_lock.lock();

creating a Guard means "spin_lock()" and dropping a Guard means
"spin_unlock()", since we cannot acquire a spinlock in one context and
release it in another context in kernel, so `Guard<..>` is `!Send`.

Back to the code here:

	unsafe impl Send for Registration {}

the safety comment needs to explain why the `Registration::drop` is safe
to call in a different context.

Hope this helps.

Regards,
Boqun

> > are talking abut the kernel modules module_init() and module_exit()
> > function. module_init() can be called before user space is even
> > started, but it could also be called by insmod. module_exit() could be
> > called by rmmod, but it could also be the kernel, after user space has
> > gone away on shutdown.
> 
> The kernel will not call module_exit() on shutdown.  Or has something
> changed recently?
> 
> > We are always in a context which can block, but
> > i never really think of this being threads. You can start a kernel
> > thread, and have some data structure exclusively used by that kernel
> > thread, but that is pretty unusual.
> > 
> > So i would probably turn this commenting around. Only comment like
> > this in the special case that a kernel thread exists, and it is
> > expected to have exclusive access.
> 
> With the driver model, you can be sure that your probe/release functions
> for the bus will never be called at the same time, and module_init/exit
> can never be called at the same time, so perhaps this isn't an issue?
> 
> thanks,
> 
> greg k-h
> 

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

[Andrew Lunn]

> One example of not `Send` type (or `!Send`) is spinlock guard:
> 
> 	let guard: Guard<..> = some_lock.lock();
> 
> creating a Guard means "spin_lock()" and dropping a Guard means
> "spin_unlock()", since we cannot acquire a spinlock in one context and
> release it in another context in kernel, so `Guard<..>` is `!Send`.

Thanks for the explanation. Kernel people might have a different
meaning for context, especially in this example. We have process
context and atomic context. Process context you are allowed to sleep,
atomic context you cannot sleep. If you are in process context and
take a spinlock, you change into atomic context. And when you release
the spinlock you go back to process context. So with this meaning of
context, you do acquire the spinlock in one context, and release it in
another.

So we are going to have to think about the context the word context is
used in, and expect kernel and Rust people to maybe think of it
differently.

	Andrew

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

[Boqun Feng]

On Sat, Nov 18, 2023 at 04:32:26PM +0100, Andrew Lunn wrote:
> > One example of not `Send` type (or `!Send`) is spinlock guard:
> > 
> > 	let guard: Guard<..> = some_lock.lock();
> > 
> > creating a Guard means "spin_lock()" and dropping a Guard means
> > "spin_unlock()", since we cannot acquire a spinlock in one context and
> > release it in another context in kernel, so `Guard<..>` is `!Send`.
> 
> Thanks for the explanation. Kernel people might have a different

Surely *we* do, and looks like I created more confusion ;-) Maybe I
should say "execution context" as in include/linux/preempt.h: NMI, hard
IRQ, softirq, task.

> meaning for context, especially in this example. We have process
> context and atomic context. Process context you are allowed to sleep,
> atomic context you cannot sleep. If you are in process context and
> take a spinlock, you change into atomic context. And when you release
> the spinlock you go back to process context. So with this meaning of
> context, you do acquire the spinlock in one context, and release it in
> another.
> 

Also as I tried to explain previously, the type of contexts doesn't
matter. Yes, once you hold a spinlock, you enter atomic context, but you
are still in the same task execution context, so acquiring and releasing
in the same task execution doesn't count as "Sending". But if after
acquired one somehow passes the guard to another task, or an interrupt
handler, that's "Sending".

> So we are going to have to think about the context the word context is
> used in, and expect kernel and Rust people to maybe think of it
> differently.
> 

In Rust doc [1], `Send` means:

	Types that can be transferred across thread boundaries.

but of course, we have more "thread-like" things in kernel, so I think
"execution context" may be a better term?

[1]: https://doc.rust-lang.org/core/marker/trait.Send.html

Regards,
Boqun

> 	Andrew

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

[Trevor Gross]

On Sat, Nov 18, 2023 at 9:54 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> In Rust doc [1], `Send` means:
>
>         Types that can be transferred across thread boundaries.
>
> but of course, we have more "thread-like" things in kernel, so I think
> "execution context" may be a better term?

The docs are pretty OS-focused here (I intend to update them at some point).

`Sync` means that if you have a >1 pointer/reference to a struct you
can access any of the fields, as allowed by the API, from any of the
references at any time (i.e. switching between any two instructions)
without causing data races. Atomic accesses or mutexes can add this
property to things that do not have it. It really doesn't matter
whether you're going between different user threads, kthreads,
interrupt/preemption contexts, or nothing at all. It's a bit more
intrinsic to the data type and it says how you _could_ use it rather
than how you do use it.

And then `Send` basically means that any pointers in your struct are
either exclusive or point to `Sync` things. Mutexes cannot add this
property to anything that does not have it.

Note that this still never lets you have more than one `&mut`
(`restrict`) reference to a piece of data at once, this mostly relates
to interior mutability (when things are allowed to be changed behind
shared `&` references - such as atomics).

The consumers of these markers are (1) the compiler knowing what can
live in statics, (2) APIs that make things potentially concurrent, and
(3) the compiler automatically marking new structs `Send`/`Sync` if
all member types follow these rules.

When trying to figure this out for C types, the question is just
whether usage of the type follows those rules. Or at least whether it
follows them whenever Rust has access to it.

---

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> +pub struct Registration {
> +    drivers: Pin<&'static mut [DriverVTable]>,
> +}
>
> [...]
>
> +// 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 {}

I don't think the impl here actually makes sense. `Registration` is a
buffer of references to `DriverVTable`. That type isn't marked Sync so
by the above rules, its references should not be either.

Tomo, does this need to be Sync at all? Probably easiest to drop the
impls if not, otherwise I think it is more correct to move them to
`DriverVTable`.  You may have had this before, I'm not sure if
discussion made you change it at some point...

---

> [1]: https://doc.rust-lang.org/core/marker/trait.Send.html
>
> Regards,
> Boqun

Sorry Boqun, the lengthy explanation is just for context and not aimed
at you in particular :)

- Trevor

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

[FUJITA Tomonori]

On Sun, 19 Nov 2023 05:06:23 -0600
Trevor Gross <tmgross@umich.edu> wrote:

>> +pub struct Registration {
>> +    drivers: Pin<&'static mut [DriverVTable]>,
>> +}
>>
>> [...]
>>
>> +// 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 {}
> 
> I don't think the impl here actually makes sense. `Registration` is a
> buffer of references to `DriverVTable`. That type isn't marked Sync so
> by the above rules, its references should not be either.
> 
> Tomo, does this need to be Sync at all? Probably easiest to drop the
> impls if not, otherwise I think it is more correct to move them to
> `DriverVTable`.  You may have had this before, I'm not sure if
> discussion made you change it at some point...

This needs to be Sync:

601 | pub struct Registration {
    |            ^^^^^^^^^^^^
note: required because it appears within the type `Module`
   --> drivers/net/phy/foo_rust.rs:5:1
    |
5   | / kernel::module_phy_driver! {
6   | |     drivers: [Foo],
7   | |     device_table: [
8   | |         DeviceId::new_with_driver::<Foo>()
...   |
13  | |     license: "GPL",
14  | | }
    | |_^
note: required by a bound in `kernel::Module`
   --> /home/ubuntu/git/linux/rust/kernel/lib.rs:69:27
    |
69  | pub trait Module: Sized + Sync {
    |                           ^^^^ required by this bound in `Module`
    = note: this error originates in the macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)


I'm not sure we discussed but making DriverVTable Sync works.

#[repr(transparent)]
pub struct DriverVTable(Opaque<bindings::phy_driver>);

// SAFETY: DriverVTable has no &self methods, so immutable references to it are useless.
unsafe impl Sync for DriverVTable {}


looks correct?

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

[Boqun Feng]

On Tue, Nov 21, 2023 at 11:13:06AM +0900, FUJITA Tomonori wrote:
[...]
> 
> I'm not sure we discussed but making DriverVTable Sync works.
> 
> #[repr(transparent)]
> pub struct DriverVTable(Opaque<bindings::phy_driver>);
> 
> // SAFETY: DriverVTable has no &self methods, so immutable references to it are useless.

Minor nitpicking, I would add one more sentense in the safety comment:

	therefore it's safe to share immutable references between
	threads.

or 
	therefore it's safe to share immutable references between
	execution contexts.

once we decide the term here ;-)

The reason is to match Sync definition [1]:

"""
Types for which it is safe to share references between threads.

This trait is automatically implemented when the compiler determines
it’s appropriate.

The precise definition is: a type T is Sync if and only if &T is Send.
In other words, if there is no possibility of undefined behavior
(including data races) when passing &T references between threads.
"""

[1]: https://doc.rust-lang.org/std/marker/trait.Sync.html

Regards,
Boqun

> unsafe impl Sync for DriverVTable {}
> 
> 
> looks correct?
> 

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

[FUJITA Tomonori]

On Fri, 17 Nov 2023 09:39:05 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>> 
>> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
>> 
>> This patch enables unstable const_maybe_uninit_zeroed feature for
>> kernel crate to enable unsafe code to handle a constant value with
>> uninitialized data. With the feature, the abstractions can initialize
>> a phy_driver structure with zero easily; instead of initializing all
>> the members by hand. It's supposed to be stable in the not so distant
>> future.
>> 
>> Link: https://github.com/rust-lang/rust/pull/116218
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> In this reply, I go through my minor nits:
> 
>> +use crate::{
>> +    prelude::{vtable, Pin},
>> +};
> 
> Normally, if you're importing specific prelude items by name instead of
> using prelude::*, then you would import them using their non-prelude
> path.

Understood. I have no reason to import specific prelude items so I use
`prelude::*` instead.


>> +#[derive(PartialEq)]
>> +pub enum DeviceState {
> 
> If you add PartialEq and you can add Eq, then also add Eq.

Added Eq.


>> +/// An adapter for the registration of a PHY driver.
>> +struct Adapter<T: Driver> {
>> +    _p: PhantomData<T>,
>> +}
> 
> You don't need this struct. The methods can be top-level functions.
> 
> But I know that others have used the same style of defining a useless
> struct, so it's fine with me.

You meant like the following?

unsafe extern "C" fn soft_reset_callback<T: Driver>(
    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)
    })
}

pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
    DriverVTable(Opaque::new(bindings::phy_driver {
	soft_reset: if T::HAS_SOFT_RESET {
            Some(soft_reset_callback::<T>)
        } else {
            None
	}
    }
}

I thought that a struct is used to group callbacks. Either is fine by
me though.


>> +    /// Defines certain other features this PHY supports.
>> +    /// It is a combination of the flags in the [`flags`] module.
>> +    const FLAGS: u32 = 0;
> 
> You need an empty line between the two lines if you intend for them to
> be separate lines in rendered documentation.

I don't intend to make them separate lines. If I make thme one line,
it's too long (over 100) so I made them two lines.


>> +#[vtable]
>> +pub trait Driver {
>> +    /// Issues a PHY software reset.
>> +    fn soft_reset(_dev: &mut Device) -> Result {
>> +        Err(code::ENOTSUPP)
>> +    }
>>      [...]
>> +}
> 
> I believe that the guidance for what to put in optional vtable-trait
> methods was changed in:
> 
> https://lore.kernel.org/all/20231026201855.1497680-1-benno.lossin@proton.me/

But VTABLE_DEFAULT_ERROR is defined in that patch, which isn't
merged. I'll update the code once that patch is merged.


>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Send for Registration {}
> 
> I would change this to "it's okay to call phy_drivers_unregister from a
> different thread than the one in which phy_drivers_register was called".
> 
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Sync for Registration {}
> 
> Here, you can say "Registration has no &self methods, so immutable
> references to it are useless".

I'll reply to Trevor mail on this issue.


>> +    // macro use only
>> +    #[doc(hidden)]
>> +    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
>> +        bindings::mdio_device_id {
>> +            phy_id: self.id,
>> +            phy_id_mask: self.mask.as_int(),
>> +        }
>> +    }
> 
> This is fine, but I probably would just expose it for everyone. It's not
> like it hurts if non-macro code can call this method, even if it doesn't
> have a reason to do so.

IIRC, someone said that drivers should not use bindings directly so
this function that returns bindings::mdio_device_id isn't exported.

Thanks!

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

[Andrew Lunn]

> I don't intend to make them separate lines. If I make thme one line,
> it's too long (over 100) so I made them two lines.

Any networking prefers 80 anyway, so shorter is better.

    Andrew

[PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

This macro creates an array of kernel's `struct phy_driver` and
registers it. This also corresponds to the kernel's
`MODULE_DEVICE_TABLE` macro, which embeds the information for module
loading into the module binary file.

A PHY driver should use this macro.

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

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 61223f638bc6..145d0407fe31 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -723,3 +723,146 @@ const fn as_int(&self) -> u32 {
         }
     }
 }
+
+/// Declares a kernel module for PHYs drivers.
+///
+/// This creates a static array of kernel's `struct phy_driver` and registers it.
+/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
+/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
+///
+/// # Examples
+///
+/// ```
+/// # mod module_phy_driver_sample {
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// kernel::module_phy_driver! {
+///     drivers: [PhyAX88772A],
+///     device_table: [
+///         DeviceId::new_with_driver::<PhyAX88772A>()
+///     ],
+///     name: "rust_asix_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust Asix PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhyAX88772A;
+///
+/// #[vtable]
+/// impl phy::Driver for PhyAX88772A {
+///     const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
+/// }
+/// # }
+/// ```
+///
+/// This expands to the following code:
+///
+/// ```ignore
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// struct Module {
+///     _reg: ::kernel::net::phy::Registration,
+/// }
+///
+/// module! {
+///     type: Module,
+///     name: "rust_asix_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust Asix PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhyAX88772A;
+///
+/// #[vtable]
+/// impl phy::Driver for PhyAX88772A {
+///     const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
+/// }
+///
+/// const _: () = {
+///     static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] =
+///         [::kernel::net::phy::create_phy_driver::<PhyAX88772A>()];
+///
+///     impl ::kernel::Module for Module {
+///         fn init(module: &'static ThisModule) -> Result<Self> {
+///             let drivers = unsafe { &mut DRIVERS };
+///             let mut reg = ::kernel::net::phy::Registration::register(
+///                 module,
+///                 ::core::pin::Pin::static_mut(drivers),
+///             )?;
+///             Ok(Module { _reg: reg })
+///         }
+///     }
+/// };
+///
+/// #[no_mangle]
+/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0x003b1861,
+///         phy_id_mask: 0xffffffff,
+///     },
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0,
+///         phy_id_mask: 0,
+///     },
+/// ];
+/// ```
+#[macro_export]
+macro_rules! module_phy_driver {
+    (@replace_expr $_t:tt $sub:expr) => {$sub};
+
+    (@count_devices $($x:expr),*) => {
+        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
+    };
+
+    (@device_table [$($dev:expr),+]) => {
+        #[no_mangle]
+        static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id;
+            $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [
+            $($dev.mdio_device_id()),+,
+            ::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)*
+        }
+
+        const _: () = {
+            static mut DRIVERS: [::kernel::net::phy::DriverVTable;
+                $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: The anonymous constant guarantees that nobody else can access
+                    // the `DRIVERS` static. The array is used only in the C side.
+                    let drivers = unsafe { &mut DRIVERS };
+                    let mut reg = ::kernel::net::phy::Registration::register(
+                        module,
+                        ::core::pin::Pin::static_mut(drivers),
+                    )?;
+                    Ok(Module { _reg: reg })
+                }
+            }
+        };
+
+        $crate::module_phy_driver!(@device_table [$($dev),+]);
+    }
+}
-- 
2.34.1

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Alice Ryhl]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> This macro creates an array of kernel's `struct phy_driver` and
> registers it. This also corresponds to the kernel's
> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
> loading into the module binary file.
> 
> A PHY driver should use this macro.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

A few minor nits:

> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {

Here, you can add $(,)? to allow trailing commas when the macro is used.
Like this:

(drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {


> +            ::kernel::bindings::mdio_device_id {

Here, I recommend `$crate` instead of `::kernel`.


> +/// #[no_mangle]
> +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
> +///     ::kernel::bindings::mdio_device_id {
> +///         phy_id: 0x003b1861,
> +///         phy_id_mask: 0xffffffff,
> +///     },
> +///     ::kernel::bindings::mdio_device_id {
> +///         phy_id: 0,
> +///         phy_id_mask: 0,
> +///     },
> +/// ];

I'd probably put a safety comment on the `#[no_mangle]` invocation to say that
"C will not read off the end of this constant since the last element is zero".

Alice

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Fri, 17 Nov 2023 09:39:08 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>> This macro creates an array of kernel's `struct phy_driver` and
>> registers it. This also corresponds to the kernel's
>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
>> loading into the module binary file.
>> 
>> A PHY driver should use this macro.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> A few minor nits:
> 
>> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
> 
> Here, you can add $(,)? to allow trailing commas when the macro is used.
> Like this:
> 
> (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {

Updated.

> 
>> +            ::kernel::bindings::mdio_device_id {
> 
> Here, I recommend `$crate` instead of `::kernel`.

I copied the code that Benno wrote, IIRC. Either is fine by me. Why
`$crate` is better here?

Also better to replace other `::kernel` in this macro?


>> +/// #[no_mangle]
>> +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
>> +///     ::kernel::bindings::mdio_device_id {
>> +///         phy_id: 0x003b1861,
>> +///         phy_id_mask: 0xffffffff,
>> +///     },
>> +///     ::kernel::bindings::mdio_device_id {
>> +///         phy_id: 0,
>> +///         phy_id_mask: 0,
>> +///     },
>> +/// ];
> 
> I'd probably put a safety comment on the `#[no_mangle]` invocation to say that
> "C will not read off the end of this constant since the last element is zero".

Added.

Thanks a lot!

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Benno Lossin]

On 19.11.23 11:50, FUJITA Tomonori wrote:
> On Fri, 17 Nov 2023 09:39:08 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>> +            ::kernel::bindings::mdio_device_id {
>>
>> Here, I recommend `$crate` instead of `::kernel`.
> 
> I copied the code that Benno wrote, IIRC. Either is fine by me. Why
> `$crate` is better here?

When I suggested that, I might have confused the location of the macro
being in the `macros` crate. There you cannot use `$crate`, since it
is not available for proc macros. But since this is in the `kernel`
crate, you can use `$crate`.

`$crate` is better, since it unambiguously refers to the current crate
and `::kernel` only works, because we named our crate `kernel`. So the
code using `$crate` is more portable.

> Also better to replace other `::kernel` in this macro?

Yes.

-- 
Cheers,
Benno

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Boqun Feng]

On Thu, Oct 26, 2023 at 09:10:47AM +0900, FUJITA Tomonori wrote:
[...]
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of kernel's `struct phy_driver` and registers it.
> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// # mod module_phy_driver_sample {
> +/// use kernel::c_str;
> +/// use kernel::net::phy::{self, DeviceId};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +///     drivers: [PhyAX88772A],
> +///     device_table: [
> +///         DeviceId::new_with_driver::<PhyAX88772A>()
> +///     ],
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +///
> +/// struct PhyAX88772A;
> +///
> +/// #[vtable]
> +/// impl phy::Driver for PhyAX88772A {
> +///     const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
> +///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
> +/// }
> +/// # }
> +/// ```

When run the following kunit command:

./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \
	--kconfig_add CONFIG_RUST=y \
	--kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \
	--kconfig_add CONFIG_PHYLIB=y \
	--kconfig_add CONFIG_NETDEVICES=y \
	--kconfig_add CONFIG_NET=y \
	--kconfig_add CONFIG_AX88796B_RUST_PHY=y \
	--kconfig_add CONFIG_AX88796B_PHY=y

I got the following errors:

	ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init
	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a
	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a

	ld.lld: error: duplicate symbol: __rust_asix_phy_exit
	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a
	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a

	ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table
	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
	>>>            rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a
	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
	>>>            drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a

Because kunit will use the above doc test to generate test, and since
CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has
been called twice, and causes duplicate symbols.

For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage
in the example. But for __mod_mdio__phydev_device_table, it's hard-coded
in `module_phy_driver!`, I don't have a quick fix right now. Also, does
it mean `module_phy_driver!` is only supposed to be "called" once for
the entire kernel?

Regards,
Boqun

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Andrew Lunn]

On Fri, Nov 17, 2023 at 02:21:38PM -0800, Boqun Feng wrote:
> On Thu, Oct 26, 2023 at 09:10:47AM +0900, FUJITA Tomonori wrote:
> [...]
> > +
> > +/// Declares a kernel module for PHYs drivers.
> > +///
> > +/// This creates a static array of kernel's `struct phy_driver` and registers it.
> > +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
> > +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// # mod module_phy_driver_sample {
> > +/// use kernel::c_str;
> > +/// use kernel::net::phy::{self, DeviceId};
> > +/// use kernel::prelude::*;
> > +///
> > +/// kernel::module_phy_driver! {
> > +///     drivers: [PhyAX88772A],
> > +///     device_table: [
> > +///         DeviceId::new_with_driver::<PhyAX88772A>()
> > +///     ],
> > +///     name: "rust_asix_phy",
> > +///     author: "Rust for Linux Contributors",
> > +///     description: "Rust Asix PHYs driver",
> > +///     license: "GPL",
> > +/// }
> > +///
> > +/// struct PhyAX88772A;
> > +///
> > +/// #[vtable]
> > +/// impl phy::Driver for PhyAX88772A {
> > +///     const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
> > +///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
> > +/// }
> > +/// # }
> > +/// ```
> 
> When run the following kunit command:
> 
> ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \
> 	--kconfig_add CONFIG_RUST=y \
> 	--kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \
> 	--kconfig_add CONFIG_PHYLIB=y \
> 	--kconfig_add CONFIG_NETDEVICES=y \
> 	--kconfig_add CONFIG_NET=y \
> 	--kconfig_add CONFIG_AX88796B_RUST_PHY=y \
> 	--kconfig_add CONFIG_AX88796B_PHY=y
> 
> I got the following errors:
> 
> 	ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __rust_asix_phy_exit
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a
> 
> Because kunit will use the above doc test to generate test, and since
> CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has
> been called twice, and causes duplicate symbols.
> 
> For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage
> in the example. But for __mod_mdio__phydev_device_table, it's hard-coded
> in `module_phy_driver!`, I don't have a quick fix right now. Also, does
> it mean `module_phy_driver!` is only supposed to be "called" once for
> the entire kernel?

Each kernel module should be in its own symbol name space. The only
symbols which are visible outside of the module are those exported
using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
export anything, in general.

Being built in also does not change this.

Neither drivers/net/phy/ax88796b_rust.o nor
rust/doctests_kernel_generated.o should have exported this symbol.

I've no idea how this actually works, i guess there are multiple
passes through the linker? Maybe once to resolve symbols across object
files within a module. Normal global symbols are then made local,
leaving only those exported with EXPORT_SYMBOL_GPL() or
EXPORT_SYMBOL()? A second pass through linker then links all the
exported symbols thorough the kernel?

	Andrew

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Benno Lossin]

On 11/17/23 23:54, Andrew Lunn wrote:
> Each kernel module should be in its own symbol name space. The only
> symbols which are visible outside of the module are those exported
> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
> export anything, in general.
> 
> Being built in also does not change this.
> 
> Neither drivers/net/phy/ax88796b_rust.o nor
> rust/doctests_kernel_generated.o should have exported this symbol.
> 
> I've no idea how this actually works, i guess there are multiple
> passes through the linker? Maybe once to resolve symbols across object
> files within a module. Normal global symbols are then made local,
> leaving only those exported with EXPORT_SYMBOL_GPL() or
> EXPORT_SYMBOL()? A second pass through linker then links all the
> exported symbols thorough the kernel?

I brought this issue up in [1], but I was a bit confused by your last
reply there, as I have no idea how the `EXPORT_SYMBOL` macros work.

IIRC on the Rust side all public items are automatically GPL exported.
But `#[no_mangle]` is probably a special case, since in userspace it
is usually used to do interop with C (and therefore the symbol is always
exported with the name not mangled).

One fix would be to make the name unique by using the type name supplied
in the macro.

[1]: https://lore.kernel.org/rust-for-linux/1aea7ddb-73b7-8228-161e-e2e4ff5bc98d@proton.me/

-- 
Cheers,
Benno

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Andrew Lunn]

On Fri, Nov 17, 2023 at 11:01:58PM +0000, Benno Lossin wrote:
> On 11/17/23 23:54, Andrew Lunn wrote:
> > Each kernel module should be in its own symbol name space. The only
> > symbols which are visible outside of the module are those exported
> > using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
> > export anything, in general.
> > 
> > Being built in also does not change this.
> > 
> > Neither drivers/net/phy/ax88796b_rust.o nor
> > rust/doctests_kernel_generated.o should have exported this symbol.
> > 
> > I've no idea how this actually works, i guess there are multiple
> > passes through the linker? Maybe once to resolve symbols across object
> > files within a module. Normal global symbols are then made local,
> > leaving only those exported with EXPORT_SYMBOL_GPL() or
> > EXPORT_SYMBOL()? A second pass through linker then links all the
> > exported symbols thorough the kernel?
> 
> I brought this issue up in [1], but I was a bit confused by your last
> reply there, as I have no idea how the `EXPORT_SYMBOL` macros work.
> 
> IIRC on the Rust side all public items are automatically GPL exported.

So that sounds wrong to me. But as i said, i don't know how this
actually works.

In kernel C code, you effectively have three levels of symbol
visibility.

Anything static is not visible outside of the .o file.

Anything global within a module is visible across multiple compilation
units within that module, but not visible outside of the module.

Symbols marked with EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL() are visible
through the entire kernel.

Device drivers generally don't have any EXPORT_SYMBOL* symbols. They
do however need to import symbols which are EXPORT_SYMBOL*.  Core
code, like phylib, has lots of EXPORT_SYMBOL*. You can also have
modules which are just libraries, and they use EXPORT_SYMBOL*. Other
modules will be linked to them.

> But `#[no_mangle]` is probably a special case, since in userspace it
> is usually used to do interop with C (and therefore the symbol is always
> exported with the name not mangled).

So you might need this for symbols which are EXPORT_SYMBOL*,
especially if they are going to be used by C code. If only other Rust
modules are going to use them, and the mangled name is predictable, i
suppose you could use the mangled name.

	Andrew

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Sat, 18 Nov 2023 00:18:10 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> But `#[no_mangle]` is probably a special case, since in userspace it
>> is usually used to do interop with C (and therefore the symbol is always
>> exported with the name not mangled).
> 
> So you might need this for symbols which are EXPORT_SYMBOL*,
> especially if they are going to be used by C code. If only other Rust
> modules are going to use them, and the mangled name is predictable, i
> suppose you could use the mangled name.

This isn't loaded into the kernel. This is used only by the tool to
build a module. When built as a module, this information is converted
into module alias information for dynamic loading.

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Fri, 17 Nov 2023 23:01:58 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 11/17/23 23:54, Andrew Lunn wrote:
>> Each kernel module should be in its own symbol name space. The only
>> symbols which are visible outside of the module are those exported
>> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
>> export anything, in general.
>> 
>> Being built in also does not change this.
>> 
>> Neither drivers/net/phy/ax88796b_rust.o nor
>> rust/doctests_kernel_generated.o should have exported this symbol.
>> 
>> I've no idea how this actually works, i guess there are multiple
>> passes through the linker? Maybe once to resolve symbols across object
>> files within a module. Normal global symbols are then made local,
>> leaving only those exported with EXPORT_SYMBOL_GPL() or
>> EXPORT_SYMBOL()? A second pass through linker then links all the
>> exported symbols thorough the kernel?
> 
> I brought this issue up in [1], but I was a bit confused by your last
> reply there, as I have no idea how the `EXPORT_SYMBOL` macros work.
> 
> IIRC on the Rust side all public items are automatically GPL exported.

Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL()
or EXPORT_SYMBOL_GPL().


> But `#[no_mangle]` is probably a special case, since in userspace it
> is usually used to do interop with C (and therefore the symbol is always
> exported with the name not mangled).
> 
> One fix would be to make the name unique by using the type name supplied
> in the macro.
> 
> [1]: https://lore.kernel.org/rust-for-linux/1aea7ddb-73b7-8228-161e-e2e4ff5bc98d@proton.me/

I fixed this in a different way; declaring
__mod_mdio__phydev_device_table only when built as a module.

(@device_table [$($dev:expr),+]) => {
 +        #[cfg(MODULE)]
          #[no_mangle]
          static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id;
              $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [

This is used for dynamic loading so when a phy driver is built-in, we
don't need this.

When a driver is built as a module, the user-space tool converts this
into moduole alias information. This __mod_mdio__phydev_device_table
isn't loaded into kernel so no conflict even when multiple phy drivers
are loaded.

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Andrew Lunn]

On Sun, Nov 19, 2023 at 06:25:44PM +0900, FUJITA Tomonori wrote:
> On Fri, 17 Nov 2023 23:01:58 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
> > On 11/17/23 23:54, Andrew Lunn wrote:
> >> Each kernel module should be in its own symbol name space. The only
> >> symbols which are visible outside of the module are those exported
> >> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
> >> export anything, in general.
> >> 
> >> Being built in also does not change this.
> >> 
> >> Neither drivers/net/phy/ax88796b_rust.o nor
> >> rust/doctests_kernel_generated.o should have exported this symbol.
> >> 
> >> I've no idea how this actually works, i guess there are multiple
> >> passes through the linker? Maybe once to resolve symbols across object
> >> files within a module. Normal global symbols are then made local,
> >> leaving only those exported with EXPORT_SYMBOL_GPL() or
> >> EXPORT_SYMBOL()? A second pass through linker then links all the
> >> exported symbols thorough the kernel?
> > 
> > I brought this issue up in [1], but I was a bit confused by your last
> > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work.
> > 
> > IIRC on the Rust side all public items are automatically GPL exported.
> 
> Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL()
> or EXPORT_SYMBOL_GPL().

Do they need to be public? Generally, a PHY driver does not export
anything. So you can probably make them private. We just however need
to ensure the compiler/linker does not think they are unused, so
throws them away.

I would however like to get an understanding how EXPORT_SYMBOL* is
supposed to work in rust. Can it really be hidden away? Or should
methods be explicitly marked like C code? What is the Rust equivalent
of the three levels of symbol scope we have in C?

	Andrew

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Sun, 19 Nov 2023 16:50:34 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Nov 19, 2023 at 06:25:44PM +0900, FUJITA Tomonori wrote:
>> On Fri, 17 Nov 2023 23:01:58 +0000
>> Benno Lossin <benno.lossin@proton.me> wrote:
>> 
>> > On 11/17/23 23:54, Andrew Lunn wrote:
>> >> Each kernel module should be in its own symbol name space. The only
>> >> symbols which are visible outside of the module are those exported
>> >> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
>> >> export anything, in general.
>> >> 
>> >> Being built in also does not change this.
>> >> 
>> >> Neither drivers/net/phy/ax88796b_rust.o nor
>> >> rust/doctests_kernel_generated.o should have exported this symbol.
>> >> 
>> >> I've no idea how this actually works, i guess there are multiple
>> >> passes through the linker? Maybe once to resolve symbols across object
>> >> files within a module. Normal global symbols are then made local,
>> >> leaving only those exported with EXPORT_SYMBOL_GPL() or
>> >> EXPORT_SYMBOL()? A second pass through linker then links all the
>> >> exported symbols thorough the kernel?
>> > 
>> > I brought this issue up in [1], but I was a bit confused by your last
>> > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work.
>> > 
>> > IIRC on the Rust side all public items are automatically GPL exported.
>> 
>> Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL()
>> or EXPORT_SYMBOL_GPL().
> 
> Do they need to be public? Generally, a PHY driver does not export
> anything. So you can probably make them private. We just however need
> to ensure the compiler/linker does not think they are unused, so
> throws them away.

The Rust ax88796b driver doesn't export anything. The Rust and C
drivers handle the device_table in the same way when they are built as
a module.

$ grep __mod_mdio /proc/kallsyms
ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust]

$ grep __mod_mdio /proc/kallsyms
ffffffffa0288010 d __mod_mdio__asix_tbl_device_table	[ax88796b]

Note that when the Rust ax88796b driver is built-in,
__mod_mdio__phydev_device_table is not defined.

Sorry about my explanation, which leads to the confusion, I think.


> I would however like to get an understanding how EXPORT_SYMBOL* is
> supposed to work in rust. Can it really be hidden away? Or should
> methods be explicitly marked like C code? What is the Rust equivalent
> of the three levels of symbol scope we have in C?

If I understand correctly, there is no official way to export symbols
in Rust kernel modules; No equivalent to EXPORT_SYMBOL* for Rust code
built as a module yet.

Note that all core Rust functions are GPL exported so they are
available to Rust kernel modules.

`static` has a different meaning in Rust but I think that you can use
file scope and kernel-module scope in Rust.

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[Andrew Lunn]

> The Rust ax88796b driver doesn't export anything. The Rust and C
> drivers handle the device_table in the same way when they are built as
> a module.
> 
> $ grep __mod_mdio /proc/kallsyms
> ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust]
> 
> $ grep __mod_mdio /proc/kallsyms
> ffffffffa0288010 d __mod_mdio__asix_tbl_device_table	[ax88796b]

I checked what r and d mean. If they are upper case, they are
exported. Lower case means they are not exported.

My laptop is using the realtek PHY driver:

0000000000000000 r __mod_mdio__realtek_tbl_device_table	[realtek]

Also lower r.

Looking at all the symbols for the realtek driver, all the symbols use
lower case. Nothing is exported.

Is that what you see for the ax88796b_rust?

	Andrew

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Mon, 20 Nov 2023 15:13:23 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> The Rust ax88796b driver doesn't export anything. The Rust and C
>> drivers handle the device_table in the same way when they are built as
>> a module.
>> 
>> $ grep __mod_mdio /proc/kallsyms
>> ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust]
>> 
>> $ grep __mod_mdio /proc/kallsyms
>> ffffffffa0288010 d __mod_mdio__asix_tbl_device_table	[ax88796b]
> 
> I checked what r and d mean. If they are upper case, they are
> exported. Lower case means they are not exported.
> 
> My laptop is using the realtek PHY driver:
> 
> 0000000000000000 r __mod_mdio__realtek_tbl_device_table	[realtek]
> 
> Also lower r.
> 
> Looking at all the symbols for the realtek driver, all the symbols use
> lower case. Nothing is exported.
> 
> Is that what you see for the ax88796b_rust?

Yes, all the symbols for ax88796b_rust use lower case.

Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

[FUJITA Tomonori]

On Fri, 17 Nov 2023 14:21:38 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> When run the following kunit command:
> 
> ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \
> 	--kconfig_add CONFIG_RUST=y \
> 	--kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \
> 	--kconfig_add CONFIG_PHYLIB=y \
> 	--kconfig_add CONFIG_NETDEVICES=y \
> 	--kconfig_add CONFIG_NET=y \
> 	--kconfig_add CONFIG_AX88796B_RUST_PHY=y \
> 	--kconfig_add CONFIG_AX88796B_PHY=y
> 
> I got the following errors:
> 
> 	ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __rust_asix_phy_exit
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a
> 
> Because kunit will use the above doc test to generate test, and since
> CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has
> been called twice, and causes duplicate symbols.
> 
> For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage
> in the example.

Surely, done.

> But for __mod_mdio__phydev_device_table, it's hard-coded
> in `module_phy_driver!`, I don't have a quick fix right now. Also, does
> it mean `module_phy_driver!` is only supposed to be "called" once for
> the entire kernel?

As I wrote in another mail, I fixed this.

[PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[FUJITA Tomonori]

From: Miguel Ojeda <ojeda@kernel.org>

This patch makes sure that the C's enum is sync with Rust sides. If
the enum is out of sync, compiling fails with an error like the
following.

Note that this is a temporary solution. It will be replaced with
bindgen when it supports generating the enum conversion code.

    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`

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/.gitignore                      |  1 +
 rust/Makefile                        | 14 +++++++++++
 rust/bindings/bindings_enum_check.rs | 36 ++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)
 create mode 100644 rust/bindings/bindings_enum_check.rs

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..a622111c8c50 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 $(srctree)/$(src)/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..eef7e9ca3c54
--- /dev/null
+++ b/rust/bindings/bindings_enum_check.rs
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bindings' enum exhaustiveness 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,
+) {
+}
-- 
2.34.1

Re: [PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[Miguel Ojeda]

On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> From: Miguel Ojeda <ojeda@kernel.org>
>
> This patch makes sure that the C's enum is sync with Rust sides. If
> the enum is out of sync, compiling fails with an error like the
> following.
>
> Note that this is a temporary solution. It will be replaced with
> bindgen when it supports generating the enum conversion code.

> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Please do not modify patches from others without warning that you did
so. I did not write this commit message nor agreed to this, but it
looks as if I did. I even explicitly said I would send the patch
independently.

As I recently told you, if you want to pick it up in your series to
showcase how it would work, you should have at least kept the WIP, put
it at the end of the series and added RFC since it is not intended to
be merged with your other patches.

Cheers,
Miguel

Re: [PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[FUJITA Tomonori]

On Thu, 26 Oct 2023 13:02:57 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> From: Miguel Ojeda <ojeda@kernel.org>
>>
>> This patch makes sure that the C's enum is sync with Rust sides. If
>> the enum is out of sync, compiling fails with an error like the
>> following.
>>
>> Note that this is a temporary solution. It will be replaced with
>> bindgen when it supports generating the enum conversion code.
> 
>> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Please do not modify patches from others without warning that you did
> so. I did not write this commit message nor agreed to this, but it
> looks as if I did. I even explicitly said I would send the patch
> independently.
>
> As I recently told you, if you want to pick it up in your series to
> showcase how it would work, you should have at least kept the WIP, put
> it at the end of the series and added RFC since it is not intended to
> be merged with your other patches.

Sorry, I totally misunderstand your intention. I thought that the PHY
abstractions needs to be merged with your patch together.

I'll drop your patch in the next version and focus on my patches.

Re: [PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[Miguel Ojeda]

On Thu, Oct 26, 2023 at 1:54 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Sorry, I totally misunderstand your intention. I thought that the PHY
> abstractions needs to be merged with your patch together.
>
> I'll drop your patch in the next version and focus on my patches.

No harm done! I understand you were trying to help, and I apologize if
I sounded too harsh.

Your abstractions are not blocked on this patch -- they could go in
without this, that is why I suggested marking this one as RFC and
putting it at the end of the series. The exhaustiveness check here is
an extra feature that prevents a class of bugs, which is great, but it
does not really affect the abstractions, i.e. there is no unsoundness
in your code whether this patch is in or not.

I will send the patch soon, and assuming it lands, then you can start
using the feature if you wish. I would recommend basing your patches
on top of that patch (or `rust-next` when the patch lands), so that
your PHY series contains the addition of `check_phy_state`.

Cheers,
Miguel

Re: [PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[Andrew Lunn]

On Thu, Oct 26, 2023 at 02:22:23PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 26, 2023 at 1:54 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > Sorry, I totally misunderstand your intention. I thought that the PHY
> > abstractions needs to be merged with your patch together.
> >
> > I'll drop your patch in the next version and focus on my patches.
> 
> No harm done! I understand you were trying to help, and I apologize if
> I sounded too harsh.
> 
> Your abstractions are not blocked on this patch -- they could go in
> without this, that is why I suggested marking this one as RFC and
> putting it at the end of the series.

That is not how netdev works. It messes up the patch flow, since the
machinery expects to commit all or nothing.

The best way forwards is you create a stable branch with this
patch. The netdev Maintainer can then pull that branch into netdev,
and Tomonori can then add his patches using it on top. When everything
meets up in linux-next, git then recognises it has the same patch
twice and drops one of them, depending on the order of the merge.

> I will send the patch soon, and assuming it lands, then you can start
> using the feature if you wish. I would recommend basing your patches
> on top of that patch (or `rust-next` when the patch lands),

That does not work. Networking patches need to be on net-next. The
stable branch solves that when we have cross subsystem dependencies.

       Andrew

Re: [PATCH net-next v7 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking

[Miguel Ojeda]

On Fri, Oct 27, 2023 at 2:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> That is not how netdev works. It messes up the patch flow, since the
> machinery expects to commit all or nothing.

Then please simply drop this patch (or improve the machinery :)

> The best way forwards is you create a stable branch with this
> patch. The netdev Maintainer can then pull that branch into netdev,
> and Tomonori can then add his patches using it on top. When everything
> meets up in linux-next, git then recognises it has the same patch
> twice and drops one of them, depending on the order of the merge.

That is only needed if you want to land all this in the next cycle, do
you? Moreover, it also assumes this exhaustiveness check lands -- it
has not been posted/discussed/agreed yet.

Thus, if either of those are false, then this bit (or the entire
series) could just wait one cycle.

> That does not work. Networking patches need to be on net-next.

I have not said the patches should not go through net-next, though.
And what I suggested definitely works.

> The
> stable branch solves that when we have cross subsystem dependencies.

Yes, we are aware of that, thank you. We are even doing it right now
with another subsystem.

Cheers,
Miguel

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

[FUJITA Tomonori]

Adds me as a maintainer and Trevor as a reviewer.

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

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

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

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

[Andrew Lunn]

On Thu, Oct 26, 2023 at 09:10:49AM +0900, FUJITA Tomonori wrote:
> Adds me as a maintainer and Trevor as a reviewer.
> 
> The files are placed at rust/kernel/ directory for now but the files
> are likely to be moved to net/ directory once a new Rust build system
> is implemented.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

    Andrew

[PATCH net-next v7 5/5] 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>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
 MAINTAINERS                      |   8 ++
 drivers/net/phy/Kconfig          |   8 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 5 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index f0f0610defd6..aad0325121e0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3058,6 +3058,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/net/asix,ax88796c.yaml
 F:	drivers/net/ethernet/asix/ax88796c_*
 
+ASIX PHY DRIVER [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:	Trevor Gross <tmgross@umich.edu>
+L:	netdev@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/ax88796b_rust.rs
+
 ASPEED CRYPTO DRIVER
 M:	Neal Liu <neal_liu@aspeedtech.com>
 L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 0faebdb184ca..11b18370a05b 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -115,6 +115,14 @@ config AX88796B_PHY
 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
 	  AX88796B package.
 
+config AX88796B_RUST_PHY
+	bool "Rust reference driver for Asix PHYs"
+	depends on RUST_PHYLIB_ABSTRACTIONS && AX88796B_PHY
+	help
+	  Uses the Rust reference driver for Asix PHYs (ax88796b_rust.ko).
+	  The features are equivalent. It supports the Asix Electronics PHY
+	  found in the X-Surf 100 AX88796B package.
+
 config BROADCOM_PHY
 	tristate "Broadcom 54XX PHYs"
 	select BCM_NET_PHYLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 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..6c24c94ab2db
--- /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.is_link_up() {
+            return Ok(0);
+        }
+        // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
+        // linkmode so use MII_BMCR as default values.
+        let ret = dev.read(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 v7 5/5] net: phy: add Rust Asix PHY driver

[Alice Ryhl]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 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>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Overall looks reasonable. I found various nits below:

There's a typo in your commit message: versionon.

> +use kernel::c_str;
> +use kernel::net::phy::{self, DeviceId, Driver};
> +use kernel::prelude::*;
> +use kernel::uapi;

You used the other import style in other patches.

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

The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
these constants are defined as the wrong type?

It's probably difficult to get bindgen to change the type, but you could
do this at the top of the function or file:

	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

> +            let _ = dev.init_hw();
> +            let _ = dev.start_aneg();

Just to confirm: You want to call `start_aneg` even if `init_hw` returns
failure? And you want to ignore both errors?

Alice

Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

On Fri, 17 Nov 2023 09:39:15 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>> 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>
>> Reviewed-by: Trevor Gross <tmgross@umich.edu>
>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> Overall looks reasonable. I found various nits below:
> 
> There's a typo in your commit message: versionon.

Oops, will fix.

> 
>> +use kernel::c_str;
>> +use kernel::net::phy::{self, DeviceId, Driver};
>> +use kernel::prelude::*;
>> +use kernel::uapi;
> 
> You used the other import style in other patches.

use kernel::{
    c_str,
    net::phy::{self, DeviceId, Driver},
    prelude::*,
    uapi,
};

?

>> +        // 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 {
> 
> The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
> these constants are defined as the wrong type?
> 
> It's probably difficult to get bindgen to change the type, but you could
> do this at the top of the function or file:

Yes, if we could specfy a type with bindgen, it's easier.

> 	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
> 	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

I'll.


>> +            let _ = dev.init_hw();
>> +            let _ = dev.start_aneg();
> 
> Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> failure? And you want to ignore both errors?

Yeah, I tried to implement the exact same behavior in the original C driver.


Thanks!

Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

[Andrew Lunn]

> >> +            let _ = dev.init_hw();
> >> +            let _ = dev.start_aneg();
> > 
> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> > failure? And you want to ignore both errors?
> 
> Yeah, I tried to implement the exact same behavior in the original C driver.

You probably could check the return values, and it would not make a
difference. Also, link_change_notify() is a void function, so you
cannot return the error anyway.

These low level functions basically only fail if the hardware is
`dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
recovery. You tend to get such errors during probe and fail the
probe. Or maybe if power management is wrong and it has turned a
critical clock off. But that is unlikely in this case, we are calling
link_change_notify because the PHY has told us something changed
recently, so it probably is alive.

I would say part of not checking the return code is also that C does
not have the nice feature that Rust has of making very simple to check
the return code. That combined with it being mostly pointless for PHY
drivers.

    Andrew

Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

[FUJITA Tomonori]

On Sun, 19 Nov 2023 17:03:30 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> >> +            let _ = dev.init_hw();
>> >> +            let _ = dev.start_aneg();
>> > 
>> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
>> > failure? And you want to ignore both errors?
>> 
>> Yeah, I tried to implement the exact same behavior in the original C driver.
> 
> You probably could check the return values, and it would not make a
> difference. Also, link_change_notify() is a void function, so you
> cannot return the error anyway.
> 
> These low level functions basically only fail if the hardware is
> `dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
> recovery. You tend to get such errors during probe and fail the
> probe. Or maybe if power management is wrong and it has turned a
> critical clock off. But that is unlikely in this case, we are calling
> link_change_notify because the PHY has told us something changed
> recently, so it probably is alive.
> 
> I would say part of not checking the return code is also that C does
> not have the nice feature that Rust has of making very simple to check
> the return code. That combined with it being mostly pointless for PHY
> drivers.

Understood. I'll check the first return value if you prefer. I might
add WARN_ON_ONCE after Rust supports it.

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index ce6640ff523f..7522070a6acd 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -95,7 +95,9 @@ 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();
+            if let Err(_) = dev.init_hw() {
+                return;
+            }
             let _ = dev.start_aneg();
         }
     }

Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

[Greg KH]

On Tue, Nov 21, 2023 at 03:19:39PM +0900, FUJITA Tomonori wrote:
> On Sun, 19 Nov 2023 17:03:30 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> >> +            let _ = dev.init_hw();
> >> >> +            let _ = dev.start_aneg();
> >> > 
> >> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> >> > failure? And you want to ignore both errors?
> >> 
> >> Yeah, I tried to implement the exact same behavior in the original C driver.
> > 
> > You probably could check the return values, and it would not make a
> > difference. Also, link_change_notify() is a void function, so you
> > cannot return the error anyway.
> > 
> > These low level functions basically only fail if the hardware is
> > `dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
> > recovery. You tend to get such errors during probe and fail the
> > probe. Or maybe if power management is wrong and it has turned a
> > critical clock off. But that is unlikely in this case, we are calling
> > link_change_notify because the PHY has told us something changed
> > recently, so it probably is alive.
> > 
> > I would say part of not checking the return code is also that C does
> > not have the nice feature that Rust has of making very simple to check
> > the return code. That combined with it being mostly pointless for PHY
> > drivers.
> 
> Understood. I'll check the first return value if you prefer. I might
> add WARN_ON_ONCE after Rust supports it.

Please don't, it shouldn't support it, handle errors properly and
return, don't panic machines (remember, the majority of the Linux
systems in the world run panic-on-warn).

thanks,
g
reg k-h

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

[Miguel Ojeda]

On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> 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.

This patch series has had 8 versions in a month. It would be better to
wait more between revisions for this kind of patch series, especially
when there is discussion still going on in the previous ones and it is
a new "type" of code.

I understand you are doing it so that people can see the latest
version (and thus focus on that), and that is helpful, but sending
versions very quickly when the discussions are ongoing splits the
discussions into several versions. That makes it more confusing for
reviewers, and essentially discourages people from being able to
follow everything.

That, in turn, contributes to the problem of reviewers repeating
feedback or missing context -- which you said you did not like.

Please see https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient

Thanks!

Cheers,
Miguel

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

[Andrew Lunn]

On Thu, Oct 26, 2023 at 12:39:46PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> 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.
> 
> This patch series has had 8 versions in a month. It would be better to
> wait more between revisions for this kind of patch series, especially
> when there is discussion still going on in the previous ones and it is
> a new "type" of code.

That is actually about right for netdev. As i said, netdev moves fast,
review comments are expected within about 3 days. We also say don't
post a new version within 24 hours. So that gives you an idea of the
min and max.

It is however good to let discussion reach some sort of conclusion,
but that also requires prompt discussion. And if that discussion is
not prompt, posting a new version is a way to kick reviewers into
action.

	Andrew

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

[Boqun Feng]

On Fri, Oct 27, 2023 at 01:48:42AM +0200, Andrew Lunn wrote:
> On Thu, Oct 26, 2023 at 12:39:46PM +0200, Miguel Ojeda wrote:
> > On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> 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.
> > 
> > This patch series has had 8 versions in a month. It would be better to
> > wait more between revisions for this kind of patch series, especially
> > when there is discussion still going on in the previous ones and it is
> > a new "type" of code.
> 
> That is actually about right for netdev. As i said, netdev moves fast,
> review comments are expected within about 3 days. We also say don't
> post a new version within 24 hours. So that gives you an idea of the
> min and max.
> 
> It is however good to let discussion reach some sort of conclusion,
> but that also requires prompt discussion. And if that discussion is
> not prompt, posting a new version is a way to kick reviewers into
> action.
> 

I wonder whether that actually helps, if a reviewer takes average four
days to review a version (wants to give accurate comments and doesn't
work on this full time), and the developer send a new version every
three days, there is no possible way for the developer to get the
reviews.

(Honestly, if people could reach out to a conclusion for anything in
three days, the world would be a much more peaceful place ;-))

Regards,
Boqun

> 	Andrew
> 

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

[Andrew Lunn]

> I wonder whether that actually helps, if a reviewer takes average four
> days to review a version (wants to give accurate comments and doesn't
> work on this full time), and the developer send a new version every
> three days, there is no possible way for the developer to get the
> reviews.
> 
> (Honestly, if people could reach out to a conclusion for anything in
> three days, the world would be a much more peaceful place ;-))

May i suggest you subscribe to the netdev list and watch it in action.

It should also be noted, patches don't need reviews to be merged. If
there is no feedback within three days, and it passes the CI tests, it
likely will be merged. Real problems can be fixed up later, if need
be.

	Andrew

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

[Boqun Feng]

On Fri, Oct 27, 2023 at 04:47:17AM +0200, Andrew Lunn wrote:
> > I wonder whether that actually helps, if a reviewer takes average four
> > days to review a version (wants to give accurate comments and doesn't
> > work on this full time), and the developer send a new version every
> > three days, there is no possible way for the developer to get the
> > reviews.
> > 
> > (Honestly, if people could reach out to a conclusion for anything in
> > three days, the world would be a much more peaceful place ;-))
> 
> May i suggest you subscribe to the netdev list and watch it in action.
> 

I'm sorry, I wasn't questioning about the process of netdev, I respect
the hard work behind that. I was simply wondering whether sending out a
new version so quickly is a good way to kick reviewers... it's sometimes
frustating to see new version post but old comments were not resolved,
it rather discourages reviewers..

> It should also be noted, patches don't need reviews to be merged. If
> there is no feedback within three days, and it passes the CI tests, it

Do the CI tests support Rust now? Does Tomo's patch pass CI? Looks like
something we'd like to see (and help).

> likely will be merged. Real problems can be fixed up later, if need
> be.

But this doesn't apply to pure API, right? So if some one post a pure
Rust API with no user, but some tests, and the CI passes, the API won't
get merged? Even though no review is fine and if API has problems, we
can fix it later?

Regards,
Boqun

> 
> 	Andrew
> 

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

[Boqun Feng]

On Thu, Oct 26, 2023 at 08:11:00PM -0700, Boqun Feng wrote:
[...]
> > likely will be merged. Real problems can be fixed up later, if need
> > be.
> 
> But this doesn't apply to pure API, right? So if some one post a pure
> Rust API with no user, but some tests, and the CI passes, the API won't
> get merged? Even though no review is fine and if API has problems, we
> can fix it later?
> 

I brought this up because (at least) at this stage one of the focus
areas of Rust is: how to wrap C structs and functions into a sound and
reasonable API. So it's ideal if we can see more API proposals.

As you may already see in the reviews of this patchset, a lot of effort
was spent on reviewing the API based on its designed semantics (rather
than its usage in the last patch). This is the extra effort of using
Rust. Is it worth? I don't know, the experiment will answer that in the
end ;-) But at least having an API design with a clear semantics and
some future guards is great.

The review because of this may take longer time than C code, so if we
really want to keep up with netdev speed, maybe we relax the
must-have-in-tree-user rule, so that we can review API design and
soundness in our pace.

Regards,
Boqun

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

[Andrew Lunn]

On Thu, Oct 26, 2023 at 09:26:05PM -0700, Boqun Feng wrote:
> On Thu, Oct 26, 2023 at 08:11:00PM -0700, Boqun Feng wrote:
> [...]
> > > likely will be merged. Real problems can be fixed up later, if need
> > > be.
> > 
> > But this doesn't apply to pure API, right? So if some one post a pure
> > Rust API with no user, but some tests, and the CI passes, the API won't
> > get merged? Even though no review is fine and if API has problems, we
> > can fix it later?
> > 
> 
> I brought this up because (at least) at this stage one of the focus
> areas of Rust is: how to wrap C structs and functions into a sound and
> reasonable API. So it's ideal if we can see more API proposals.
> 
> As you may already see in the reviews of this patchset, a lot of effort
> was spent on reviewing the API based on its designed semantics (rather
> than its usage in the last patch). This is the extra effort of using
> Rust. Is it worth? I don't know, the experiment will answer that in the
> end ;-) But at least having an API design with a clear semantics and
> some future guards is great.
> 
> The review because of this may take longer time than C code, so if we
> really want to keep up with netdev speed, maybe we relax the
> must-have-in-tree-user rule, so that we can review API design and
> soundness in our pace.

The rule about having a user is there for a few reasons:

Without seeing the user, you cannot say if the API makes sense.  How
is the user using the API? Is the architecture of the user correct?
Fixing the architecture of the user could change the API. As an
example, i've seen Rust wrapper on top of sockets. The read/write
method use a plain block of memory. However, for a network filesystem,
what you actually need to send is probably represented by a folio of
pages in the buffer cache. Its more efficient to hand the folio over
to the network stack. If the data is coming from user space, its
probably coming via a socket. So its probably in the form of a list of
chunks of data, maybe still in userspace, maybe with a header added in
kernel space. You want to pass that representation to the stack, which
can already handle it in an optimal way. A plain block of memory could
in fact be correct, there are use cases where its simple command being
sent to a modem. But its impossible to say what is correct unless you
can use the user.

Another point is that internal API are unstable. Any developer can
change any API anywhere in the kernel, if they can justify it. That is
an important feature of the kernel, and we don't like making it harder
to change APIs. If you cannot see both sides of an API, its hard to
know if you can change it, or how you need to change it. And at the
moment, there are very few Rust developers. An API in Rust is going to
be harder for a developer to change. So we have a strong reason to
keep Rust APIs minimal, with clear users.

But this API unstableness plays both ways. You don't need a perfect
API before it is merged. You just need it good enough. You can keep
working on it once its merged. If you missed something which makes is
unsound, not a problem, its just a bug, fix it an move on, like any
other bug.

	Andrew    

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

[Miguel Ojeda]

On Fri, Oct 27, 2023 at 4:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Without seeing the user, you cannot say if the API makes sense.

(snip)

We are aware of all that, thank you.

But I am not sure what you are trying to point out. If I understand
correctly, Boqun was just suggesting an idea to split the pace, not
disputing the value of the rule or asking why it is in place.

> But this API unstableness plays both ways. You don't need a perfect
> API before it is merged. You just need it good enough. You can keep
> working on it once its merged.

Indeed, but there is no rush to put things in either. And for us,
"good enough" so far (i.e. for the very first abstractions), means
"everyone is in on the same page, no known soundness issues,
well-designed API that can serve as an example, enough time to review,
etc.".

In other words, we are trying to build consensus, not just put code
into the kernel.

> If you missed something which makes is
> unsound, not a problem, its just a bug, fix it an move on, like any
> other bug.

Sure, as long as you consider them stable-worthy issues and are happy
taking patches that may need to substantially change an API to fix the
unsoundness in some cases, that is fine.

Cheers,
Miguel

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

[Andrew Lunn]

> Do the CI tests support Rust now? Does Tomo's patch pass CI? Looks like
> something we'd like to see (and help).

Its work in progress.

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

These are the current tests we have. You can see it fails two tests. I
would say neither are blockers. netdev does try to stick to 80
character line length, so it would be nice to fix that. The checkpatch
warning about the Kconfig help can also be ignored.

There are currently a few builds performed for each patch, once with
gcc and once with clang/llvm. These use the allmodconfig kernel
configuration, since that generally builds the most code. However,
Rust is not enabled in the configuration. So i submitted a new test,
based on the clang build, which massages the kernel configuration to
actually enable Rust, and to ensure the dependencies are fulfilled to
allow the PHY driver to be enabled, and then enable it. We want the
build with a patch to have equal or less errors and warning from the
toolchain.

Its not clear how long it will take before this new test becomes
active. The build machine does not have a Rust toolchain yet, etc.  To
make up for that, i just build the series myself on my machine, and it
builds cleanly for me.

We are also open to add more tests. You will get more return on
investment if you extend the C=1 checks, since that is used in a lot
more places than networking. But we can add more tests to the
networking CI system, if you can tell us what to test, how to test it,
and how to evaluate the results.

> > likely will be merged. Real problems can be fixed up later, if need
> > be.
> 
> But this doesn't apply to pure API, right? So if some one post a pure
> Rust API with no user, but some tests, and the CI passes, the API won't
> get merged? Even though no review is fine and if API has problems, we
> can fix it later?

There is always a human involved. If a reviewer does not pick up the
missing user, the Maintainer should and reject the patch.

	Andrew

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

[Miguel Ojeda]

On Fri, Oct 27, 2023 at 4:47 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> It should also be noted, patches don't need reviews to be merged. If
> there is no feedback within three days, and it passes the CI tests, it
> likely will be merged. Real problems can be fixed up later, if need
> be.

Passing CI tests does not tell you whether abstractions are
well-designed or sound, which is the key property Rust abstractions
need.

And I hope by "don't need reviews to be merged" you mean "at least
somebody, perhaps the applier, has taken a look".

Cheers,
Miguel

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

[Andrew Lunn]

On Fri, Oct 27, 2023 at 12:22:07PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 27, 2023 at 4:47 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > It should also be noted, patches don't need reviews to be merged. If
> > there is no feedback within three days, and it passes the CI tests, it
> > likely will be merged. Real problems can be fixed up later, if need
> > be.
> 
> Passing CI tests does not tell you whether abstractions are
> well-designed or sound, which is the key property Rust abstractions
> need.

We see CI as a tool to do the boring stuff. Does the code even compile
without adding errors/warnings. Does it have the needed signed-off-by,
does checkpatch spot anything nasty going on. Part of being able to
handle the volume of patches in netdev is automating all this sort of
stuff.

> And I hope by "don't need reviews to be merged" you mean "at least
> somebody, perhaps the applier, has taken a look".

A human is always involved, looking at the CI results, and if nothing
bad is reported, looking at the code if there are no Acked-by, or
Reviewed-by from trusted people.

The API being unsound is just another bug. I nobody spots the problem
it can be fixed, just like any other bug. 

	Andrew

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

[Miguel Ojeda]

On Fri, Oct 27, 2023 at 1:48 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> That is actually about right for netdev. As i said, netdev moves fast,
> review comments are expected within about 3 days. We also say don't
> post a new version within 24 hours. So that gives you an idea of the
> min and max.

And as I said when you told us that, that may be the right policy for
C netdev, which has been around for a long time, has a well supported
infrastructure, the code base is well-known, etc.

For Rust, it is not, for multiple reasons. For starters, we are not
talking here about just another patch to your subsystem. This is a
whole new subsystem API, in a new language, that needs careful design.
Moreover, Rust abstractions are supposed to be "sound" (a property
that C APIs do not need).

On top of that, two subsystems are reviewing it, and on our side we
simply do not have the resources to commit to netdev review pace, as
we told you privately. I am aware of at least 3 reviewers who have not
had the time to look into the more recent versions yet, and it is
unlikely they will have time before LPC. I would really recommend they
are given the chance to give feedback.

So, if you appreciate/want/need our feedback, you will need to be a
bit more patient.

By the way, your docs say patches are triaged in less than 48 hours as
well as "Generally speaking". From that wording, I wouldn't expect
every single patch to take 48 hours to be fully reviewed (not just
triaged), and I would say the "very first Rust abstractions code" is
not a common situation.

> It is however good to let discussion reach some sort of conclusion,
> but that also requires prompt discussion.

I don't see why that would require "prompt discussion".

> And if that discussion is
> not prompt, posting a new version is a way to kick reviewers into
> action.

No, sorry, posting a new version in order to push reviewers is not the
right thing to do. The patches were not being ignored.

From your own (netdev) docs -- not even the general kernel ones:

    "Do not post a new version of the code if the discussion about the
previous version is still ongoing, unless directly instructed by a
reviewer."

    "Asking the maintainer for status updates on your patch is a good
way to ensure your patch is ignored or pushed to the bottom of the
priority list."

    "Make sure you address all the feedback in your new posting."

Cheers,
Miguel

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

[Jakub Kicinski]

On Fri, 27 Oct 2023 12:21:33 +0200 Miguel Ojeda wrote:
> And as I said when you told us that, that may be the right policy for
> C netdev, which has been around for a long time, has a well supported
> infrastructure, the code base is well-known, etc.
> 
> For Rust, it is not, for multiple reasons. For starters, we are not
> talking here about just another patch to your subsystem. This is a
> whole new subsystem API, in a new language, that needs careful design.
> Moreover, Rust abstractions are supposed to be "sound" (a property
> that C APIs do not need).

Nobody is questioning that.

> On top of that, two subsystems are reviewing it, and on our side we
> simply do not have the resources to commit to netdev review pace, as
> we told you privately. I am aware of at least 3 reviewers who have not
> had the time to look into the more recent versions yet, and it is
> unlikely they will have time before LPC. I would really recommend they
> are given the chance to give feedback.
> 
> So, if you appreciate/want/need our feedback, you will need to be a
> bit more patient.

To be sure our process is not misunderstood - it's not about impatience
(🥴️) or some rules we made up.  We get slightly over 100 patches a day
(for us to apply, not subtrees). Longer review cycles would make keeping
track of patches and discussions unmanageable.

Is the expectation that over time you'll be less and less involved
in particular subsystems? What's the patch volume right now for Rust?

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

[Miguel Ojeda]

On Fri, Oct 27, 2023 at 4:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> To be sure our process is not misunderstood - it's not about impatience
> (🥴️) or some rules we made up.  We get slightly over 100 patches a day
> (for us to apply, not subtrees). Longer review cycles would make keeping
> track of patches and discussions unmanageable.

Of course -- that is completely understandable. We are not trying to
change how netdev works. If you have some policies they are probably
the best for your situation.

To be clear, we are not the ones that want to upstream this code. In
fact, we have other items that have higher priority for us. But
Tomonori submitted this, and now we are being told that the Rust
subsystem somehow has to provide reviews within days. We cannot commit
to that, and that is what we told Andrew privately.

In addition, for Rust, we are trying to get the very first
abstractions that get into mainline as reasonably well-designed as we
can (and ideally "sound"), so that they can serve as an example. This
takes time, and typically several iterations.

> Is the expectation that over time you'll be less and less involved
> in particular subsystems? What's the patch volume right now for Rust?

Indeed, the plan is that eventually each subsystem handles Rust like
any other thing. Otherwise, it does not scale.

In fact, the sooner that happens, the better, but we would like to
have some consistency and getting people on the same page around what
is expected from Rust code and abstractions. This also takes time,
because it means typically talking and discussing with people.

For instance, as a trivial example, Andrew raised the maximum length
of a line in one of the last messages. We would like to avoid this
kind of difference between parts of the kernel -- it is the only
chance we will get, and there is really no reason to be inconsistent
(ideally, even automated, where possible).

Now, if netdev is extremely busy, then precisely because of that, it
may be a good idea to take the Rust stuff slowly, so that, by the time
it is in, you can actually handle any patches swiftly without having
to wait on us.

Cheers,
Miguel

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

[Andrew Lunn]

> For instance, as a trivial example, Andrew raised the maximum length
> of a line in one of the last messages. We would like to avoid this
> kind of difference between parts of the kernel -- it is the only
> chance we will get, and there is really no reason to be inconsistent
> (ideally, even automated, where possible).

It should be noted that netdev prefers 80, which the coding standard
expresses as the preferred value. checkpatch however now allows up to
100. The netdev CI job adds an extra parameter to checkpatch to
enforce the preferred 80.

You will probably find different levels of acceptance of 80 to 100 in
different subsystems. So i'm not sure you will be able to achieve
consistence.

It should also be noted that 80, or 100, is not a strict limit. Being
able to grep the kernel for strings is important. So the coding
standard allows you to go passed this limit in order that you don't
need to break a string. checkpatch understands this. I don't know if
your automated tools support such exceptions.

     Andrew

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

[Miguel Ojeda]

On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> You will probably find different levels of acceptance of 80 to 100 in
> different subsystems. So i'm not sure you will be able to achieve
> consistence.

I would definitely agree if this were C. I happen to maintain
`.clang-format`, and it was an interesting process to approximate a
"common enough" style as the base one.

But for Rust, it is easy, because all the code is new. All Rust files
are formatted the same way, across the entire kernel.

> It should also be noted that 80, or 100, is not a strict limit. Being
> able to grep the kernel for strings is important. So the coding
> standard allows you to go passed this limit in order that you don't
> need to break a string. checkpatch understands this. I don't know if
> your automated tools support such exceptions.

Not breaking string literals is the default behavior of `rustfmt` (and
we use its default behavior).

It is also definitely possible to turn off `rustfmt` locally, i.e. for
particular "items" (e.g. a function, a block, a statement), rather
than lines, which is very convenient.

However, as far as I recall, we have never needed to disable it. I am
sure it will eventually be needed somewhere, but what I am trying to
say is that it works well enough that one can just use it.

Cheers,
Miguel

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

[Benno Lossin]

On 10/28/23 13:07, Miguel Ojeda wrote:
> On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote:
>> It should also be noted that 80, or 100, is not a strict limit. Being
>> able to grep the kernel for strings is important. So the coding
>> standard allows you to go passed this limit in order that you don't
>> need to break a string. checkpatch understands this. I don't know if
>> your automated tools support such exceptions.
> 
> Not breaking string literals is the default behavior of `rustfmt` (and
> we use its default behavior).
> 
> It is also definitely possible to turn off `rustfmt` locally, i.e. for
> particular "items" (e.g. a function, a block, a statement), rather
> than lines, which is very convenient.
> 
> However, as far as I recall, we have never needed to disable it. I am
> sure it will eventually be needed somewhere, but what I am trying to
> say is that it works well enough that one can just use it.

We have it disabled on the `pub mod code` in error.rs line 20:
https://elixir.bootlin.com/linux/latest/source/rust/kernel/error.rs#L20

-- 
Cheers,
Benno

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

[Miguel Ojeda]

On Sat, Oct 28, 2023 at 1:41 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> We have it disabled on the `pub mod code` in error.rs line 20:
> https://elixir.bootlin.com/linux/latest/source/rust/kernel/error.rs#L20

Thanks Benno -- I checked the `rust` branch, and forgot about that one.

It is an interesting example though, because it could have gone either
way: we were not using it for the same code in the `rust` branch -- it
was an aesthetic improvement for consistency. In fact, if we do the
macro differently, it would not be needed.

Cheers,
Miguel

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

[Andrew Lunn]

On Sat, Oct 28, 2023 at 01:07:43PM +0200, Miguel Ojeda wrote:
> On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > You will probably find different levels of acceptance of 80 to 100 in
> > different subsystems. So i'm not sure you will be able to achieve
> > consistence.
> 
> I would definitely agree if this were C. I happen to maintain
> `.clang-format`, and it was an interesting process to approximate a
> "common enough" style as the base one.
> 
> But for Rust, it is easy, because all the code is new. All Rust files
> are formatted the same way, across the entire kernel.

I would say this is not a technical issue, but a social one. In order
to keep the netdev people happy, you are going to limit it to 80. But
i would not be too surprised if another subsystem says the code would
be more readable with 100, like the C code in our subsystem. We want
Rust to be 100 as well.

Linux can be very fragmented like this across subsystems. Its just the
way it is, and you might just have to fit in. I don't know, we will
see.

	Andrew

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

[Miguel Ojeda]

On Sat, Oct 28, 2023 at 5:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I would say this is not a technical issue, but a social one. In order
> to keep the netdev people happy, you are going to limit it to 80. But
> i would not be too surprised if another subsystem says the code would
> be more readable with 100, like the C code in our subsystem. We want
> Rust to be 100 as well.

To be clear, we don't really care about 80, 100, or 120, or tabs vs.
spaces. What we want is consistency, and it was decided to go with the
defaults of `rustfmt`.

We may want to tweak a knob here or there in the future, but it would
still apply to every single file.

> Linux can be very fragmented like this across subsystems. Its just the
> way it is, and you might just have to fit in. I don't know, we will
> see.

Yes, but that is an artifact of history and the tools used at the
time. We can improve things now, thus why it was decided to do it
consistently for all Rust code.

Cheers,
Miguel