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

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

[FUJITA Tomonori]

Hi,

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

Without the user of the abstractions, it's hard to review so I include
the [4/4] patch, which adds the Asix PHY driver to samples/rust.

If you are interested in the past discussion like why abstractions for
PHY drivers?, please check out [3]. The short answer is it's much
easier to implement useful abstractions for PHY drivers rather than
for network drivers.

Before submitting to netdev, I would like to iron out Rust issues.
Any feedback is welcome. Thanks!

[1] https://github.com/fujita/rust-realtek-phy
[2] https://github.com/fujita/rust-asix-phy
[3] https://lwn.net/ml/netdev/20230710073703.147351-1-fujita.tomonori@gmail.com/


FUJITA Tomonori (4):
  rust: core abstractions for network PHY drivers
  rust: phy: add module device table support
  MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY
    LIBRARY
  sample: rust: add Asix PHY driver

 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 687 ++++++++++++++++++++++++++++++++
 samples/rust/Kconfig            |  13 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_ax88796b.rs   | 105 +++++
 8 files changed, 818 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs
 create mode 100644 samples/rust/rust_ax88796b.rs


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

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

[FUJITA Tomonori]

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

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

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..e84fa513dfda 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,8 @@
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..b49b052969e5
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_PHYLIB)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..2c5ac5e3213a
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,651 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::vtable, prelude::Box, str::CStr, types::Opaque};
+use core::marker::PhantomData;
+
+/// Corresponds to the kernel's `enum phy_state`.
+#[derive(PartialEq)]
+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+    /// Unknown, not defined in the kernel.
+    Unknown,
+}
+
+/// Wraps the kernel's `struct phy_device`.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
+    /// may read or write to the `phy_device` object.
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        unsafe { &mut *(ptr as *mut Self) }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn id(&mut self) -> u32 {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Returns true if the PHY has no link.
+    pub fn link(&mut self) -> bool {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe { (*phydev).link() != 0 }
+    }
+
+    /// Gets the state of the PHY.
+    pub fn state(&mut self) -> DeviceState {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        let state = unsafe { (*phydev).state };
+        match state {
+            0 => DeviceState::Down,
+            1 => DeviceState::Ready,
+            2 => DeviceState::Halted,
+            3 => DeviceState::Error,
+            4 => DeviceState::Up,
+            5 => DeviceState::Running,
+            6 => DeviceState::NoLink,
+            7 => DeviceState::CableTest,
+            _ => DeviceState::Unknown,
+        }
+    }
+
+    /// Returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&mut self) -> bool {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
+    }
+
+    /// Returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&mut self) -> bool {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe { (*phydev).autoneg_complete() != 0 }
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&mut self, speed: i32) {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe {
+            (*phydev).speed = speed;
+        }
+    }
+
+    /// Sets duplex mode.
+    pub fn set_duplex(&mut self, is_full: bool) {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
+        unsafe {
+            if is_full {
+                (*phydev).duplex = bindings::DUPLEX_FULL as i32;
+            } else {
+                (*phydev).duplex = bindings::DUPLEX_HALF as i32;
+            }
+        }
+    }
+
+    /// Executes software reset the PHY via BMCR_RESET bit.
+    pub fn soft_reset(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_soft_reset(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Reads a given PHY register.
+    pub fn read(&mut self, regnum: u32) -> Result<i32> {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret =
+            unsafe { bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret)
+        }
+    }
+
+    /// Writes a given PHY register.
+    pub fn write(&mut self, regnum: u32, val: u16) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum, val)
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Reads a given PHY register without the MDIO bus lock taken.
+    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret =
+            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret)
+        }
+    }
+
+    /// Writes a given PHY register without the MDIO bus lock taken.
+    pub fn lockless_write(&mut self, regnum: u32, val: u16) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::__mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum, val)
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Resumes the PHY via BMCR_PDOWN bit.
+    pub fn resume(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_resume(phydev) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Suspends the PHY via BMCR_PDOWN bit.
+    pub fn suspend(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_suspend(phydev) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Checks the link status and updates current link state.
+    pub fn read_status(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // 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(())
+        }
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&mut self, page: i32, regnum: u32) -> Result<i32> {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page, regnum) };
+        if ret < 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(ret)
+        }
+    }
+
+    /// Updates the link status.
+    pub fn update_link(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_update_link(phydev) };
+        if ret < 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Reads Link partner ability.
+    pub fn read_lpa(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_lpa(phydev) };
+        if ret < 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Resolves the advertisements into PHY settings.
+    pub fn resolve_aneg_linkmode(&mut self) {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        unsafe {
+            bindings::phy_resolve_aneg_linkmode(phydev);
+        }
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_init_hw(phydev) };
+        if ret != 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Starts auto-negotiation.
+    pub fn start_aneg(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_start_aneg(phydev) };
+        if ret != 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Reads PHY abilities from Clause 22 registers.
+    pub fn read_abilities(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_abilities(phydev) };
+        if ret != 0 {
+            return Err(Error::from_errno(ret));
+        } else {
+            Ok(())
+        }
+    }
+}
+
+/// PHY is internal.
+pub const PHY_IS_INTERNAL: u32 = 0x00000001;
+/// PHY needs to be reset after the refclk is enabled.
+pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
+/// Polling is used to detect PHY status changes.
+pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
+/// Don't suspend.
+pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
+
+/// An adapter for the registration of a PHY driver.
+pub struct Adapter<T: Driver> {
+    name: &'static CStr,
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    /// Creates a new `Adapter` instance.
+    pub const fn new(name: &'static CStr) -> Adapter<T> {
+        Self {
+            name,
+            _p: PhantomData,
+        }
+    }
+
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn read_page_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            let ret = T::read_page(dev)?;
+            Ok(ret)
+        })
+    }
+
+    unsafe extern "C" fn write_page_callback(
+        phydev: *mut bindings::phy_device,
+        page: i32,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_page(dev, page)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            let ret = T::read_mmd(dev, devnum, regnum)?;
+            Ok(ret)
+        })
+    }
+
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+
+    fn driver(&self) -> bindings::phy_driver {
+        bindings::phy_driver {
+            name: self.name.as_char_ptr() as *mut i8,
+            flags: <T>::FLAGS,
+            soft_reset: if <T>::HAS_SOFT_RESET {
+                Some(Self::soft_reset_callback)
+            } else {
+                None
+            },
+            get_features: if <T>::HAS_GET_FEATURES {
+                Some(Self::get_features_callback)
+            } else {
+                None
+            },
+            match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
+                Some(Self::match_phy_device_callback)
+            } else {
+                None
+            },
+            suspend: if <T>::HAS_SUSPEND {
+                Some(Self::suspend_callback)
+            } else {
+                None
+            },
+            resume: if <T>::HAS_RESUME {
+                Some(Self::resume_callback)
+            } else {
+                None
+            },
+            config_aneg: if <T>::HAS_CONFIG_ANEG {
+                Some(Self::config_aneg_callback)
+            } else {
+                None
+            },
+            read_status: if <T>::HAS_READ_STATUS {
+                Some(Self::read_status_callback)
+            } else {
+                None
+            },
+            read_page: if <T>::HAS_READ_PAGE {
+                Some(Self::read_page_callback)
+            } else {
+                None
+            },
+            write_page: if <T>::HAS_WRITE_PAGE {
+                Some(Self::write_page_callback)
+            } else {
+                None
+            },
+            read_mmd: if <T>::HAS_READ_MMD {
+                Some(Self::read_mmd_callback)
+            } else {
+                None
+            },
+            write_mmd: if <T>::HAS_WRITE_MMD {
+                Some(Self::write_mmd_callback)
+            } else {
+                None
+            },
+            link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
+                Some(Self::link_change_notify_callback)
+            } else {
+                None
+            },
+            ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+        }
+    }
+}
+
+/// Corresponds to functions in `struct phy_driver`.
+#[vtable]
+pub trait Driver {
+    /// Corresponds to `flags` in `struct phy_driver`.
+    const FLAGS: u32 = 0;
+
+    /// Corresponds to `soft_reset` in `struct phy_driver`.
+    fn soft_reset(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `get_features` in `struct phy_driver`.
+    fn get_features(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `match_phy_device` in `struct phy_driver`.
+    fn match_phy_device(_dev: &mut Device) -> bool;
+
+    /// Corresponds to `config_aneg` in `struct phy_driver`.
+    fn config_aneg(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `read_page` in `struct phy_driver`.
+    fn read_page(_dev: &mut Device) -> Result<i32> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `write_page` in `struct phy_driver`.
+    fn write_page(_dev: &mut Device, _page: i32) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `read_status` in `struct phy_driver`.
+    fn read_status(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `suspend` in `struct phy_driver`.
+    fn suspend(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `resume` in `struct phy_driver`.
+    fn resume(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `read_mmd` in `struct phy_driver`.
+    fn read_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16) -> Result<i32> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `write_mmd` in `struct phy_driver`.
+    fn write_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Corresponds to `link_change_notify` in `struct phy_driver`.
+    fn link_change_notify(_dev: &mut Device) {}
+}
+
+/// Registration structure for a PHY driver.
+///
+/// `Registration` can keep multiple `phy_drivers` object because
+/// commonly one module implements multiple PHYs drivers.
+pub struct Registration<const N: usize> {
+    module: &'static crate::ThisModule,
+    drivers: [Option<Box<bindings::phy_driver>>; N],
+}
+
+impl<const N: usize> Registration<{ N }> {
+    /// Creates a new `Registration` instance.
+    pub fn new(module: &'static crate::ThisModule) -> Self {
+        const NONE: Option<Box<bindings::phy_driver>> = None;
+        Registration {
+            module,
+            drivers: [NONE; N],
+        }
+    }
+
+    /// Registers a PHY driver.
+    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) -> Result {
+        for i in 0..N {
+            if self.drivers[i].is_none() {
+                let mut drv = Box::try_new(adapter.driver())?;
+                // SAFETY: Just an FFI call.
+                let ret = unsafe { bindings::phy_driver_register(drv.as_mut(), self.module.0) };
+                if ret != 0 {
+                    return Err(Error::from_errno(ret));
+                }
+                self.drivers[i] = Some(drv);
+                return Ok(());
+            }
+        }
+        Err(code::EINVAL)
+    }
+}
+
+impl<const N: usize> Drop for Registration<{ N }> {
+    fn drop(&mut self) {
+        for i in 0..N {
+            if let Some(mut drv) = self.drivers[i].take() {
+                unsafe {
+                    // SAFETY: Just an FFI call.
+                    bindings::phy_driver_unregister(drv.as_mut());
+                }
+            } else {
+                break;
+            }
+        }
+    }
+}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl<const N: usize> Send for Registration<{ N }> {}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl<const N: usize> Sync for Registration<{ N }> {}
-- 
2.34.1

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

[Andrew Lunn]

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

How does this scale when there are 200 subsystems using binding
helpers?

> +/// Wraps the kernel's `struct phy_device`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.

Well that is untrue. phylib passes phydev to the MAC driver, in its
adjust link callback. It can then read from the phy_device
object. There are also API calls the MAC can use to change values in
the phydev structure. The phylib core will also modify values in
phydev. In theory, the core will protect the phy_device structure
using the phydev->lock mutex, although we might be missing a few
locks/unlocks, and suspend/resume does not take these locks because of
deadlock. A phy driver however should not need to touch this lock, it
is taken before there is a call into the driver.

> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *(ptr as *mut Self) }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn id(&mut self) -> u32 {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).phy_id }
> +    }
> +
> +    /// Returns true if the PHY has no link.
> +    pub fn link(&mut self) -> bool {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).link() != 0 }

That seems an odd way to express it.

unsigned link:1;

It is a bitfield of 1 bit. So why not simply == 1 ? Or even just plain
(*phydev).link() ?

> +    /// Gets the state of the PHY.
> +    pub fn state(&mut self) -> DeviceState {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        let state = unsafe { (*phydev).state };
> +        match state {
> +            0 => DeviceState::Down,
> +            1 => DeviceState::Ready,
> +            2 => DeviceState::Halted,
> +            3 => DeviceState::Error,
> +            4 => DeviceState::Up,
> +            5 => DeviceState::Running,
> +            6 => DeviceState::NoLink,
> +            7 => DeviceState::CableTest,

Can you directly reference the enum values phy_state? Otherwise this
is error prone.

> +            _ => DeviceState::Unknown,

This should not happen, and if it does something bad is going
on. WARN_ONCE() so we know about it.

> +    /// Sets duplex mode.
> +    pub fn set_duplex(&mut self, is_full: bool) {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe {
> +            if is_full {
> +                (*phydev).duplex = bindings::DUPLEX_FULL as i32;
> +            } else {
> +                (*phydev).duplex = bindings::DUPLEX_HALF as i32;
> +            }
> +        }
> +    }

How is DUPLEX_UNKNOWN handled? Duplex is not boolean. If we don't have
link duplex is not known yet, so should be set to DUPLEX_UNKNOWN.

> +
> +    /// Executes software reset the PHY via BMCR_RESET bit.
> +    pub fn soft_reset(&mut self) -> Result {

I suggest you keep to the genphy_ naming if possible, to make it clear
this is using a generic PHY method.

> +    /// Reads a given PHY register.
> +    pub fn read(&mut self, regnum: u32) -> Result<i32> {

I suggest you get hold of a copy of IEEE 802.3. It is free to
download. Check out clause 22 and clause 45.

There are two registers spaces in PHYs, defined in clause 22 and
clause 45. These are known as C22 and C45. C22 states there are 32
registers which are u16. C45 has 32 x 32 registers, which are u16. The
core of phylib is a bit messy, but phy drivers have a clean API. To
access a C22 register phy_read()/phy_write() it used. To access a C45
register phy_read_mmd()/phy_write_mmd() is used.

I would suggest the comment becomes "Reads a given C22 PHY register".

> +    /// Writes a given PHY register.

Writes a given C22 PHY register.

> +    /// Reads a given PHY register without the MDIO bus lock taken.
> +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret =
> +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret)
> +        }
> +    }

Now we are getting into the hairy stuff. Locking is
interesting. Rather than duplicate all the interesting stuff, can you
just use the high level C functions? In fact, the driver you posted
does not need anything other than phy_read() and phy_write(), so i
would not even bother with these yet.

> +    /// Resumes the PHY via BMCR_PDOWN bit.
> +    pub fn resume(&mut self) -> Result {

genphy_resume()

> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_resume(phydev) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Suspends the PHY via BMCR_PDOWN bit.
> +    pub fn suspend(&mut self) -> Result {

genphy_suspend()

> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_suspend(phydev) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Checks the link status and updates current link state.
> +    pub fn read_status(&mut self) -> Result {

genphy_read_status()

> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // 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(())
> +        }
> +    }
> +
> +    /// Updates the link status.
> +    pub fn update_link(&mut self) -> Result {

genphy_read_link()

> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_update_link(phydev) };
> +        if ret < 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads Link partner ability.
> +    pub fn read_lpa(&mut self) -> Result {

genphy_read_lpa() etc...

> +/// PHY is internal.
> +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
> +/// PHY needs to be reset after the refclk is enabled.
> +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
> +/// Polling is used to detect PHY status changes.
> +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
> +/// Don't suspend.
> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;

Does Rust have the concept of a bit? Anything like the BIT() macro?
We have seen bugs where developers have not realized these are bits,
and so have defined 1, 2, 3, not 1, 2, 4. We can avoid that sort of
error by using BIT(0), BIT(1), and then the next one is obviously
BIT(2), not 3.

> +/// Corresponds to functions in `struct phy_driver`.
> +#[vtable]
> +pub trait Driver {
> +    /// Corresponds to `flags` in `struct phy_driver`.
> +    const FLAGS: u32 = 0;
> +
> +    /// Corresponds to `soft_reset` in `struct phy_driver`.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }

This is beyond my limited knowledge of Rust. The PHY core assumes for
most of the ops in phy_driver it is either a valid function pointer or
NULL. We have code like:

       if (phydev->drv->soft_reset) {
                ret = phydev->drv->soft_reset(phydev);

and

        if (phydev->drv->set_loopback)
                ret = phydev->drv->set_loopback(phydev, enable);
        else
                ret = genphy_loopback(phydev, enable);

So the default cannot be a function which returns -EOPNOTSUPP. It
needs to be a NULL.

      Andrew

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

[Boqun Feng]

On Wed, Sep 13, 2023 at 10:11:57PM +0200, Andrew Lunn wrote:
> > 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>
> 
> How does this scale when there are 200 subsystems using binding
> helpers?
> 
> > +/// Wraps the kernel's `struct phy_device`.
> > +#[repr(transparent)]
> > +pub struct Device(Opaque<bindings::phy_device>);
> > +
> > +impl Device {
> > +    /// Creates a new [`Device`] instance from a raw pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > +    /// may read or write to the `phy_device` object.
> 
> Well that is untrue. phylib passes phydev to the MAC driver, in its

Note that a "# Safety" section before an unsafe function is the "safety
requirement", i.e. the things that callers must obey the requirement in
order to call these unsafe functions. Here it's supposed to  describe
"what's the safe usage of `from_raw` function".

> adjust link callback. It can then read from the phy_device
> object. There are also API calls the MAC can use to change values in
> the phydev structure. The phylib core will also modify values in
> phydev. In theory, the core will protect the phy_device structure
> using the phydev->lock mutex, although we might be missing a few
> locks/unlocks, and suspend/resume does not take these locks because of
> deadlock. A phy driver however should not need to touch this lock, it
> is taken before there is a call into the driver.
> 
[...]
> 
> > +
> > +    /// Executes software reset the PHY via BMCR_RESET bit.
> > +    pub fn soft_reset(&mut self) -> Result {
> 
> I suggest you keep to the genphy_ naming if possible, to make it clear
> this is using a generic PHY method.
> 

My opinion may not matter, but this function is actually

	net::phy::Device::soft_reset()

, so it could be a step backwards if we use a prefix in a language that
support namespace ;-) But I'll leave it the people who will read the
code day-to-day.

> > +    /// Reads a given PHY register.
> > +    pub fn read(&mut self, regnum: u32) -> Result<i32> {
[...]
> > +    /// Reads a given PHY register without the MDIO bus lock taken.
> > +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
> > +        let phydev = Opaque::get(&self.0);
> > +        // SAFETY: This object is initialized by the `from_raw` constructor,
> > +        // so an FFI call with a valid pointer.
> > +        let ret =
> > +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
> > +        if ret < 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(ret)
> > +        }
> > +    }
> 
> Now we are getting into the hairy stuff. Locking is
> interesting. Rather than duplicate all the interesting stuff, can you
> just use the high level C functions? In fact, the driver you posted
> does not need anything other than phy_read() and phy_write(), so i
> would not even bother with these yet.
> 

Agreed, these functions have no users. Also the wrappers doesn't reflect
the lock requirements, at least not in comments, after a quick look, I
suppose all the functions should be under phy_device->lock?

> > +    /// Resumes the PHY via BMCR_PDOWN bit.
> > +    pub fn resume(&mut self) -> Result {

Regards,
Boqun

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

[Andrew Lunn]

> > > +impl Device {
> > > +    /// Creates a new [`Device`] instance from a raw pointer.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > > +    /// may read or write to the `phy_device` object.
> > 
> > Well that is untrue. phylib passes phydev to the MAC driver, in its
> 
> Note that a "# Safety" section before an unsafe function is the "safety
> requirement", i.e. the things that callers must obey the requirement in
> order to call these unsafe functions.

So by lifetime, it means while it is inside this function?

> > > +    /// Executes software reset the PHY via BMCR_RESET bit.
> > > +    pub fn soft_reset(&mut self) -> Result {
> > 
> > I suggest you keep to the genphy_ naming if possible, to make it clear
> > this is using a generic PHY method.
> > 
> 
> My opinion may not matter, but this function is actually
> 
> 	net::phy::Device::soft_reset()
> 
> , so it could be a step backwards if we use a prefix in a language that
> support namespace ;-) But I'll leave it the people who will read the
> code day-to-day.

So a bit of background. IEEE 802.3 defines how a well behaved PHY
should work. We have a number of helper functions based on that
specification, all starting with the prefix genphy_. We expect PHY
drivers to use these helpers where possible. It helps reviewing driver
submissions, in that if we see genphy_foo we know it is good quality
core code. If it is not genphy_foo, we need to review it closely.

When reviewing a Rust PHY driver, i want the same hint. This is a
wrapper around a trusted genphy_foo C function, so i don't need to
look too closely at it.

> Agreed, these functions have no users. Also the wrappers doesn't reflect
> the lock requirements, at least not in comments, after a quick look, I
> suppose all the functions should be under phy_device->lock?

The core will take that locking before calling into the driver. So the
driver never should need to touch phydev->lock. It is one of those
rules of thumb:

	Driver writers often don't understand locking, so try to
	ensure the core does all the locking, not the driver.

     Andrew

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

[Boqun Feng]

On Wed, Sep 13, 2023 at 11:05:31PM +0200, Andrew Lunn wrote:
> > > > +impl Device {
> > > > +    /// Creates a new [`Device`] instance from a raw pointer.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > > > +    /// may read or write to the `phy_device` object.
> > > 
> > > Well that is untrue. phylib passes phydev to the MAC driver, in its
> > 
> > Note that a "# Safety" section before an unsafe function is the "safety
> > requirement", i.e. the things that callers must obey the requirement in
> > order to call these unsafe functions.
> 
> So by lifetime, it means while it is inside this function?
> 

No ;-) Let's take a look at the function:

	pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
	    unsafe { &mut *(ptr as *mut Self) }
	}

"'a" is a lifetime notation in Rust, so usually we call it

	lifetime 'a

now, you can see the function returns a "&'a mut Self", (Self is Device
here), so it's: a mutable reference to Device whose lifetime is 'a,
until now 'a is still a lifetime variable. When the function is called,
you can think as a concrete lifetime is bound to that variable, for
example:

	{
		// create a mutable reference whose lifetime is '1
		// '1 is a concrete lifetime starts at here.
		let dev: &'1 mut Device = unsafe { Device::from_raw(..) };

		// '1 continues here
		<use dev as a mutable reference to Device>
		// '1 continues here
	} // "dev" is out of scope, so '1 ends here.

in above case, '1 is bound to 'a. A concrete lifetime can usually be
thought of as a region in the code.

Back to the safety requirement, it basically says the caller should
guarantee that as long as the return mutable reference is alive, the
underlying object (phy_device in memory) should be exclusively
accessable. For example, in the above case, it's the caller's
responsiblitly to guarantee "dev" is always a valid reference (during
lifetime '1).
			

Others may explain better than me, but the above is the conceptual model
that I use, hope it helps.


> > > > +    /// Executes software reset the PHY via BMCR_RESET bit.
> > > > +    pub fn soft_reset(&mut self) -> Result {
> > > 
> > > I suggest you keep to the genphy_ naming if possible, to make it clear
> > > this is using a generic PHY method.
> > > 
> > 
> > My opinion may not matter, but this function is actually
> > 
> > 	net::phy::Device::soft_reset()
> > 
> > , so it could be a step backwards if we use a prefix in a language that
> > support namespace ;-) But I'll leave it the people who will read the
> > code day-to-day.
> 
> So a bit of background. IEEE 802.3 defines how a well behaved PHY
> should work. We have a number of helper functions based on that
> specification, all starting with the prefix genphy_. We expect PHY
> drivers to use these helpers where possible. It helps reviewing driver
> submissions, in that if we see genphy_foo we know it is good quality
> core code. If it is not genphy_foo, we need to review it closely.
> 
> When reviewing a Rust PHY driver, i want the same hint. This is a
> wrapper around a trusted genphy_foo C function, so i don't need to
> look too closely at it.
> 

Understood ;-)

> > Agreed, these functions have no users. Also the wrappers doesn't reflect
> > the lock requirements, at least not in comments, after a quick look, I
> > suppose all the functions should be under phy_device->lock?
> 
> The core will take that locking before calling into the driver. So the
> driver never should need to touch phydev->lock. It is one of those
> rules of thumb:
> 
> 	Driver writers often don't understand locking, so try to
> 	ensure the core does all the locking, not the driver.

Make sense. I brought that up because I noticed that Fujita used all
"&mut self" (i.e. mutable references) for all the functions, and mutable
references mean exclusive accesses, which can usually be thought as
"must be called with locked held" (if the underlying object is avaiable
for multipl threads).

Regards,
Boqun

> 
>      Andrew

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

[Trevor Gross]

On Wed, Sep 13, 2023 at 11:05:31PM +0200, Andrew Lunn wrote:
> > [...]
> >
> > So by lifetime, it means while it is inside this function?
> >

Boqun covered this example quite well, here are some more general
crash course notes if it helps -

Lifetime is just rust lingo for compiler-tracked metadata on a
pointer, pointer + lifetime = reference (`&`). An example

    struct Foo<'a> {
        bar: &'a u32
    }

Read that as:
- a `Foo` will exist and be valid for some amount of time (of course)
- call the time it exists for `'a` [^1]
- `bar` is a pointer to something
- `bar` has lifetime `'a`. Which means, whatever `bar` points to must
  be valid for at least `'a`, i.e, at least as long as `Foo` is valid

Same rules you have to follow in any language, having these
annotations just tells the compiler about it. Then it keeps track and
makes sure they get followed. Functions give similar information:

    /// Return the first substring of `haystack` that starts with `needle`
    fn strstr<'a, 'b>(haystack: &'a str, needle: &'b str) -> &'a str;

The return value and `haystack` have the same lifetime, so you know
you can't `destroy` haystack as long as the return value is in use.
`needle` has a different lifetime so it's clear that it is unrelated.
In the original function

>> +    /// # Safety
>> +    ///
>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> +    /// may read or write to the `phy_device` object.
>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self

A compiler-tracked lifetime is conjured from nothing, so the caller
gets to decide what that is (usually implicitly but can be specified).
This could be anything, hence "unsafe" and instructions on how to make
it correct. Once created, the compiler will track the above rules.

The other part of the picture is that rust strictly enforces one
`&mut` reference XOR any number `&` (const) references to a piece of
data at a time [^2]. The exception to this is of course things that
safely allow access behind a non-exclusive reference -

> In theory, the core will protect the phy_device structure
> using the phydev->lock mutex, although we might be missing a few [...]

- such as mutexes :). [^3] The original comment could probably add
something like "with the exception of mutex-protected fields" or a
mention about the callbacks.

Bit awkward at first, but it winds up being very nice to kick the
mental pointer-validity-chasing load to the compiler.

[^1]: could be called `'foo` `'buffer` or anything else,
      `'a' `'b` ... are common for simple things
[^2]: at a lower level `&mut` is basically C's `restrict`. Easier
      to use when the compiler checks it.
[^3]: these types use `UnsafeCell` to tell the compiler
      they allow this and not to emit `restrict`. `Opaque`
      in bindings also does this.

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

[Miguel Ojeda]

Hi Andrew,

Answering a few of the bits here not already discussed -- and, by the
way, thanks a lot for taking a look at these early RFCs and making the
effort on reading Rust code.

On Wed, Sep 13, 2023 at 10:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> How does this scale when there are 200 subsystems using binding
> helpers?

This should be solved with the new build system. The plan is to have
bindings per subsystem (in your own folder etc.), as well as the
abstractions etc.

> > +            _ => DeviceState::Unknown,
>
> This should not happen, and if it does something bad is going
> on. WARN_ONCE() so we know about it.

An alternative could be to return an error instead. That way having a
`DeviceState` already guarantees one has a valid state.

> Does Rust have the concept of a bit? Anything like the BIT() macro?
> We have seen bugs where developers have not realized these are bits,
> and so have defined 1, 2, 3, not 1, 2, 4. We can avoid that sort of
> error by using BIT(0), BIT(1), and then the next one is obviously
> BIT(2), not 3.

We have some bit utils around, but not upstreamed yet, yeah, e.g.
things like https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/types.rs#L312

Cheers,
Miguel

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

[Andrew Lunn]

> > > +            _ => DeviceState::Unknown,
> >
> > This should not happen, and if it does something bad is going
> > on. WARN_ONCE() so we know about it.
> 
> An alternative could be to return an error instead. That way having a
> `DeviceState` already guarantees one has a valid state.

If this happens, it is because the C and Rust code has diverged. We
want it to be loud and obvious when this happens, so a WARN_ON() and a
stack trace is what i would recommend. Or better still, some way to
trigger a BUILD_BUG_ON() at build time.

Hum, does the kernel Opps code understand Rust symbols, and can unwind
a call stack which is a mix of C and Rust functions?

> > Does Rust have the concept of a bit? Anything like the BIT() macro?
> > We have seen bugs where developers have not realized these are bits,
> > and so have defined 1, 2, 3, not 1, 2, 4. We can avoid that sort of
> > error by using BIT(0), BIT(1), and then the next one is obviously
> > BIT(2), not 3.
> 
> We have some bit utils around, but not upstreamed yet, yeah, e.g.
> things like https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/types.rs#L312

let y: u64 = bit(34).into();

Seems a bit verbose, compared to the CPP macro BIT(). But lets see it
in action.

	Andrew

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

[Miguel Ojeda]

On Thu, Sep 14, 2023 at 2:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> If this happens, it is because the C and Rust code has diverged. We
> want it to be loud and obvious when this happens, so a WARN_ON() and a
> stack trace is what i would recommend. Or better still, some way to
> trigger a BUILD_BUG_ON() at build time.

If it cannot ever happen, then definitely we should do that (and keep
the function `-> DeviceState`), of course. Otherwise, a Rust panic
could also be justifiable in that case (if we are really sure it
cannot happen), or even assuming it and using `unsafe` (if needed for
performance/size).

What I meant to say is that, in general, it is a good idea to make
invalid states unrepresentable, i.e. to have a `DeviceState` without
`Unknown` (and to achieve that, the function can reach for one of the
alternatives above). That way, when we pass around a `DeviceState`, we
are guaranteed it has one of the expected values.

(i.e. if I understood you correctly, you wanted to have `WARN_ONCE()`
without changing the return type, which would mean keeping the
`Unknown` case)

> Hum, does the kernel Opps code understand Rust symbols, and can unwind
> a call stack which is a mix of C and Rust functions?

Do you mean for printing the backtrace in `WARN_ONCE()`? (you get
Rust-mangled symbols)

> let y: u64 = bit(34).into();
>
> Seems a bit verbose, compared to the CPP macro BIT(). But lets see it
> in action.

Yeah, that one is a type, but we could have a function or a `bit!`
macro for getting a raw integer,

Cheers,
Miguel

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

[Andrew Lunn]

On Thu, Sep 14, 2023 at 01:03:06PM +0200, Miguel Ojeda wrote:
> On Thu, Sep 14, 2023 at 2:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > If this happens, it is because the C and Rust code has diverged. We
> > want it to be loud and obvious when this happens, so a WARN_ON() and a
> > stack trace is what i would recommend. Or better still, some way to
> > trigger a BUILD_BUG_ON() at build time.
> 
> If it cannot ever happen, then definitely we should do that (and keep
> the function `-> DeviceState`), of course. Otherwise, a Rust panic
> could also be justifiable in that case (if we are really sure it
> cannot happen), or even assuming it and using `unsafe` (if needed for
> performance/size).

These are states of a state machine. If it is not one of the defined
states, the state machine has gone off the rails.

It is actually unlikely to happen, so if it does, my first guess for
debugging it would be that the C and Rust implementation have
diverged. A state has been added or removed in one implementation, but
not the other.

This is why i asked if it is possible to directly use the enum in the
Rust code. If a state is removed from the enum and the C code, but not
this method, we will fail at build time, which is a lot more obvious
than at run time. This seems like a useful tool for anybody trying to
maintain a parallel C and Rust implementation of something.

Also, nothing much in the PHY subsystem is performance critical. We
spend a lot of time twiddling our thumbs waiting for the slow MDIO bus
to perform transactions with the PHY. So i would always lean towards
correctness and self problem detection over performance.

> What I meant to say is that, in general, it is a good idea to make
> invalid states unrepresentable, i.e. to have a `DeviceState` without
> `Unknown` (and to achieve that, the function can reach for one of the
> alternatives above). That way, when we pass around a `DeviceState`, we
> are guaranteed it has one of the expected values.
> 
> (i.e. if I understood you correctly, you wanted to have `WARN_ONCE()`
> without changing the return type, which would mean keeping the
> `Unknown` case)

The state is probably valid, just the Rust implementation has no idea
of it, because the Rust code is out of date. So returning it seems
like the correct thing to do. A Rust user of it is probably going to
do the wrong thing, so a WARN_ONCE() seems justified, but if it is
passed to C code, it will probably be O.K, since it is valid in the C
world.

	Andrew

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

[FUJITA Tomonori]

Hi,

On Thu, 14 Sep 2023 13:03:06 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Sep 14, 2023 at 2:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> If this happens, it is because the C and Rust code has diverged. We
>> want it to be loud and obvious when this happens, so a WARN_ON() and a
>> stack trace is what i would recommend. Or better still, some way to
>> trigger a BUILD_BUG_ON() at build time.
> 
> If it cannot ever happen, then definitely we should do that (and keep
> the function `-> DeviceState`), of course. Otherwise, a Rust panic
> could also be justifiable in that case (if we are really sure it
> cannot happen), or even assuming it and using `unsafe` (if needed for
> performance/size).
> 
> What I meant to say is that, in general, it is a good idea to make
> invalid states unrepresentable, i.e. to have a `DeviceState` without
> `Unknown` (and to achieve that, the function can reach for one of the
> alternatives above). That way, when we pass around a `DeviceState`, we
> are guaranteed it has one of the expected values.
> 
> (i.e. if I understood you correctly, you wanted to have `WARN_ONCE()`
> without changing the return type, which would mean keeping the
> `Unknown` case)
> 
>> Hum, does the kernel Opps code understand Rust symbols, and can unwind
>> a call stack which is a mix of C and Rust functions?
> 
> Do you mean for printing the backtrace in `WARN_ONCE()`? (you get
> Rust-mangled symbols)

To avoid this enum issues, can we convert C enum to Rust enum?

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

[FUJITA Tomonori]

Hi,

On Wed, 13 Sep 2023 22:11:57 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        unsafe { &mut *(ptr as *mut Self) }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn id(&mut self) -> u32 {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> +        unsafe { (*phydev).phy_id }
>> +    }
>> +
>> +    /// Returns true if the PHY has no link.
>> +    pub fn link(&mut self) -> bool {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> +        unsafe { (*phydev).link() != 0 }
> 
> That seems an odd way to express it.
> 
> unsigned link:1;
> 
> It is a bitfield of 1 bit. So why not simply == 1 ? Or even just plain
> (*phydev).link() ?

The accessors is generated for bitfield. In this case:

pub fn link(&self) -> core::ffi::c_uint;
pub fn set_link(&mut self, val: core::ffi::c_uint);

a bit weird for 1 bit though.

The function name should be something is_linkup()?


>> +    /// Sets duplex mode.
>> +    pub fn set_duplex(&mut self, is_full: bool) {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> +        unsafe {
>> +            if is_full {
>> +                (*phydev).duplex = bindings::DUPLEX_FULL as i32;
>> +            } else {
>> +                (*phydev).duplex = bindings::DUPLEX_HALF as i32;
>> +            }
>> +        }
>> +    }
> 
> How is DUPLEX_UNKNOWN handled? Duplex is not boolean. If we don't have
> link duplex is not known yet, so should be set to DUPLEX_UNKNOWN.

Understood. I'll fix.

>> +
>> +    /// Executes software reset the PHY via BMCR_RESET bit.
>> +    pub fn soft_reset(&mut self) -> Result {
> 
> I suggest you keep to the genphy_ naming if possible, to make it clear
> this is using a generic PHY method.

I'll keep the genphy_ naming for all the genphy_ functions.


>> +    /// Reads a given PHY register.
>> +    pub fn read(&mut self, regnum: u32) -> Result<i32> {
> 
> I suggest you get hold of a copy of IEEE 802.3. It is free to
> download. Check out clause 22 and clause 45.

I've just started to learn.


> There are two registers spaces in PHYs, defined in clause 22 and
> clause 45. These are known as C22 and C45. C22 states there are 32
> registers which are u16. C45 has 32 x 32 registers, which are u16. The
> core of phylib is a bit messy, but phy drivers have a clean API. To
> access a C22 register phy_read()/phy_write() it used. To access a C45
> register phy_read_mmd()/phy_write_mmd() is used.
> 
> I would suggest the comment becomes "Reads a given C22 PHY register".
> 
>> +    /// Writes a given PHY register.
> 
> Writes a given C22 PHY register.

Will fix.

>> +    /// Reads a given PHY register without the MDIO bus lock taken.
>> +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor,
>> +        // so an FFI call with a valid pointer.
>> +        let ret =
>> +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret)
>> +        }
>> +    }
> 
> Now we are getting into the hairy stuff. Locking is
> interesting. Rather than duplicate all the interesting stuff, can you
> just use the high level C functions? In fact, the driver you posted
> does not need anything other than phy_read() and phy_write(), so i
> would not even bother with these yet.

Yeah, I can't figure out a clean way to handle this.

The realtek driver uses them:

https://github.com/fujita/rust-realtek-phy/blob/master/rust_realtek.rs#L33

It implements .read_page and .write_page so read and write without
locking the MDIO bus are necessary? Can we do in differently?


>> +/// PHY is internal.
>> +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
>> +/// PHY needs to be reset after the refclk is enabled.
>> +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
>> +/// Polling is used to detect PHY status changes.
>> +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
>> +/// Don't suspend.
>> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
> 
> Does Rust have the concept of a bit? Anything like the BIT() macro?
> We have seen bugs where developers have not realized these are bits,
> and so have defined 1, 2, 3, not 1, 2, 4. We can avoid that sort of
> error by using BIT(0), BIT(1), and then the next one is obviously
> BIT(2), not 3.

I'll try bit! macro in the next round.


>> +/// Corresponds to functions in `struct phy_driver`.
>> +#[vtable]
>> +pub trait Driver {
>> +    /// Corresponds to `flags` in `struct phy_driver`.
>> +    const FLAGS: u32 = 0;
>> +
>> +    /// Corresponds to `soft_reset` in `struct phy_driver`.
>> +    fn soft_reset(_dev: &mut Device) -> Result {
>> +        Err(code::ENOTSUPP)
>> +    }
> 
> This is beyond my limited knowledge of Rust. The PHY core assumes for
> most of the ops in phy_driver it is either a valid function pointer or
> NULL. We have code like:
> 
>        if (phydev->drv->soft_reset) {
>                 ret = phydev->drv->soft_reset(phydev);
> 
> and
> 
>         if (phydev->drv->set_loopback)
>                 ret = phydev->drv->set_loopback(phydev, enable);
>         else
>                 ret = genphy_loopback(phydev, enable);
> 
> So the default cannot be a function which returns -EOPNOTSUPP. It
> needs to be a NULL.

It will be a NULL if a driver doesn't implement.

This is not Rust stuff; kernel Rust specific. If abstraction code
implement a function, a driver doesn't need to implement it. If the
driver doesn't, the function pointer is set to NULL.

If abstraction code just declare a function, a driver always need to
implement that function.

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

[Andrew Lunn]

On Sun, Sep 17, 2023 at 07:17:08PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Wed, 13 Sep 2023 22:11:57 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >> +        unsafe { &mut *(ptr as *mut Self) }
> >> +    }
> >> +
> >> +    /// Gets the id of the PHY.
> >> +    pub fn id(&mut self) -> u32 {
> >> +        let phydev = Opaque::get(&self.0);
> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> +        unsafe { (*phydev).phy_id }
> >> +    }
> >> +
> >> +    /// Returns true if the PHY has no link.
> >> +    pub fn link(&mut self) -> bool {
> >> +        let phydev = Opaque::get(&self.0);
> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> +        unsafe { (*phydev).link() != 0 }
> > 
> > That seems an odd way to express it.
> > 
> > unsigned link:1;
> > 
> > It is a bitfield of 1 bit. So why not simply == 1 ? Or even just plain
> > (*phydev).link() ?
> 
> The accessors is generated for bitfield. In this case:
> 
> pub fn link(&self) -> core::ffi::c_uint;
> pub fn set_link(&mut self, val: core::ffi::c_uint);

So Rust has thrown away the size of the bitfield?

struct foo {
  unsigned long bitfield:1;
  unsigned long nibblefield:4;
};

int main(int argc, char *argv[])
{
  struct foo foo;

  foo.bitfield=2;
  foo.nibblefield=42;
}

$ gcc -Wall test.c
test.c: In function ‘main’:
test.c:10:16: warning: unsigned conversion from ‘int’ to ‘unsigned char:1’ changes value from ‘2’ to ‘0’ [-Woverflow]
   10 |   foo.bitfield=2;
      |                ^
test.c:11:19: warning: unsigned conversion from ‘int’ to ‘unsigned char:4’ changes value from ‘42’ to ‘10’ [-Woverflow]
   11 |   foo.nibblefield=42;
      |

So i also assume Rust is not going to do these range checks? Will they
get done a runtime when the setter is called?

> a bit weird for 1 bit though.
> 
> The function name should be something is_linkup()?

Naming is hard. At least for the moment, anybody writing a PHY driver
is going to be using a C driver as a template. They will see code
like:

msc.c:	if (!phydev->link && priv && priv->edpd_enable &&
teranetics.c:	phydev->link = 1;
teranetics.c:			phydev->link = 0;
teranetics.c:			phydev->link = 0;
uPD60620.c:	phydev->link = 0;
uPD60620.c:			phydev->link = 1;

Since it appears your must have a getter/setter, i would keep with
get_link() and set_link().

> > Now we are getting into the hairy stuff. Locking is
> > interesting. Rather than duplicate all the interesting stuff, can you
> > just use the high level C functions? In fact, the driver you posted
> > does not need anything other than phy_read() and phy_write(), so i
> > would not even bother with these yet.
> 
> Yeah, I can't figure out a clean way to handle this.
> 
> The realtek driver uses them:
> 
> https://github.com/fujita/rust-realtek-phy/blob/master/rust_realtek.rs#L33
> 
> It implements .read_page and .write_page so read and write without
> locking the MDIO bus are necessary? Can we do in differently?

This locking is not nice, but it is mostly hidden away in the phylib
core. We probably don't want to duplicate it in Rust. So i would
suggest you wrap the high level API, phy_read_paged(),
phy_write_paged(), phy_modify_paged_changed().

rtl8211e_config_init() does:

        oldpage = phy_select_page(phydev, 0x7);
        if (oldpage < 0)
                goto err_restore_page;

        ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
        if (ret)
                goto err_restore_page;

        ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
                           | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
                           val);

err_restore_page:
        return phy_restore_page(phydev, oldpage, ret);

This should not be time critical, so i would turn this into
phy_read_paged() followed by phy_modify_paged().

rtlgen_read_mmd() is different.

phy_read_mmd() takes the MDIO bus lock and then calls
phydev->drv->read_mmd which is rtlgen_read_mmd(). This is why
__phy_read() is used. So you are going to need to wrap __phy_read()
and __phy_write().

Or we need to take a step back and take a long hard look at the
locking and try to clean it up and simplify it. It might then become
possible to duplicate the locking model in Rust.

> If abstraction code just declare a function, a driver always need to
> implement that function.

Ah, that is not going to scale well. There is something like 250
struct phy_driver at the moment. We keep adding new ops to that
structure. I really don't want to have to modify all 250 of those
structures when i add a new op which only one or two drivers need.

	   Andrew

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

[Trevor Gross]

On Sun, Sep 17, 2023 at 11:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
> [...]
> > If abstraction code just declare a function, a driver always need to
> > implement that function.
>
> Ah, that is not going to scale well. There is something like 250
> struct phy_driver at the moment. We keep adding new ops to that
> structure. I really don't want to have to modify all 250 of those
> structures when i add a new op which only one or two drivers need.

This concern is about adding a function pointer field to the C struct
without needing to update all future Rust drivers, correct? If so,
that will be fine currently. This comes from where the driver is
created

> +    fn driver(&self) -> bindings::phy_driver {
> +        bindings::phy_driver {
> +            name: self.name.as_char_ptr() as *mut i8,
> +            flags: <T>::FLAGS,
> +            soft_reset: if <T>::HAS_SOFT_RESET {
> +                Some(Self::soft_reset_callback)
> +            } else {
> +                None
> +            },
> +            // other fields ...
> +            ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> +        }

The `..` covers all unspecified fields and just says they will be
zeroed. How this all works is:

- A trait is created (`Driver`) that has one function for each function
  pointer field in a C vtable struct (`phy_driver`).
- Each function in the trait can either have no implementation, (every
  implementer needs to provide one), or a default implementation, (an
implementer can
  optionally provide one or fall back to the default). This is just
standard Rust, and
  I think that is what Fujita was saying
- What the `#[vtable]` macro does is give you a way to check if a
  implementer/driver provides their own implementation of the default
method. It does this by
  setting `HAS_FN_NAME`
- Then, when creating the c vtable struct from a type that implements the
  trait, you check that value for each field and either set it to null or to a
  function pointer

This means that the `phy_driver` that C sees has null for all
unimplemented fields and not the default implementation. In the above
snip creating the `phy_driver`, it checks whether or not the driver
provides a `soft_reset` implementation with `HAS_SOFT_RESET`, and then
either sets that field to a null pointer `None` or a function pointer
`Some(Self::soft_reset_callback)`.

I think that technically the default implementations in the trait
could be `unimplemented!()` which calls back to `BUG()`, since their
use would indicate something incorrect with the abstraction like an
unhandled field. within this small subset of code.

Does any of that clear up concerns about providing nulls or adding new fields?

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

[Andrew Lunn]

On Sun, Sep 17, 2023 at 02:42:02PM -0400, Trevor Gross wrote:
> On Sun, Sep 17, 2023 at 11:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > [...]
> > > If abstraction code just declare a function, a driver always need to
> > > implement that function.
> >
> > Ah, that is not going to scale well. There is something like 250
> > struct phy_driver at the moment. We keep adding new ops to that
> > structure. I really don't want to have to modify all 250 of those
> > structures when i add a new op which only one or two drivers need.
> 
> This concern is about adding a function pointer field to the C struct
> without needing to update all future Rust drivers, correct?

Partially. Assuming Rust is successful, and in 10 years time we have
250 devices supported by Rust, i also don't want to have to edit all
250 entries when a new member is added to the Rust version of struct
phy_driver.

> If so,
> that will be fine currently. This comes from where the driver is
> created
> 
> > +    fn driver(&self) -> bindings::phy_driver {
> > +        bindings::phy_driver {
> > +            name: self.name.as_char_ptr() as *mut i8,
> > +            flags: <T>::FLAGS,
> > +            soft_reset: if <T>::HAS_SOFT_RESET {
> > +                Some(Self::soft_reset_callback)
> > +            } else {
> > +                None
> > +            },
> > +            // other fields ...
> > +            ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> > +        }
> 
> The `..` covers all unspecified fields and just says they will be
> zeroed. How this all works is:
> 
> - A trait is created (`Driver`) that has one function for each function
>   pointer field in a C vtable struct (`phy_driver`).
> - Each function in the trait can either have no implementation, (every
>   implementer needs to provide one), or a default implementation, (an
> implementer can
>   optionally provide one or fall back to the default). This is just
> standard Rust, and
>   I think that is what Fujita was saying
> - What the `#[vtable]` macro does is give you a way to check if a
>   implementer/driver provides their own implementation of the default
> method. It does this by
>   setting `HAS_FN_NAME`
> - Then, when creating the c vtable struct from a type that implements the
>   trait, you check that value for each field and either set it to null or to a
>   function pointer
> 
> This means that the `phy_driver` that C sees has null for all
> unimplemented fields and not the default implementation. In the above
> snip creating the `phy_driver`, it checks whether or not the driver
> provides a `soft_reset` implementation with `HAS_SOFT_RESET`, and then
> either sets that field to a null pointer `None` or a function pointer
> `Some(Self::soft_reset_callback)`.

O.K, that is the behaviour we want, in terms of NULL, or a driver
specific function pointer.

A follow up question to this. This currently does not apply to PHY
drivers, but the concept of a structure of function pointers and maybe
a few other data fields exists all over the kernel. There has been an
effort to make them all const, so the C compiler puts them in the
.rodata section. The kernel then uses the MMU to protect it, so it
really is read only. That protects from a class to attacks where a
function pointer gets overwritten and then jumped through.

Can Rust create these structures read only in the .rodata section? Can
this driver actually do this, which would be an improvement over the
current C PHY drivers?

	Andrew

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

[Trevor Gross]

On Sun, Sep 17, 2023 at 3:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Sep 17, 2023 at 02:42:02PM -0400, Trevor Gross wrote:
> > On Sun, Sep 17, 2023 at 11:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > [...]
> > > > If abstraction code just declare a function, a driver always need to
> > > > implement that function.
> > >
> > > Ah, that is not going to scale well. There is something like 250
> > > struct phy_driver at the moment. We keep adding new ops to that
> > > structure. I really don't want to have to modify all 250 of those
> > > structures when i add a new op which only one or two drivers need.
> >
> > This concern is about adding a function pointer field to the C struct
> > without needing to update all future Rust drivers, correct?
>
> Partially. Assuming Rust is successful, and in 10 years time we have
> 250 devices supported by Rust, i also don't want to have to edit all
> 250 entries when a new member is added to the Rust version of struct
> phy_driver.
> [...]
> O.K, that is the behaviour we want, in terms of NULL, or a driver
> specific function pointer.

Currently if you add a field to the struct in C, everything in Rust
will continue to work without changes. If you want to be able to use
it in the Rust side, you need to update both the `Driver` trait and
`fn driver(&self) -> bindings::phy_driver` [1] but none of the
implementations.

> A follow up question to this. This currently does not apply to PHY
> drivers, but the concept of a structure of function pointers and maybe
> a few other data fields exists all over the kernel. There has been an
> effort to make them all const, so the C compiler puts them in the
> .rodata section. The kernel then uses the MMU to protect it, so it
> really is read only. That protects from a class to attacks where a
> function pointer gets overwritten and then jumped through.
>
> Can Rust create these structures read only in the .rodata section? Can
> this driver actually do this, which would be an improvement over the
> current C PHY drivers?
>
>         Andrew

Generally, this should be possible. Any function marked `const` can be
assigned to a static:

    const fn make_device() -> Device {
        todo!()
    }

    #[no_mangle]
    static _some_driver: Device = make_device();

That function can only call other const functions, but I don't think
that is a problem here. A caveat is that in a simple test I'm trying,
anything that contains a function pointer seems to go into `.data`
instead of `.rodata`. I'm trying to figure out why this but will have
to get back about it.

Is that something you would be interested in for the `phy_driver`? I
may be able to try it out if so, unless Fujita would like to.
`phy_driver_register` doesn't seem to take a pointer to const data,
but I assume it doesn't do any modification?

[1]: Updating `Driver` but not the mapping function would mean that
the default function in `Driver` may get called, but this would
generally be incorrect. That is why I suggested making them default to
something that `BUG()`s, but I'm also not sure what policy is on this.

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

[Andrew Lunn]

> That function can only call other const functions, but I don't think
> that is a problem here. A caveat is that in a simple test I'm trying,
> anything that contains a function pointer seems to go into `.data`
> instead of `.rodata`. I'm trying to figure out why this but will have
> to get back about it.
> 
> Is that something you would be interested in for the `phy_driver`?

I was wrong here. As you noticed, they are currently not const. I just
tried to actually make them cost, and it does not work. Embedded in
struct phy_driver, is struct mdio_driver_common. That needs to be
read/write. We have a bit of history here. Originally in Linux MDIO
busses could only support PHY devices. But as things evolved, it
became clear other things could be placed on an MDIO bus, such as an
Ethernet switch. So we had to do a bit of refactoring, and ended up
with this structure. It is probably possible to do some additional
refactoring to make struct phy_driver const, but at the moment, i
don't plan on doing it.

What we do however have in PHY drivers is the table:

static struct mdio_device_id __maybe_unused asix_tbl[] = {
        { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772A) },
        { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772C) },
        { PHY_ID_ASIX_AX88796B, 0xfffffff0 },
        { }
};

This could be const, and two C drivers do mark it const, including the
realtek driver which is being re-written in Rust. This table is not
security relevant, it does not have any function pointers in it, so
its not the best example.

Even if not relevant for PHY drivers, getting const structures into
.rodata is something Kees Cook is going to want to have.

	Andrew

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

[Trevor Gross]

On Sun, Sep 17, 2023 at 9:18 PM Andrew Lunn <andrew@lunn.ch> wrote:
> I was wrong here. As you noticed, they are currently not const. I just
> tried to actually make them cost, and it does not work. Embedded in
> struct phy_driver, is struct mdio_driver_common. That needs to be
> read/write. We have a bit of history here. Originally in Linux MDIO
> busses could only support PHY devices. But as things evolved, it
> became clear other things could be placed on an MDIO bus, such as an
> Ethernet switch. So we had to do a bit of refactoring, and ended up
> with this structure. It is probably possible to do some additional
> refactoring to make struct phy_driver const, but at the moment, i
> don't plan on doing it.

Thanks for verifying so fast.

> What we do however have in PHY drivers is the table:
>
> static struct mdio_device_id __maybe_unused asix_tbl[] = {
>         { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772A) },
>         { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772C) },
>         { PHY_ID_ASIX_AX88796B, 0xfffffff0 },
>         { }
> };
>
> This could be const, and two C drivers do mark it const, including the
> realtek driver which is being re-written in Rust. This table is not
> security relevant, it does not have any function pointers in it, so
> its not the best example.
>
> Even if not relevant for PHY drivers, getting const structures into
> .rodata is something Kees Cook is going to want to have.
>
>         Andrew

I believe that table should live in rodata with the current
implementation (haven't actually verified). It definitely makes sense
to put as much as possible there, we can make this work.

Is there always a 1:1:1 phy_driver:phy_id:phy_id_mask relationship? I
think we may be able to get away with one macro on the Rust side that
registers both the `mdio_device_id` and the `phy_driver`. This macro
could also do a compile time check that all devices are ordered
correctly, if that is a common mistake (as you mentioned in a separate
thread).

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

[FUJITA Tomonori]

Hi,

On Sun, 17 Sep 2023 22:22:04 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sun, Sep 17, 2023 at 9:18 PM Andrew Lunn <andrew@lunn.ch> wrote:
>> I was wrong here. As you noticed, they are currently not const. I just
>> tried to actually make them cost, and it does not work. Embedded in
>> struct phy_driver, is struct mdio_driver_common. That needs to be
>> read/write. We have a bit of history here. Originally in Linux MDIO
>> busses could only support PHY devices. But as things evolved, it
>> became clear other things could be placed on an MDIO bus, such as an
>> Ethernet switch. So we had to do a bit of refactoring, and ended up
>> with this structure. It is probably possible to do some additional
>> refactoring to make struct phy_driver const, but at the moment, i
>> don't plan on doing it.
> 
> Thanks for verifying so fast.
> 
>> What we do however have in PHY drivers is the table:
>>
>> static struct mdio_device_id __maybe_unused asix_tbl[] = {
>>         { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772A) },
>>         { PHY_ID_MATCH_EXACT(PHY_ID_ASIX_AX88772C) },
>>         { PHY_ID_ASIX_AX88796B, 0xfffffff0 },
>>         { }
>> };
>>
>> This could be const, and two C drivers do mark it const, including the
>> realtek driver which is being re-written in Rust. This table is not
>> security relevant, it does not have any function pointers in it, so
>> its not the best example.
>>
>> Even if not relevant for PHY drivers, getting const structures into
>> .rodata is something Kees Cook is going to want to have.
>>
>>         Andrew
> 
> I believe that table should live in rodata with the current
> implementation (haven't actually verified). It definitely makes sense
> to put as much as possible there, we can make this work.

It is placed at .modinfo section by the module tools. You can find
"mdio:" string:

:~/git/rust-asix-phy$ readelf -x .modinfo rust_ax88796b.ko

Hex dump of section '.modinfo':
  0x00000000 61757468 6f723d52 75737420 666f7220 author=Rust for
  0x00000010 4c696e75 7820436f 6e747269 6275746f Linux Contributo
  0x00000020 72730064 65736372 69707469 6f6e3d52 rs.description=R
  0x00000030 75737420 41736978 20504859 73206472 ust Asix PHYs dr
  0x00000040 69766572 006c6963 656e7365 3d47504c iver.license=GPL
  0x00000050 00766572 6d616769 633d362e 362e302d .vermagic=6.6.0-
  0x00000060 7263312b 20534d50 20707265 656d7074 rc1+ SMP preempt
  0x00000070 206d6f64 5f756e6c 6f616420 006e616d  mod_unload .nam
  0x00000080 653d7275 73745f61 78383837 39366200 e=rust_ax88796b.
  0x00000090 64657065 6e64733d 00616c69 61733d6d depends=.alias=m
  0x000000a0 64696f3a 30303030 30303030 30303131 dio:000000000011
  0x000000b0 31303131 30303031 31303030 30313130 1011000110000110
  0x000000c0 30303031 00                         0001.


> Is there always a 1:1:1 phy_driver:phy_id:phy_id_mask relationship? I
> think we may be able to get away with one macro on the Rust side that
> registers both the `mdio_device_id` and the `phy_driver`. This macro
> could also do a compile time check that all devices are ordered
> correctly, if that is a common mistake (as you mentioned in a separate
> thread).

Seems currently, no. After investigating the existing PHY modules, in
most of them (41/52), one driver has one entry in mdio_device_id.

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

[Andrew Lunn]

> It is placed at .modinfo section by the module tools. You can find
> "mdio:" string:

Ah, good point. It probably is not even loaded into memory. It is
there for depmod, to build the table /lib/modules/*/modules.alias.

> > Is there always a 1:1:1 phy_driver:phy_id:phy_id_mask relationship?

No.

In theory, one entry could cover a number of PHYs. Each Vendor should
have one or more OUIs. The ID used here is a mangled version of that
OUI in the high order bits. So you could in theory just have one entry
with a mask value for just the OUI and the driver will get loaded for
all the vendors PHYs. But that is not very flexible, why keep to
register compatibility when you can change everything and write a new
driver. At least some vendors seem to think that way :-(

	Andrew

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

[FUJITA Tomonori]

Hi,

On Sun, 17 Sep 2023 17:06:35 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Sep 17, 2023 at 07:17:08PM +0900, FUJITA Tomonori wrote:
>> Hi,
>> 
>> On Wed, 13 Sep 2023 22:11:57 +0200
>> Andrew Lunn <andrew@lunn.ch> wrote:
>> 
>> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> >> +        unsafe { &mut *(ptr as *mut Self) }
>> >> +    }
>> >> +
>> >> +    /// Gets the id of the PHY.
>> >> +    pub fn id(&mut self) -> u32 {
>> >> +        let phydev = Opaque::get(&self.0);
>> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> >> +        unsafe { (*phydev).phy_id }
>> >> +    }
>> >> +
>> >> +    /// Returns true if the PHY has no link.
>> >> +    pub fn link(&mut self) -> bool {
>> >> +        let phydev = Opaque::get(&self.0);
>> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> >> +        unsafe { (*phydev).link() != 0 }
>> > 
>> > That seems an odd way to express it.
>> > 
>> > unsigned link:1;
>> > 
>> > It is a bitfield of 1 bit. So why not simply == 1 ? Or even just plain
>> > (*phydev).link() ?
>> 
>> The accessors is generated for bitfield. In this case:
>> 
>> pub fn link(&self) -> core::ffi::c_uint;
>> pub fn set_link(&mut self, val: core::ffi::c_uint);
> 
> So Rust has thrown away the size of the bitfield?

No, sorry about the confusion. The generated code to get looks like:

pub fn link(&self) -> core::ffi::c_uint {
    unsafe {::core::mem::transmute(self._bitfield_1.get(14usize, 1u8) as u32) }
}

The second argument is bit width.

pub fn set_link(&mut self, val: core::ffi::c_uint) {
    unsafe {
        let val: u32 = ::core::mem::transmute(val);
        self._bitfield_1.set(14usize, 1u8, val as u64)
    }
}

The second argument is bit width too.


>> The function name should be something is_linkup()?
> 
> Naming is hard. At least for the moment, anybody writing a PHY driver
> is going to be using a C driver as a template. They will see code
> like:
> 
> msc.c:	if (!phydev->link && priv && priv->edpd_enable &&
> teranetics.c:	phydev->link = 1;
> teranetics.c:			phydev->link = 0;
> teranetics.c:			phydev->link = 0;
> uPD60620.c:	phydev->link = 0;
> uPD60620.c:			phydev->link = 1;
> 
> Since it appears your must have a getter/setter, i would keep with
> get_link() and set_link().

I'll use these names.

>> > Now we are getting into the hairy stuff. Locking is
>> > interesting. Rather than duplicate all the interesting stuff, can you
>> > just use the high level C functions? In fact, the driver you posted
>> > does not need anything other than phy_read() and phy_write(), so i
>> > would not even bother with these yet.
>> 
>> Yeah, I can't figure out a clean way to handle this.
>> 
>> The realtek driver uses them:
>> 
>> https://github.com/fujita/rust-realtek-phy/blob/master/rust_realtek.rs#L33
>> 
>> It implements .read_page and .write_page so read and write without
>> locking the MDIO bus are necessary? Can we do in differently?
> 
> This locking is not nice, but it is mostly hidden away in the phylib
> core. We probably don't want to duplicate it in Rust. So i would
> suggest you wrap the high level API, phy_read_paged(),
> phy_write_paged(), phy_modify_paged_changed().
> 
> rtl8211e_config_init() does:
> 
>         oldpage = phy_select_page(phydev, 0x7);
>         if (oldpage < 0)
>                 goto err_restore_page;
> 
>         ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
>         if (ret)
>                 goto err_restore_page;
> 
>         ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
>                            | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
>                            val);
> 
> err_restore_page:
>         return phy_restore_page(phydev, oldpage, ret);
> 
> This should not be time critical, so i would turn this into
> phy_read_paged() followed by phy_modify_paged().
> 
> rtlgen_read_mmd() is different.
> 
> phy_read_mmd() takes the MDIO bus lock and then calls
> phydev->drv->read_mmd which is rtlgen_read_mmd(). This is why
> __phy_read() is used. So you are going to need to wrap __phy_read()
> and __phy_write().
> 
> Or we need to take a step back and take a long hard look at the
> locking and try to clean it up and simplify it. It might then become
> possible to duplicate the locking model in Rust.

Understood. I'll drop read_lockless and write_lockless in the next
round and think about how to handle realtek.


>> If abstraction code just declare a function, a driver always need to
>> implement that function.
> 
> Ah, that is not going to scale well. There is something like 250
> struct phy_driver at the moment. We keep adding new ops to that
> structure. I really don't want to have to modify all 250 of those
> structures when i add a new op which only one or two drivers need.

Again, sorry about the confusion. Trevor explained better. We don't
need to do such.

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

[Trevor Gross]

> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +    /// Unknown, not defined in the kernel.
> +    Unknown,
> +}

These can embed the C bindings, but it does look clunky

    pub enum DeviceState {
        Down = bindings::phy_state::PHY_DOWN as isize
        Ready = bindings::phy_state::PHY_READY as isize,
    }

Adding `From`/`Into` `phy_state` may ease use in the future.

> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *(ptr as *mut Self) }
> +    }

`.cast()` is somewhat preferred to `as *` since it prevents
accidentally changing mutability (not a problem here of course)

+    /// Executes software reset the PHY via BMCR_RESET bit.
+    pub fn soft_reset(&mut self) -> Result {
+        let phydev = Opaque::get(&self.0);
+        // SAFETY: This object is initialized by the `from_raw` constructor,
+        // so an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_soft_reset(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }

`error::to_result(ret)` could replace the last 5 lines

> +    pub fn resolve_aneg_linkmode(&mut self) {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        unsafe {
> +            bindings::phy_resolve_aneg_linkmode(phydev);
> +        }
> +    }

Does the block fit on one line with the semicolon outside?

> +
> +    fn driver(&self) -> bindings::phy_driver {
> +        bindings::phy_driver {
> +            name: self.name.as_char_ptr() as *mut i8,
> +            flags: <T>::FLAGS,
> +            soft_reset: if <T>::HAS_SOFT_RESET {
> +                Some(Self::soft_reset_callback)
> +            } else {
> +                None
> +            },
> +            get_features: if <T>::HAS_GET_FEATURES {
> +                Some(Self::get_features_callback)
> +            } else {
> +                None
> +            },
> [ ... ]

`bool` has a `-> Option` function that can make the above more compact

    soft_reset: <T>::HAS_SOFT_RESET.then_some(Self::soft_reset_callback)

> +impl<const N: usize> Drop for Registration<{ N }> {
> +    fn drop(&mut self) {
> +        for i in 0..N {
> +            if let Some(mut drv) = self.drivers[i].take() {
> +                unsafe {
> +                    // SAFETY: Just an FFI call.
> +                    bindings::phy_driver_unregister(drv.as_mut());
> +                }
> +            } else {

Safety comments have to go outside the `{ }` to get picked up by the
clippy lint that we may enable. I think the block could collapse too
with the semicolon on the outside

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

[Jarkko Sakkinen]

On Thu Sep 14, 2023 at 8:47 AM EEST, Trevor Gross wrote:
> > +/// Corresponds to the kernel's `enum phy_state`.
> > +#[derive(PartialEq)]
> > +pub enum DeviceState {
> > +    /// PHY device and driver are not ready for anything.
> > +    Down,
> > +    /// PHY is ready to send and receive packets.
> > +    Ready,
> > +    /// PHY is up, but no polling or interrupts are done.
> > +    Halted,
> > +    /// PHY is up, but is in an error state.
> > +    Error,
> > +    /// PHY and attached device are ready to do work.
> > +    Up,
> > +    /// PHY is currently running.
> > +    Running,
> > +    /// PHY is up, but not currently plugged in.
> > +    NoLink,
> > +    /// PHY is performing a cable test.
> > +    CableTest,
> > +    /// Unknown, not defined in the kernel.
> > +    Unknown,
> > +}
>
> These can embed the C bindings, but it does look clunky
>
>     pub enum DeviceState {
>         Down = bindings::phy_state::PHY_DOWN as isize
>         Ready = bindings::phy_state::PHY_READY as isize,
>     }
>
> Adding `From`/`Into` `phy_state` may ease use in the future.

Just observing but usually in kernel patch reviews anything that
may have use in future should be left out.

BR, Jarkko

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

[Trevor Gross]

On Thu, Sep 14, 2023 at 6:17 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> [ ... ]
> > Adding `From`/`Into` `phy_state` may ease use in the future.
>
> Just observing but usually in kernel patch reviews anything that
> may have use in future should be left out.
>
> BR, Jarkko

Of course - I didn't make it clear but we are already doing that
conversion in `pub fn state(&mut self)` (at least one way), I just
meant to refactor it out to something reusable.

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

[Andrew Lunn]

On Thu, Sep 14, 2023 at 01:47:48AM -0400, Trevor Gross wrote:
> > +/// Corresponds to the kernel's `enum phy_state`.
> > +#[derive(PartialEq)]
> > +pub enum DeviceState {
> > +    /// PHY device and driver are not ready for anything.
> > +    Down,
> > +    /// PHY is ready to send and receive packets.
> > +    Ready,
> > +    /// PHY is up, but no polling or interrupts are done.
> > +    Halted,
> > +    /// PHY is up, but is in an error state.
> > +    Error,
> > +    /// PHY and attached device are ready to do work.
> > +    Up,
> > +    /// PHY is currently running.
> > +    Running,
> > +    /// PHY is up, but not currently plugged in.
> > +    NoLink,
> > +    /// PHY is performing a cable test.
> > +    CableTest,
> > +    /// Unknown, not defined in the kernel.
> > +    Unknown,
> > +}
> 
> These can embed the C bindings, but it does look clunky
> 
>     pub enum DeviceState {
>         Down = bindings::phy_state::PHY_DOWN as isize
>         Ready = bindings::phy_state::PHY_READY as isize,
>     }

Just for my own ignorance of Rust. Does phy_state::PHY_DOWN refer to
the C enum value:

https://elixir.bootlin.com/linux/latest/source/include/linux/phy.h#L507

I can take Clunky when i know i have the compilers on my side ensuring
the two implementations are better in sync. Using this, it looks like
that:

1) The values are guaranteed to be the same, no matter if the enum is changed

2) If an enum value is removed, the Rust code will fail to build if it
   is also not changed.
  
What is not addressed here is if an additional value is added to the
enum.

What some sometimes have in C is an enum value _LAST, which is clearly
documented to always stay at the end, new values have to be inserted
before it. Would it be possible to do a build time comparison of
phy_state::_LAST and in this case Unknown, and fail the build if they
have different values?

	Andrew

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

[Trevor Gross]

On Thu, Sep 14, 2023 at 8:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
> [ ... ]
> >
> > These can embed the C bindings, but it does look clunky
> >
> >     pub enum DeviceState {
> >         Down = bindings::phy_state::PHY_DOWN as isize
> >         Ready = bindings::phy_state::PHY_READY as isize,
> >     }
>
> Just for my own ignorance of Rust. Does phy_state::PHY_DOWN refer to
> the C enum value:
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/phy.h#L507

I actually stand corrected: we generate enums as consts rather than an
enum, so `phy_state_PHY_DOWN` would be correct. But yes, that gives it
the same numeric representation of the C enum value.

> I can take Clunky when i know i have the compilers on my side ensuring
> the two implementations are better in sync. Using this, it looks like
> that:
>
> 1) The values are guaranteed to be the same, no matter if the enum is changed
>
> 2) If an enum value is removed, the Rust code will fail to build if it
>    is also not changed.
>
> What is not addressed here is if an additional value is added to the
> enum.
>
> What some sometimes have in C is an enum value _LAST, which is clearly
> documented to always stay at the end, new values have to be inserted
> before it. Would it be possible to do a build time comparison of
> phy_state::_LAST and in this case Unknown, and fail the build if they
> have different values?

The `Down = some_value` pattern lets you convert from one of Rust's
enums to an integer and guarantees its internal representation, but it
does not let you automatically convert in the opposite direction for
the reason you mentioned. So, you still have to provide a conversion
like was in the original patch

> +        match state {
> +            phy_state_PHY_DOWN => DeviceState::Down,
> +            /* .. */
> +            _ => ??
> +    }

But the exhaustiveness is tricky. `match` is always compile-time
verified to be exhaustive, but there is the caveat that C enums can
technically be any value even if not specified (Rust enums can only
ever be on of the specified variants).

You and Miguel discussed that a bit but to summarize, you have the
following options:

1. - convert with a match, unknown states go to DeviceState::Unknown
  (like this patch does). The downside is you need to check for that
  extra variant everywhere you go.
2. convert with a match but make it an infallible operation by
  returning a `Result` type. You have to do something with the error at some
  point
3. convert with a match, bail with `panic!` on an unexpected value.
  Of course this is heavy handed
4. assume that a phy_state will never have any value other than
  those specified in the C source, which then allows for exhaustive
  matching. This means that if a variant is added to the C enum then the
  compiler makes sure it is handled on the Rust side, but also means that
  things will silently fail if we ever get a value that isn't in the C enum

I think what you may be asking for, and what would be ideal, is a way
to do all of the following:
- make sure that every C enum variant has a Rust representation
- match that exhaustively for the conversion
- panic if we ever get something else because it means the C side
  produced a bad value that is probably causing problems there too

Unfortunately, I don't know of any way to do that. #3 comes close, but
then you panic on "value added in C that we just forgot to add in
Rust". But maybe this is okay since it means the code is unshippable?

This is kind of a good question for RFL in general, maybe the others
have thought about it more.

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

[Trevor Gross]

On Thu, Sep 14, 2023 at 3:42 PM Trevor Gross <tmgross@umich.edu> wrote:
> I think what you may be asking for, and what would be ideal, is a way
> to do all of the following:
> - make sure that every C enum variant has a Rust representation
> - match that exhaustively for the conversion
> - panic if we ever get something else because it means the C side
>   produced a bad value that is probably causing problems there too
>
> Unfortunately, I don't know of any way to do that. #3 comes close, but
> then you panic on "value added in C that we just forgot to add in
> Rust". But maybe this is okay since it means the code is unshippable?
>
> This is kind of a good question for RFL in general, maybe the others
> have thought about it more.

We are developing a few hostprog lints, maybe this could be one of
them. An annotation `// CHECK-C-enum-exhaustive: phy_device` or
similar could indicate to check that every variant in the C source
shows up on the left side of a match pattern.

This would be useful elsewhere too.

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

[Finn Behrens]

On 13 Sep 2023, at 15:36, FUJITA Tomonori wrote:

> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   3 +
>  rust/kernel/lib.rs              |   2 +
>  rust/kernel/net.rs              |   6 +
>  rust/kernel/net/phy.rs          | 651 ++++++++++++++++++++++++++++++++
>  4 files changed, 662 insertions(+)
>  create mode 100644 rust/kernel/net.rs
>  create mode 100644 rust/kernel/net/phy.rs
>
> 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..e84fa513dfda 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -36,6 +36,8 @@
>  pub mod ioctl;
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>  pub mod prelude;
>  pub mod print;
>  mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..b49b052969e5
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_PHYLIB)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..2c5ac5e3213a
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,651 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::vtable, prelude::Box, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +    /// Unknown, not defined in the kernel.
> +    Unknown,
> +}
> +
> +/// Wraps the kernel's `struct phy_device`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *(ptr as *mut Self) }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn id(&mut self) -> u32 {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).phy_id }
> +    }
> +
> +    /// Returns true if the PHY has no link.
> +    pub fn link(&mut self) -> bool {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).link() != 0 }
> +    }
> +
> +    /// Gets the state of the PHY.
> +    pub fn state(&mut self) -> DeviceState {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        let state = unsafe { (*phydev).state };
> +        match state {
> +            0 => DeviceState::Down,
> +            1 => DeviceState::Ready,
> +            2 => DeviceState::Halted,
> +            3 => DeviceState::Error,
> +            4 => DeviceState::Up,
> +            5 => DeviceState::Running,
> +            6 => DeviceState::NoLink,
> +            7 => DeviceState::CableTest,
> +            _ => DeviceState::Unknown,
> +        }
> +    }
> +
> +    /// Returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&mut self) -> bool {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
> +    }
> +
> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&mut self) -> bool {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe { (*phydev).autoneg_complete() != 0 }
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&mut self, speed: i32) {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe {
> +            (*phydev).speed = speed;
> +        }
> +    }
> +
> +    /// Sets duplex mode.
> +    pub fn set_duplex(&mut self, is_full: bool) {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> +        unsafe {
> +            if is_full {
> +                (*phydev).duplex = bindings::DUPLEX_FULL as i32;
> +            } else {
> +                (*phydev).duplex = bindings::DUPLEX_HALF as i32;
> +            }
> +        }
> +    }
> +
> +    /// Executes software reset the PHY via BMCR_RESET bit.
> +    pub fn soft_reset(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_soft_reset(phydev) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads a given PHY register.
> +    pub fn read(&mut self, regnum: u32) -> Result<i32> {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret =
> +            unsafe { bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret)
> +        }
> +    }
> +
> +    /// Writes a given PHY register.
> +    pub fn write(&mut self, regnum: u32, val: u16) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe {
> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum, val)
> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads a given PHY register without the MDIO bus lock taken.
> +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.

This function also has the safety requirement of holding the mdiobus lock.
This could either be an unsafe function, or some other way to ensure the caller holds the lock.

> +        let ret =
> +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret)
> +        }
> +    }
> +
> +    /// Writes a given PHY register without the MDIO bus lock taken.
> +    pub fn lockless_write(&mut self, regnum: u32, val: u16) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.

Same as above.

> +        let ret = unsafe {
> +            bindings::__mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum, val)
> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Resumes the PHY via BMCR_PDOWN bit.
> +    pub fn resume(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_resume(phydev) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Suspends the PHY via BMCR_PDOWN bit.
> +    pub fn suspend(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_suspend(phydev) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Checks the link status and updates current link state.
> +    pub fn read_status(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // 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(())
> +        }
> +    }
> +
> +    /// Reads a paged register.
> +    pub fn read_paged(&mut self, page: i32, regnum: u32) -> Result<i32> {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_read_paged(phydev, page, regnum) };
> +        if ret < 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(ret)
> +        }
> +    }
> +
> +    /// Updates the link status.
> +    pub fn update_link(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_update_link(phydev) };
> +        if ret < 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads Link partner ability.
> +    pub fn read_lpa(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_read_lpa(phydev) };
> +        if ret < 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Resolves the advertisements into PHY settings.
> +    pub fn resolve_aneg_linkmode(&mut self) {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        unsafe {
> +            bindings::phy_resolve_aneg_linkmode(phydev);
> +        }
> +    }
> +
> +    /// Initializes the PHY.
> +    pub fn init_hw(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_init_hw(phydev) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Starts auto-negotiation.
> +    pub fn start_aneg(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_start_aneg(phydev) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads PHY abilities from Clause 22 registers.
> +    pub fn read_abilities(&mut self) -> Result {
> +        let phydev = Opaque::get(&self.0);
> +        // SAFETY: This object is initialized by the `from_raw` constructor,
> +        // so an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_read_abilities(phydev) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        } else {
> +            Ok(())
> +        }
> +    }
> +}
> +
> +/// PHY is internal.
> +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
> +/// PHY needs to be reset after the refclk is enabled.
> +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
> +/// Polling is used to detect PHY status changes.
> +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
> +/// Don't suspend.
> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
> +
> +/// An adapter for the registration of a PHY driver.
> +pub struct Adapter<T: Driver> {
> +    name: &'static CStr,
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> +    /// Creates a new `Adapter` instance.
> +    pub const fn new(name: &'static CStr) -> Adapter<T> {
> +        Self {
> +            name,
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    unsafe extern "C" fn soft_reset_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::soft_reset(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn get_features_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::get_features(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::suspend(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::resume(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn config_aneg_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::config_aneg(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn read_page_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            let ret = T::read_page(dev)?;
> +            Ok(ret)
> +        })
> +    }
> +
> +    unsafe extern "C" fn write_page_callback(
> +        phydev: *mut bindings::phy_device,
> +        page: i32,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::write_page(dev, page)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn read_status_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::read_status(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn match_phy_device_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::match_phy_device(dev) as i32
> +    }
> +
> +    unsafe extern "C" fn read_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            let ret = T::read_mmd(dev, devnum, regnum)?;
> +            Ok(ret)
> +        })
> +    }
> +
> +    unsafe extern "C" fn write_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +        val: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::write_mmd(dev, devnum, regnum, val)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::link_change_notify(dev);
> +    }
> +
> +    fn driver(&self) -> bindings::phy_driver {
> +        bindings::phy_driver {
> +            name: self.name.as_char_ptr() as *mut i8,
> +            flags: <T>::FLAGS,
> +            soft_reset: if <T>::HAS_SOFT_RESET {
> +                Some(Self::soft_reset_callback)
> +            } else {
> +                None
> +            },
> +            get_features: if <T>::HAS_GET_FEATURES {
> +                Some(Self::get_features_callback)
> +            } else {
> +                None
> +            },
> +            match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
> +                Some(Self::match_phy_device_callback)
> +            } else {
> +                None
> +            },
> +            suspend: if <T>::HAS_SUSPEND {
> +                Some(Self::suspend_callback)
> +            } else {
> +                None
> +            },
> +            resume: if <T>::HAS_RESUME {
> +                Some(Self::resume_callback)
> +            } else {
> +                None
> +            },
> +            config_aneg: if <T>::HAS_CONFIG_ANEG {
> +                Some(Self::config_aneg_callback)
> +            } else {
> +                None
> +            },
> +            read_status: if <T>::HAS_READ_STATUS {
> +                Some(Self::read_status_callback)
> +            } else {
> +                None
> +            },
> +            read_page: if <T>::HAS_READ_PAGE {
> +                Some(Self::read_page_callback)
> +            } else {
> +                None
> +            },
> +            write_page: if <T>::HAS_WRITE_PAGE {
> +                Some(Self::write_page_callback)
> +            } else {
> +                None
> +            },
> +            read_mmd: if <T>::HAS_READ_MMD {
> +                Some(Self::read_mmd_callback)
> +            } else {
> +                None
> +            },
> +            write_mmd: if <T>::HAS_WRITE_MMD {
> +                Some(Self::write_mmd_callback)
> +            } else {
> +                None
> +            },
> +            link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
> +                Some(Self::link_change_notify_callback)
> +            } else {
> +                None
> +            },
> +            ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> +        }
> +    }
> +}
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +#[vtable]
> +pub trait Driver {
> +    /// Corresponds to `flags` in `struct phy_driver`.
> +    const FLAGS: u32 = 0;
> +
> +    /// Corresponds to `soft_reset` in `struct phy_driver`.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `get_features` in `struct phy_driver`.
> +    fn get_features(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `match_phy_device` in `struct phy_driver`.
> +    fn match_phy_device(_dev: &mut Device) -> bool;
> +
> +    /// Corresponds to `config_aneg` in `struct phy_driver`.
> +    fn config_aneg(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `read_page` in `struct phy_driver`.
> +    fn read_page(_dev: &mut Device) -> Result<i32> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `write_page` in `struct phy_driver`.
> +    fn write_page(_dev: &mut Device, _page: i32) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `read_status` in `struct phy_driver`.
> +    fn read_status(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `suspend` in `struct phy_driver`.
> +    fn suspend(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `resume` in `struct phy_driver`.
> +    fn resume(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `read_mmd` in `struct phy_driver`.
> +    fn read_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16) -> Result<i32> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `write_mmd` in `struct phy_driver`.
> +    fn write_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16, _val: u16) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Corresponds to `link_change_notify` in `struct phy_driver`.
> +    fn link_change_notify(_dev: &mut Device) {}
> +}
> +
> +/// Registration structure for a PHY driver.
> +///
> +/// `Registration` can keep multiple `phy_drivers` object because
> +/// commonly one module implements multiple PHYs drivers.
> +pub struct Registration<const N: usize> {
> +    module: &'static crate::ThisModule,
> +    drivers: [Option<Box<bindings::phy_driver>>; N],
> +}
> +
> +impl<const N: usize> Registration<{ N }> {
> +    /// Creates a new `Registration` instance.
> +    pub fn new(module: &'static crate::ThisModule) -> Self {
> +        const NONE: Option<Box<bindings::phy_driver>> = None;
> +        Registration {
> +            module,
> +            drivers: [NONE; N],
> +        }
> +    }
> +
> +    /// Registers a PHY driver.
> +    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) -> Result {
> +        for i in 0..N {
> +            if self.drivers[i].is_none() {
> +                let mut drv = Box::try_new(adapter.driver())?;
> +                // SAFETY: Just an FFI call.
> +                let ret = unsafe { bindings::phy_driver_register(drv.as_mut(), self.module.0) };
> +                if ret != 0 {
> +                    return Err(Error::from_errno(ret));
> +                }
> +                self.drivers[i] = Some(drv);
> +                return Ok(());
> +            }
> +        }
> +        Err(code::EINVAL)
> +    }
> +}
> +
> +impl<const N: usize> Drop for Registration<{ N }> {
> +    fn drop(&mut self) {
> +        for i in 0..N {
> +            if let Some(mut drv) = self.drivers[i].take() {
> +                unsafe {
> +                    // SAFETY: Just an FFI call.
> +                    bindings::phy_driver_unregister(drv.as_mut());
> +                }
> +            } else {
> +                break;
> +            }
> +        }
> +    }
> +}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl<const N: usize> Send for Registration<{ N }> {}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl<const N: usize> Sync for Registration<{ N }> {}
> -- 
> 2.34.1

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

[Andrew Lunn]

> > +    /// Reads a given PHY register without the MDIO bus lock taken.
> > +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
> > +        let phydev = Opaque::get(&self.0);
> > +        // SAFETY: This object is initialized by the `from_raw` constructor,
> > +        // so an FFI call with a valid pointer.
> 
> This function also has the safety requirement of holding the mdiobus lock.
> This could either be an unsafe function, or some other way to ensure the caller holds the lock.

For Linux kernel mailing lists, it is considered good practice to trim
emails to just the relevant content. You should also try to keep to <
~80 characters per line. Basically stick to the old netiquette
guidelines.

One option for this safety requirement might be to use

mutex_is_locked(bus->mdio_lock)

	Andrew

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

[Benno Lossin]

 > This patch adds abstractions to implement network PHY drivers; the
 > driver registration and bindings for some of callback functions in
 > struct phy_driver and many genphy_ functions.
 >
 > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
 > ---
 >  rust/bindings/bindings_helper.h |   3 +
 >  rust/kernel/lib.rs              |   2 +
 >  rust/kernel/net.rs              |   6 +
 >  rust/kernel/net/phy.rs          | 651 ++++++++++++++++++++++++++++++++
 >  4 files changed, 662 insertions(+)
 >  create mode 100644 rust/kernel/net.rs
 >  create mode 100644 rust/kernel/net/phy.rs
 >
 > 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..e84fa513dfda 100644
 > --- a/rust/kernel/lib.rs
 > +++ b/rust/kernel/lib.rs
 > @@ -36,6 +36,8 @@
 >  pub mod ioctl;
 >  #[cfg(CONFIG_KUNIT)]
 >  pub mod kunit;
 > +#[cfg(CONFIG_NET)]
 > +pub mod net;
 >  pub mod prelude;
 >  pub mod print;
 >  mod static_assert;
 > diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
 > new file mode 100644
 > index 000000000000..b49b052969e5
 > --- /dev/null
 > +++ b/rust/kernel/net.rs
 > @@ -0,0 +1,6 @@
 > +// SPDX-License-Identifier: GPL-2.0
 > +
 > +//! Networking.
 > +
 > +#[cfg(CONFIG_PHYLIB)]
 > +pub mod phy;
 > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
 > new file mode 100644
 > index 000000000000..2c5ac5e3213a
 > --- /dev/null
 > +++ b/rust/kernel/net/phy.rs
 > @@ -0,0 +1,651 @@
 > +// SPDX-License-Identifier: GPL-2.0
 > +
 > +//! Network PHY device.
 > +//!
 > +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
 > +
 > +use crate::{bindings, error::*, prelude::vtable, prelude::Box, 
str::CStr, types::Opaque};
 > +use core::marker::PhantomData;
 > +
 > +/// Corresponds to the kernel's `enum phy_state`.
 > +#[derive(PartialEq)]
 > +pub enum DeviceState {
 > +    /// PHY device and driver are not ready for anything.
 > +    Down,
 > +    /// PHY is ready to send and receive packets.
 > +    Ready,
 > +    /// PHY is up, but no polling or interrupts are done.
 > +    Halted,
 > +    /// PHY is up, but is in an error state.
 > +    Error,
 > +    /// PHY and attached device are ready to do work.
 > +    Up,
 > +    /// PHY is currently running.
 > +    Running,
 > +    /// PHY is up, but not currently plugged in.
 > +    NoLink,
 > +    /// PHY is performing a cable test.
 > +    CableTest,
 > +    /// Unknown, not defined in the kernel.
 > +    Unknown,
 > +}
 > +
 > +/// Wraps the kernel's `struct phy_device`.
 > +#[repr(transparent)]
 > +pub struct Device(Opaque<bindings::phy_device>);
 > +
 > +impl Device {
 > +    /// Creates a new [`Device`] instance from a raw pointer.
 > +    ///
 > +    /// # Safety
 > +    ///
 > +    /// For the duration of the lifetime 'a, the pointer must be 
valid for writing and nobody else
 > +    /// may read or write to the `phy_device` object.
 > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> 
&'a mut Self {
 > +        unsafe { &mut *(ptr as *mut Self) }
 > +    }
 > +
 > +    /// Gets the id of the PHY.
 > +    pub fn id(&mut self) -> u32 {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe { (*phydev).phy_id }
 > +    }
 > +
 > +    /// Returns true if the PHY has no link.
 > +    pub fn link(&mut self) -> bool {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe { (*phydev).link() != 0 }
 > +    }
 > +
 > +    /// Gets the state of the PHY.
 > +    pub fn state(&mut self) -> DeviceState {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        let state = unsafe { (*phydev).state };
 > +        match state {
 > +            0 => DeviceState::Down,
 > +            1 => DeviceState::Ready,
 > +            2 => DeviceState::Halted,
 > +            3 => DeviceState::Error,
 > +            4 => DeviceState::Up,
 > +            5 => DeviceState::Running,
 > +            6 => DeviceState::NoLink,
 > +            7 => DeviceState::CableTest,
 > +            _ => DeviceState::Unknown,
 > +        }
 > +    }
 > +
 > +    /// Returns true if auto-negotiation is enabled.
 > +    pub fn is_autoneg_enabled(&mut self) -> bool {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
 > +    }
 > +
 > +    /// Returns true if auto-negotiation is completed.
 > +    pub fn is_autoneg_completed(&mut self) -> bool {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe { (*phydev).autoneg_complete() != 0 }
 > +    }
 > +
 > +    /// Sets the speed of the PHY.
 > +    pub fn set_speed(&mut self, speed: i32) {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe {
 > +            (*phydev).speed = speed;
 > +        }
 > +    }
 > +
 > +    /// Sets duplex mode.
 > +    pub fn set_duplex(&mut self, is_full: bool) {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor, so it's valid.
 > +        unsafe {
 > +            if is_full {
 > +                (*phydev).duplex = bindings::DUPLEX_FULL as i32;
 > +            } else {
 > +                (*phydev).duplex = bindings::DUPLEX_HALF as i32;
 > +            }
 > +        }
 > +    }
 > +
 > +    /// Executes software reset the PHY via BMCR_RESET bit.
 > +    pub fn soft_reset(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_soft_reset(phydev) };
 > +        if ret < 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Reads a given PHY register.
 > +    pub fn read(&mut self, regnum: u32) -> Result<i32> {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret =
 > +            unsafe { bindings::mdiobus_read((*phydev).mdio.bus, 
(*phydev).mdio.addr, regnum) };
 > +        if ret < 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(ret)
 > +        }
 > +    }
 > +
 > +    /// Writes a given PHY register.
 > +    pub fn write(&mut self, regnum: u32, val: u16) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe {
 > +            bindings::mdiobus_write((*phydev).mdio.bus, 
(*phydev).mdio.addr, regnum, val)
 > +        };
 > +        if ret < 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Reads a given PHY register without the MDIO bus lock taken.
 > +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret =
 > +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, 
(*phydev).mdio.addr, regnum) };
 > +        if ret < 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(ret)
 > +        }
 > +    }
 > +
 > +    /// Writes a given PHY register without the MDIO bus lock taken.
 > +    pub fn lockless_write(&mut self, regnum: u32, val: u16) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe {
 > +            bindings::__mdiobus_write((*phydev).mdio.bus, 
(*phydev).mdio.addr, regnum, val)
 > +        };
 > +        if ret < 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Resumes the PHY via BMCR_PDOWN bit.
 > +    pub fn resume(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_resume(phydev) };
 > +        if ret != 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Suspends the PHY via BMCR_PDOWN bit.
 > +    pub fn suspend(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_suspend(phydev) };
 > +        if ret != 0 {
 > +            Err(Error::from_errno(ret))
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Checks the link status and updates current link state.
 > +    pub fn read_status(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // 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(())
 > +        }
 > +    }
 > +
 > +    /// Reads a paged register.
 > +    pub fn read_paged(&mut self, page: i32, regnum: u32) -> 
Result<i32> {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::phy_read_paged(phydev, page, 
regnum) };
 > +        if ret < 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(ret)
 > +        }
 > +    }
 > +
 > +    /// Updates the link status.
 > +    pub fn update_link(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_update_link(phydev) };
 > +        if ret < 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Reads Link partner ability.
 > +    pub fn read_lpa(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_read_lpa(phydev) };
 > +        if ret < 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Resolves the advertisements into PHY settings.
 > +    pub fn resolve_aneg_linkmode(&mut self) {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        unsafe {
 > +            bindings::phy_resolve_aneg_linkmode(phydev);
 > +        }
 > +    }
 > +
 > +    /// Initializes the PHY.
 > +    pub fn init_hw(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::phy_init_hw(phydev) };
 > +        if ret != 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Starts auto-negotiation.
 > +    pub fn start_aneg(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::phy_start_aneg(phydev) };
 > +        if ret != 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +
 > +    /// Reads PHY abilities from Clause 22 registers.
 > +    pub fn read_abilities(&mut self) -> Result {
 > +        let phydev = Opaque::get(&self.0);
 > +        // SAFETY: This object is initialized by the `from_raw` 
constructor,
 > +        // so an FFI call with a valid pointer.
 > +        let ret = unsafe { bindings::genphy_read_abilities(phydev) };
 > +        if ret != 0 {
 > +            return Err(Error::from_errno(ret));
 > +        } else {
 > +            Ok(())
 > +        }
 > +    }
 > +}
 > +
 > +/// PHY is internal.
 > +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
 > +/// PHY needs to be reset after the refclk is enabled.
 > +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
 > +/// Polling is used to detect PHY status changes.
 > +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
 > +/// Don't suspend.
 > +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;

I think we should create a macro for creating flags, it should create a 
newtype and
work similar to an enum declaration. On the newtype it should implement 
the `Or` trait
for allowing `PHY_IS_INTERNAL | PHY_RST_AFTER_CLK_EN`.

 > +
 > +/// An adapter for the registration of a PHY driver.
 > +pub struct Adapter<T: Driver> {
 > +    name: &'static CStr,
 > +    _p: PhantomData<T>,
 > +}
 > +
 > +impl<T: Driver> Adapter<T> {
 > +    /// Creates a new `Adapter` instance.
 > +    pub const fn new(name: &'static CStr) -> Adapter<T> {
 > +        Self {
 > +            name,
 > +            _p: PhantomData,
 > +        }
 > +    }
 > +
 > +    unsafe extern "C" fn soft_reset_callback(
 > +        phydev: *mut bindings::phy_device,
 > +    ) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::soft_reset(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn get_features_callback(
 > +        phydev: *mut bindings::phy_device,
 > +    ) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::get_features(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn suspend_callback(phydev: *mut 
bindings::phy_device) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::suspend(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn resume_callback(phydev: *mut 
bindings::phy_device) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::resume(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn config_aneg_callback(
 > +        phydev: *mut bindings::phy_device,
 > +    ) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::config_aneg(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn read_page_callback(phydev: *mut 
bindings::phy_device) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            let ret = T::read_page(dev)?;
 > +            Ok(ret)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn write_page_callback(
 > +        phydev: *mut bindings::phy_device,
 > +        page: i32,
 > +    ) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::write_page(dev, page)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn read_status_callback(
 > +        phydev: *mut bindings::phy_device,
 > +    ) -> core::ffi::c_int {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::read_status(dev)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn match_phy_device_callback(
 > +        phydev: *mut bindings::phy_device,
 > +    ) -> core::ffi::c_int {
 > +        // SAFETY: The C API guarantees that `phydev` is valid while 
this function is running.
 > +        let dev = unsafe { Device::from_raw(phydev) };
 > +        T::match_phy_device(dev) as i32
 > +    }
 > +
 > +    unsafe extern "C" fn read_mmd_callback(
 > +        phydev: *mut bindings::phy_device,
 > +        devnum: i32,
 > +        regnum: u16,
 > +    ) -> i32 {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            let ret = T::read_mmd(dev, devnum, regnum)?;
 > +            Ok(ret)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn write_mmd_callback(
 > +        phydev: *mut bindings::phy_device,
 > +        devnum: i32,
 > +        regnum: u16,
 > +        val: u16,
 > +    ) -> i32 {
 > +        from_result(|| {
 > +            // SAFETY: The C API guarantees that `phydev` is valid 
while this function is running.
 > +            let dev = unsafe { Device::from_raw(phydev) };
 > +            T::write_mmd(dev, devnum, regnum, val)?;
 > +            Ok(0)
 > +        })
 > +    }
 > +
 > +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut 
bindings::phy_device) {
 > +        // SAFETY: The C API guarantees that `phydev` is valid while 
this function is running.
 > +        let dev = unsafe { Device::from_raw(phydev) };
 > +        T::link_change_notify(dev);
 > +    }
 > +
 > +    fn driver(&self) -> bindings::phy_driver {
 > +        bindings::phy_driver {
 > +            name: self.name.as_char_ptr() as *mut i8,
 > +            flags: <T>::FLAGS,
 > +            soft_reset: if <T>::HAS_SOFT_RESET {
 > +                Some(Self::soft_reset_callback)
 > +            } else {
 > +                None
 > +            },
 > +            get_features: if <T>::HAS_GET_FEATURES {
 > +                Some(Self::get_features_callback)
 > +            } else {
 > +                None
 > +            },
 > +            match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
 > +                Some(Self::match_phy_device_callback)
 > +            } else {
 > +                None
 > +            },
 > +            suspend: if <T>::HAS_SUSPEND {
 > +                Some(Self::suspend_callback)
 > +            } else {
 > +                None
 > +            },
 > +            resume: if <T>::HAS_RESUME {
 > +                Some(Self::resume_callback)
 > +            } else {
 > +                None
 > +            },
 > +            config_aneg: if <T>::HAS_CONFIG_ANEG {
 > +                Some(Self::config_aneg_callback)
 > +            } else {
 > +                None
 > +            },
 > +            read_status: if <T>::HAS_READ_STATUS {
 > +                Some(Self::read_status_callback)
 > +            } else {
 > +                None
 > +            },
 > +            read_page: if <T>::HAS_READ_PAGE {
 > +                Some(Self::read_page_callback)
 > +            } else {
 > +                None
 > +            },
 > +            write_page: if <T>::HAS_WRITE_PAGE {
 > +                Some(Self::write_page_callback)
 > +            } else {
 > +                None
 > +            },
 > +            read_mmd: if <T>::HAS_READ_MMD {
 > +                Some(Self::read_mmd_callback)
 > +            } else {
 > +                None
 > +            },
 > +            write_mmd: if <T>::HAS_WRITE_MMD {
 > +                Some(Self::write_mmd_callback)
 > +            } else {
 > +                None
 > +            },
 > +            link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
 > +                Some(Self::link_change_notify_callback)
 > +            } else {
 > +                None
 > +            },
 > +            ..unsafe { 
core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
 > +        }
 > +    }
 > +}
 > +
 > +/// Corresponds to functions in `struct phy_driver`.
 > +#[vtable]
 > +pub trait Driver {
 > +    /// Corresponds to `flags` in `struct phy_driver`.
 > +    const FLAGS: u32 = 0;
 > +
 > +    /// Corresponds to `soft_reset` in `struct phy_driver`.
 > +    fn soft_reset(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `get_features` in `struct phy_driver`.
 > +    fn get_features(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `match_phy_device` in `struct phy_driver`.
 > +    fn match_phy_device(_dev: &mut Device) -> bool;
 > +
 > +    /// Corresponds to `config_aneg` in `struct phy_driver`.
 > +    fn config_aneg(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `read_page` in `struct phy_driver`.
 > +    fn read_page(_dev: &mut Device) -> Result<i32> {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `write_page` in `struct phy_driver`.
 > +    fn write_page(_dev: &mut Device, _page: i32) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `read_status` in `struct phy_driver`.
 > +    fn read_status(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `suspend` in `struct phy_driver`.
 > +    fn suspend(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `resume` in `struct phy_driver`.
 > +    fn resume(_dev: &mut Device) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `read_mmd` in `struct phy_driver`.
 > +    fn read_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16) -> 
Result<i32> {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `write_mmd` in `struct phy_driver`.
 > +    fn write_mmd(_dev: &mut Device, _devnum: i32, _regnum: u16, 
_val: u16) -> Result {
 > +        Err(code::ENOTSUPP)
 > +    }
 > +
 > +    /// Corresponds to `link_change_notify` in `struct phy_driver`.
 > +    fn link_change_notify(_dev: &mut Device) {}
 > +}
 > +
 > +/// Registration structure for a PHY driver.
 > +///
 > +/// `Registration` can keep multiple `phy_drivers` object because
 > +/// commonly one module implements multiple PHYs drivers.
 > +pub struct Registration<const N: usize> {
 > +    module: &'static crate::ThisModule,
 > +    drivers: [Option<Box<bindings::phy_driver>>; N],

Why is this not a vector? You have to allocate in `register` anyways.
You could also use `Vec::try_with_capacity(N)` to initialize the vector with
capacity for N elements.

 > +}
 > +
 > +impl<const N: usize> Registration<{ N }> {
 > +    /// Creates a new `Registration` instance.
 > +    pub fn new(module: &'static crate::ThisModule) -> Self {
 > +        const NONE: Option<Box<bindings::phy_driver>> = None;
 > +        Registration {
 > +            module,
 > +            drivers: [NONE; N],
 > +        }
 > +    }
 > +
 > +    /// Registers a PHY driver.
 > +    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) -> 
Result {

The way this function is used in the driver makes me think that the 
`Adapter`
type does not have to be public. So I would suggest to change the 
signature to
`pub fn register<T: Driver>(&mut self, name: &'static CStr) -> Result`.


 > +        for i in 0..N {
 > +            if self.drivers[i].is_none() {
 > +                let mut drv = Box::try_new(adapter.driver())?;
 > +                // SAFETY: Just an FFI call.
 > +                let ret = unsafe { 
bindings::phy_driver_register(drv.as_mut(), self.module.0) };

This call exposes the driver to the C side and thus it is able to be 
modified at any time,
which means that in Rust it should be put into an `UnsafeCell`, better 
even in this case it
should just be `Opaque`, so the `drivers` fields should be of type
`[Option<Box<Opaque<bindings::phy_driver>>>]`.
It would also be a good idea to pin it, since the C side will rely on 
the pointee not moving
and it will prevent accidentally moving it.

 > +                if ret != 0 {
 > +                    return Err(Error::from_errno(ret));
 > +                }
 > +                self.drivers[i] = Some(drv);
 > +                return Ok(());
 > +            }
 > +        }
 > +        Err(code::EINVAL)
 > +    }
 > +}
 > +
 > +impl<const N: usize> Drop for Registration<{ N }> {
 > +    fn drop(&mut self) {
 > +        for i in 0..N {
 > +            if let Some(mut drv) = self.drivers[i].take() {
 > +                unsafe {
 > +                    // SAFETY: Just an FFI call.
 > +                    bindings::phy_driver_unregister(drv.as_mut());
 > +                }
 > +            } else {
 > +                break;
 > +            }
 > +        }
 > +    }
 > +}
 > +
 > +// SAFETY: `Registration` does not expose any of its state across 
threads.
 > +unsafe impl<const N: usize> Send for Registration<{ N }> {}
 > +
 > +// SAFETY: `Registration` does not expose any of its state across 
threads.
 > +unsafe impl<const N: usize> Sync for Registration<{ N }> {}
 > --
 > 2.34.1
 >
 >

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

[FUJITA Tomonori]

Hi,

On Mon, 18 Sep 2023 10:22:22 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>  > +/// PHY is internal.
>  > +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
>  > +/// PHY needs to be reset after the refclk is enabled.
>  > +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
>  > +/// Polling is used to detect PHY status changes.
>  > +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
>  > +/// Don't suspend.
>  > +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
> 
> I think we should create a macro for creating flags, it should create a 
> newtype and
> work similar to an enum declaration. On the newtype it should implement 
> the `Or` trait
> for allowing `PHY_IS_INTERNAL | PHY_RST_AFTER_CLK_EN`.

You are thinking about something like:
https://crates.io/crates/enumflags2

?

>  > +/// Registration structure for a PHY driver.
>  > +///
>  > +/// `Registration` can keep multiple `phy_drivers` object because
>  > +/// commonly one module implements multiple PHYs drivers.
>  > +pub struct Registration<const N: usize> {
>  > +    module: &'static crate::ThisModule,
>  > +    drivers: [Option<Box<bindings::phy_driver>>; N],
> 
> Why is this not a vector? You have to allocate in `register` anyways.
> You could also use `Vec::try_with_capacity(N)` to initialize the vector with
> capacity for N elements.

I thought that this is simpler. What are advantages of Vec over this?


>  > +impl<const N: usize> Registration<{ N }> {
>  > +    /// Creates a new `Registration` instance.
>  > +    pub fn new(module: &'static crate::ThisModule) -> Self {
>  > +        const NONE: Option<Box<bindings::phy_driver>> = None;
>  > +        Registration {
>  > +            module,
>  > +            drivers: [NONE; N],
>  > +        }
>  > +    }
>  > +
>  > +    /// Registers a PHY driver.
>  > +    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) -> 
> Result {
> 
> The way this function is used in the driver makes me think that the 
> `Adapter`
> type does not have to be public. So I would suggest to change the 
> signature to
> `pub fn register<T: Driver>(&mut self, name: &'static CStr) -> Result`.

Ah, right. I'll in the next round.

> 
>  > +        for i in 0..N {
>  > +            if self.drivers[i].is_none() {
>  > +                let mut drv = Box::try_new(adapter.driver())?;
>  > +                // SAFETY: Just an FFI call.
>  > +                let ret = unsafe { 
> bindings::phy_driver_register(drv.as_mut(), self.module.0) };
> 
> This call exposes the driver to the C side and thus it is able to be 
> modified at any time,
> which means that in Rust it should be put into an `UnsafeCell`, better 
> even in this case it
> should just be `Opaque`, so the `drivers` fields should be of type
> `[Option<Box<Opaque<bindings::phy_driver>>>]`.

Understood.

> It would also be a good idea to pin it, since the C side will rely on 
> the pointee not moving
> and it will prevent accidentally moving it.

The above drv is touched by only this file. What possible cases where
it would be moved?

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

[Benno Lossin]

On 9/18/23 15:09, FUJITA Tomonori wrote:
> Hi,
> 
> On Mon, 18 Sep 2023 10:22:22 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>   > +/// PHY is internal.
>>   > +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
>>   > +/// PHY needs to be reset after the refclk is enabled.
>>   > +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
>>   > +/// Polling is used to detect PHY status changes.
>>   > +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
>>   > +/// Don't suspend.
>>   > +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
>>
>> I think we should create a macro for creating flags, it should create a
>> newtype and
>> work similar to an enum declaration. On the newtype it should implement
>> the `Or` trait
>> for allowing `PHY_IS_INTERNAL | PHY_RST_AFTER_CLK_EN`.
> 
> You are thinking about something like:
> https://crates.io/crates/enumflags2
> 
> ?

I would prefer https://crates.io/crates/bitflags
because this library does not create a separate type for multiple flags.

I will look into this and see with what I come up.

> 
>>   > +/// Registration structure for a PHY driver.
>>   > +///
>>   > +/// `Registration` can keep multiple `phy_drivers` object because
>>   > +/// commonly one module implements multiple PHYs drivers.
>>   > +pub struct Registration<const N: usize> {
>>   > +    module: &'static crate::ThisModule,
>>   > +    drivers: [Option<Box<bindings::phy_driver>>; N],
>>
>> Why is this not a vector? You have to allocate in `register` anyways.
>> You could also use `Vec::try_with_capacity(N)` to initialize the vector with
>> capacity for N elements.
> 
> I thought that this is simpler. What are advantages of Vec over this?
> 

Pushing a new value is a lot simpler, since it only is a single function 
call and not a loop. You also do not need a generic parameter.

> 
>>   > +impl<const N: usize> Registration<{ N }> {
>>   > +    /// Creates a new `Registration` instance.
>>   > +    pub fn new(module: &'static crate::ThisModule) -> Self {
>>   > +        const NONE: Option<Box<bindings::phy_driver>> = None;
>>   > +        Registration {
>>   > +            module,
>>   > +            drivers: [NONE; N],
>>   > +        }
>>   > +    }
>>   > +
>>   > +    /// Registers a PHY driver.
>>   > +    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) ->
>> Result {
>>
>> The way this function is used in the driver makes me think that the
>> `Adapter`
>> type does not have to be public. So I would suggest to change the
>> signature to
>> `pub fn register<T: Driver>(&mut self, name: &'static CStr) -> Result`.
> 
> Ah, right. I'll in the next round.
> 
>>
>>   > +        for i in 0..N {
>>   > +            if self.drivers[i].is_none() {
>>   > +                let mut drv = Box::try_new(adapter.driver())?;
>>   > +                // SAFETY: Just an FFI call.
>>   > +                let ret = unsafe {
>> bindings::phy_driver_register(drv.as_mut(), self.module.0) };
>>
>> This call exposes the driver to the C side and thus it is able to be
>> modified at any time,
>> which means that in Rust it should be put into an `UnsafeCell`, better
>> even in this case it
>> should just be `Opaque`, so the `drivers` fields should be of type
>> `[Option<Box<Opaque<bindings::phy_driver>>>]`.
> 
> Understood.

Actually, I talked with Gary and what I suggested is actually not 
sufficient, because `Box` means that we have a unique pointer, even 
though it contains an Opaque. So what we would suggest would be to use 
`Box::into_raw` then both store that pointer and give it to 
`phy_driver_register`.

> 
>> It would also be a good idea to pin it, since the C side will rely on
>> the pointee not moving
>> and it will prevent accidentally moving it.
> 
> The above drv is touched by only this file. What possible cases where
> it would be moved?
> 

If you do what I wrote above, then this is not a problem. The rationale 
behind this suggestion was to make it impossible to move the value in a 
future modification of this file.

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

[FUJITA Tomonori]

Hi,

On Mon, 18 Sep 2023 15:20:53 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 9/18/23 15:09, FUJITA Tomonori wrote:
>> Hi,
>> 
>> On Mon, 18 Sep 2023 10:22:22 +0000
>> Benno Lossin <benno.lossin@proton.me> wrote:
>> 
>>>   > +/// PHY is internal.
>>>   > +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
>>>   > +/// PHY needs to be reset after the refclk is enabled.
>>>   > +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
>>>   > +/// Polling is used to detect PHY status changes.
>>>   > +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
>>>   > +/// Don't suspend.
>>>   > +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
>>>
>>> I think we should create a macro for creating flags, it should create a
>>> newtype and
>>> work similar to an enum declaration. On the newtype it should implement
>>> the `Or` trait
>>> for allowing `PHY_IS_INTERNAL | PHY_RST_AFTER_CLK_EN`.
>> 
>> You are thinking about something like:
>> https://crates.io/crates/enumflags2
>> 
>> ?
> 
> I would prefer https://crates.io/crates/bitflags
> because this library does not create a separate type for multiple flags.
> 
> I will look into this and see with what I come up.

Thanks. Until then, I leave the current code alone.


>>>   > +/// Registration structure for a PHY driver.
>>>   > +///
>>>   > +/// `Registration` can keep multiple `phy_drivers` object because
>>>   > +/// commonly one module implements multiple PHYs drivers.
>>>   > +pub struct Registration<const N: usize> {
>>>   > +    module: &'static crate::ThisModule,
>>>   > +    drivers: [Option<Box<bindings::phy_driver>>; N],
>>>
>>> Why is this not a vector? You have to allocate in `register` anyways.
>>> You could also use `Vec::try_with_capacity(N)` to initialize the vector with
>>> capacity for N elements.
>> 
>> I thought that this is simpler. What are advantages of Vec over this?
>> 
> 
> Pushing a new value is a lot simpler, since it only is a single function 
> call and not a loop. You also do not need a generic parameter.

How about the following Vec version, looks simpler?

pub struct Registration {
    module: &'static crate::ThisModule,
    drivers: Vec<*mut bindings::phy_driver>,
}

impl Registration {
    /// Creates a new `Registration` instance.
    pub fn new(module: &'static crate::ThisModule) -> Self {
        Registration {
            module,
            drivers: Vec::new(),
        }
    }

    /// Registers a PHY driver.
    pub fn register<T: Driver>(&mut self) -> Result {
        self.drivers.try_reserve(1)?;
        let drv = Box::into_raw(Box::try_new(Adapter::<T>::make_phy_driver())?);
        let ret = unsafe { bindings::phy_driver_register(drv, self.module.0) };
        if ret != 0 {
            // free memory
            unsafe {
                let _ = Box::from_raw(drv);
            }
            return Err(Error::from_errno(ret));
        }
        // we just called try_reserve() above so it will not fail.
        let _ = self.drivers.try_push(drv);
        Ok(())
    }
}

Trevor proposed another implementation to use static allocation, what
do you think?

https://lore.kernel.org/rust-for-linux/CALNs47vp6D-tqxa5NnE3i-M765GxwVxC2OArjBcfwY3zAcd8TQ@mail.gmail.com/T/#m52eeee680a531f6d1a8b0398c1462a70e38cdb67


> Actually, I talked with Gary and what I suggested is actually not 
> sufficient, because `Box` means that we have a unique pointer, even 
> though it contains an Opaque. So what we would suggest would be to use 
> `Box::into_raw` then both store that pointer and give it to 
> `phy_driver_register`.
> 
>> 
>>> It would also be a good idea to pin it, since the C side will rely on
>>> the pointee not moving
>>> and it will prevent accidentally moving it.
>> 
>> The above drv is touched by only this file. What possible cases where
>> it would be moved?
>> 
> 
> If you do what I wrote above, then this is not a problem. The rationale 
> behind this suggestion was to make it impossible to move the value in a 
> future modification of this file.

an precaution for a future modificaiton of this file, understood.

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

[Benno Lossin]

On 19.09.23 12:26, FUJITA Tomonori wrote:
>>>>    > +/// Registration structure for a PHY driver.
>>>>    > +///
>>>>    > +/// `Registration` can keep multiple `phy_drivers` object because
>>>>    > +/// commonly one module implements multiple PHYs drivers.
>>>>    > +pub struct Registration<const N: usize> {
>>>>    > +    module: &'static crate::ThisModule,
>>>>    > +    drivers: [Option<Box<bindings::phy_driver>>; N],
>>>>
>>>> Why is this not a vector? You have to allocate in `register` anyways.
>>>> You could also use `Vec::try_with_capacity(N)` to initialize the vector with
>>>> capacity for N elements.
>>>
>>> I thought that this is simpler. What are advantages of Vec over this?
>>>
>>
>> Pushing a new value is a lot simpler, since it only is a single function
>> call and not a loop. You also do not need a generic parameter.
> 
> How about the following Vec version, looks simpler?

[...]

Yes it does look simpler.

> 
> Trevor proposed another implementation to use static allocation, what
> do you think?

I think that we should use static allocation wherever possible/if the C
side uses it, we should try to also achieve it.

-- 
Cheers,
Benno

[RFC PATCH v1 2/4] rust: phy: add module device table support

[FUJITA Tomonori]

This macro corresponds to the kernel's MODULE_DEVICE_TABLE macro,
which embeds the information for module loading into the module binary
file.


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

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 2c5ac5e3213a..ffe1a609a7b2 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -649,3 +649,39 @@ unsafe impl<const N: usize> Send for Registration<{ N }> {}
 
 // SAFETY: `Registration` does not expose any of its state across threads.
 unsafe impl<const N: usize> Sync for Registration<{ N }> {}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! replace_expr {
+    ($_t:tt $sub:expr) => {
+        $sub
+    };
+}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! count_drivers {
+    ( $(($x:expr, $_y:expr)),* ) => {
+        0usize $(+ $crate::replace_expr!($x 1usize))*
+    };
+}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! mdio_device_id_table {
+    ($(($x:expr, $y:expr)),*) => {
+        [$(kernel::bindings::mdio_device_id {phy_id: $x, phy_id_mask:$y}),*, kernel::bindings::mdio_device_id {phy_id:0, phy_id_mask:0}]
+    }
+}
+
+/// Defines module device table.
+///
+/// This corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
+/// for module loading into the module binary file.
+#[macro_export]
+macro_rules! phy_module_device_table {
+    ($name:ident, [$($t:tt)*]) => {
+        #[no_mangle]
+        static $name: [kernel::bindings::mdio_device_id; $crate::count_drivers!($($t)*)+1] = $crate::mdio_device_id_table!($($t)*);
+    }
+}
-- 
2.34.1

Re: [RFC PATCH v1 2/4] rust: phy: add module device table support

[Trevor Gross]

On Wed, Sep 13, 2023 at 9:37 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This macro corresponds to the kernel's MODULE_DEVICE_TABLE macro,
> which embeds the information for module loading into the module binary
> file.
>
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 2c5ac5e3213a..ffe1a609a7b2 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -649,3 +649,39 @@ unsafe impl<const N: usize> Send for Registration<{ N }> {}
>
>  // SAFETY: `Registration` does not expose any of its state across threads.
>  unsafe impl<const N: usize> Sync for Registration<{ N }> {}
> +
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! replace_expr {
> +    ($_t:tt $sub:expr) => {
> +        $sub
> +    };
> +}
> +
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! count_drivers {
> +    ( $(($x:expr, $_y:expr)),* ) => {
> +        0usize $(+ $crate::replace_expr!($x 1usize))*
> +    };
> +}
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! mdio_device_id_table {
> +    ($(($x:expr, $y:expr)),*) => {
> +        [$(kernel::bindings::mdio_device_id {phy_id: $x, phy_id_mask:$y}),*, kernel::bindings::mdio_device_id {phy_id:0, phy_id_mask:0}]

Some formatting like
    [
        $(kernel::bindings::mdio_device_id {
            phy_id: $x,
            phy_id_mask:$y
          }),*,
        kernel::bindings::mdio_device_id {
            phy_id: 0,
            phy_id_mask: 0
        }
    ]

I think `),*,` expands the same as `),*`

> +    }
> +}
> +/// Defines module device table.
> +///
> +/// This corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file.
> +#[macro_export]
> +macro_rules! phy_module_device_table {
> +    ($name:ident, [$($t:tt)*]) => {
> +        #[no_mangle]
> +        static $name: [kernel::bindings::mdio_device_id; $crate::count_drivers!($($t)*)+1] = $crate::mdio_device_id_table!($($t)*);
> +    }
> +}
> --
> 2.34.1

Is there a reason that the mdio_device_id_table is separate from
phy_module_device_table? You could use one match pattern

    ($name:ident, [ $( ($x:expr, $y:expr) ),*  $(,)?])

Also added the optional trailing comma matcher `$(,)?` there.

Actually, since there is only one public macro, all three hidden
macros could actually all be internal patterns within
phy_module_device_table. E.g. `(@replace_expr $_from:tt, $sub:expr)`,
just helps reduce what is in macro scope.

Could you also give the capture vars more descriptive names? This one
isn't all that bad but macros generally aren't the easiest thing to
understand by reading...

Re: [RFC PATCH v1 2/4] rust: phy: add module device table support

[Trevor Gross]

Actually, looking closer at the use of this macro

+kernel::phy_module_device_table!(
+    __mod_mdio__asix_tbl_device_table,
+    [(PhyAx88772A::PHY_ID_ASIX_AX88772A, 0xffffffff)]
+);

Since the `paste!` macro is in kernel now this could be refactored
more like `MODULE_DEVICE_TABLE`, i.e. just take "asix" rather than the
clunky name.

Maybe it would be good to have a trait for a MDIO device as well

    enum PhyIdMask {
        Exact,
        Model,
        Vendor,
        Custom(u32)
    }

    impl PhyIdMask {
        const fn as_int(self) -> u32 { /* ... */ }
    }

    trait MdioDevice {
        const PHY_ID: u32;
        const PHY_ID_MASK: PhyIdMask;
    }

Then the macro just becomes

    kernel::register_mdio_device!(asix: PhyAx88772A, PhyAx88772C, PhyAx88796B);

And expands as

    static __mod_mdio__asix_tbl_device_table: [mdio_device_id; 3] = [
        mdio_device_id {
            phy_id: PhyAx88772A::PHY_ID,
            phy_id_mask: PhyAx88772A::PHY_ID_MASK.as_int()
        }
        // ...
    ];

Re: [RFC PATCH v1 2/4] rust: phy: add module device table support

[FUJITA Tomonori]

Hi,

On Thu, 14 Sep 2023 03:23:56 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> Actually, looking closer at the use of this macro
> 
> +kernel::phy_module_device_table!(
> +    __mod_mdio__asix_tbl_device_table,
> +    [(PhyAx88772A::PHY_ID_ASIX_AX88772A, 0xffffffff)]
> +);
> 
> Since the `paste!` macro is in kernel now this could be refactored
> more like `MODULE_DEVICE_TABLE`, i.e. just take "asix" rather than the
> clunky name.

Thanks! That's exactly what I had benn looking for.


> Maybe it would be good to have a trait for a MDIO device as well
> 
>     enum PhyIdMask {
>         Exact,
>         Model,
>         Vendor,
>         Custom(u32)
>     }
> 
>     impl PhyIdMask {
>         const fn as_int(self) -> u32 { /* ... */ }
>     }
> 
>     trait MdioDevice {
>         const PHY_ID: u32;
>         const PHY_ID_MASK: PhyIdMask;
>     }
> 
> Then the macro just becomes
> 
>     kernel::register_mdio_device!(asix: PhyAx88772A, PhyAx88772C, PhyAx88796B);

One driver doesn't always correspond to one device id entry. For
example, realtek PHY module supports tons of drivers but have only one
device id entry.

https://github.com/torvalds/linux/blob/f0b0d403eabbe135d8dbb40ad5e41018947d336c/drivers/net/phy/realtek.c#L1089

So I prefer to define the device id independently. I use the following
code now.

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 8e7f4aff5dfb..b69933f4d054 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -650,3 +650,77 @@ unsafe impl<const N: usize> Send for Registration<{ N }> {}
 
 // SAFETY: `Registration` does not expose any of its state across threads.
 unsafe impl<const N: usize> Sync for Registration<{ N }> {}
+
+pub enum DeviceMask {
+    Exact,
+    Model,
+    Vendor,
+    Custom(u32),
+}
+
+impl DeviceMask {
+    pub const fn as_int(self) -> u32 {
+        match self {
+            DeviceMask::Exact => !0,
+            DeviceMask::Model => !0 << 4,
+            DeviceMask::Vendor => !0 << 10,
+            DeviceMask::Custom(mask) => mask,
+        }
+    }
+}
+
+pub struct DeviceId {
+    pub id: u32,
+    pub mask: DeviceMask,
+}
+
+impl DeviceId {
+    pub const fn new_with_exact_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Exact,
+        }
+    }
+
+    pub const fn new_with_model_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Model,
+        }
+    }
+
+    pub const fn new_with_vendor_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Vendor,
+        }
+    }
+
+    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Custom(mask),
+        }
+    }
+}
+
+/// Defines module device table.
+///
+/// This corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
+/// for module loading into the module binary file.
+#[macro_export]
+macro_rules! phy_module_device_table {
+    (@replace_expr $_t:tt $sub:expr) => {$sub};
+
+    (@count_devices $($x:expr),*) => {
+        0usize $(+ $crate::phy_module_device_table!(@replace_expr $x 1usize))*
+    };
+
+    ($name:ident, [$($t:expr),+]) => {
+        ::kernel::macros::paste! {
+            #[no_mangle]
+            static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::phy_module_device_table!(@count_devices $($t),+) + 1] =
+             [ $(kernel::bindings::mdio_device_id{phy_id: $t.id, phy_id_mask:$t.mask.as_int()}),+, kernel::bindings::mdio_device_id {phy_id:0, phy_id_mask:0}];
+        }
+    }
+}

Re: [RFC PATCH v1 2/4] rust: phy: add module device table support

[Andrew Lunn]

> One driver doesn't always correspond to one device id entry. For
> example, realtek PHY module supports tons of drivers but have only one
> device id entry.
> 
> https://github.com/torvalds/linux/blob/f0b0d403eabbe135d8dbb40ad5e41018947d336c/drivers/net/phy/realtek.c#L1089
> 
> So I prefer to define the device id independently. I use the following
> code now.

Just an FYI: order is important. Matching is performed using a
value:mask pair. The core does not sort on mask size, it assumes the
driver registers the phy_driver structures in order, so that a more
specific value:mask pair comes first followed by more generic
value:mask pairs.

	Andrew

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

[FUJITA Tomonori]

The files are placed at rust/kernel/ directory for now but the files
are likely to be moved to net/ directory if things go well.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..95ecf96b911a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7797,6 +7797,7 @@ F:	include/trace/events/mdio.h
 F:	include/uapi/linux/mdio.h
 F:	include/uapi/linux/mii.h
 F:	net/core/of_net.c
+F:	rust/kernel/net/phy.rs
 
 EXEC & BINFMT API
 R:	Eric Biederman <ebiederm@xmission.com>
-- 
2.34.1

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Andrew Lunn]

On Wed, Sep 13, 2023 at 10:36:08PM +0900, FUJITA Tomonori wrote:
> The files are placed at rust/kernel/ directory for now but the files
> are likely to be moved to net/ directory if things go well.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..95ecf96b911a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7797,6 +7797,7 @@ F:	include/trace/events/mdio.h
>  F:	include/uapi/linux/mdio.h
>  F:	include/uapi/linux/mii.h
>  F:	net/core/of_net.c
> +F:	rust/kernel/net/phy.rs

I would suggest you add yourself as a Maintainer as well. You are
basically stepping up to forever maintaining these files, at least
until Russell, Heiner and I learn Rust.

And to make that maintenance work simpler, you might want to move
phy.rs to drivers/net/phy, so it is alongside phy_device.c,
phy-core.c, and phy.c, the files it needs to track.

	Andrew

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[FUJITA Tomonori]

Hi,

On Wed, 13 Sep 2023 20:57:40 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Sep 13, 2023 at 10:36:08PM +0900, FUJITA Tomonori wrote:
>> The files are placed at rust/kernel/ directory for now but the files
>> are likely to be moved to net/ directory if things go well.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  MAINTAINERS | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 90f13281d297..95ecf96b911a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7797,6 +7797,7 @@ F:	include/trace/events/mdio.h
>>  F:	include/uapi/linux/mdio.h
>>  F:	include/uapi/linux/mii.h
>>  F:	net/core/of_net.c
>> +F:	rust/kernel/net/phy.rs
> 
> I would suggest you add yourself as a Maintainer as well. You are
> basically stepping up to forever maintaining these files, at least
> until Russell, Heiner and I learn Rust.

Happy to do so.

> And to make that maintenance work simpler, you might want to move
> phy.rs to drivers/net/phy, so it is alongside phy_device.c,
> phy-core.c, and phy.c, the files it needs to track.

Works for me.

Miguel, any concerns to put Rust files not under rust/ directory?

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Miguel Ojeda]

On Sun, Sep 17, 2023 at 2:32 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Miguel, any concerns to put Rust files not under rust/ directory?

If it needs to be part of the `kernel` crate, then the file needs to
remain in `rust/kernel` for the time being. The plan is to relax this
restriction in the future and move the files to their respective
places.

Cheers,
Miguel

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Andrew Lunn]

On Tue, Sep 19, 2023 at 02:06:21PM +0200, Miguel Ojeda wrote:
> On Sun, Sep 17, 2023 at 2:32 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > Miguel, any concerns to put Rust files not under rust/ directory?
> 
> If it needs to be part of the `kernel` crate, then the file needs to
> remain in `rust/kernel` for the time being.

So a question from a none Rust person. Why would it need to be in the
'kernel' crate?

The PHY driver is a kernel module, which optionally can be built
in. My maybe overly simplistic expectation is that the kernel runtime
linker will link undefined symbols in the module to symbols which have
already been loaded in the kernel, be they C, Rust, or assembly. The
symbols just need EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL() so they are
visible.

   Andrew

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Trevor Gross]

On Tue, Sep 19, 2023 at 12:34 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> So a question from a none Rust person. Why would it need to be in the
> 'kernel' crate?
>
> The PHY driver is a kernel module, which optionally can be built
> in. My maybe overly simplistic expectation is that the kernel runtime
> linker will link undefined symbols in the module to symbols which have
> already been loaded in the kernel, be they C, Rust, or assembly. The
> symbols just need EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL() so they are
> visible.
>
>    Andrew
>

I think that the abstractions just need to live under `rust/kernel`
because the way to handle dependencies hasn't been decided on. I
believe this is in the works but just not here yet.

The drivers themselves should be able to go anywhere though, kbuild
knows what to do.

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Miguel Ojeda]

On Tue, Sep 19, 2023 at 6:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> So a question from a none Rust person. Why would it need to be in the
> 'kernel' crate?

Sorry, I missed this one.

Rust requires to perform some work on dependencies given there are no
headers. One could definitely put the abstractions elsewhere, but
properly sharing them requires a more involved build system. Thus, for
simplicity, abstractions go for the moment in a single translation
unit (called the `kernel` crate) that gets compiled first.

Like Trevor said, leaf modules that use those abstractions can be
already placed in the usual places.

Cheers,
Miguel

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Andrew Lunn]

On Sat, Sep 23, 2023 at 02:05:45AM +0200, Miguel Ojeda wrote:
> On Tue, Sep 19, 2023 at 6:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > So a question from a none Rust person. Why would it need to be in the
> > 'kernel' crate?
> 
> Sorry, I missed this one.
> 
> Rust requires to perform some work on dependencies given there are no
> headers. One could definitely put the abstractions elsewhere, but
> properly sharing them requires a more involved build system. Thus, for
> simplicity, abstractions go for the moment in a single translation
> unit (called the `kernel` crate) that gets compiled first.

My limited understanding of the Kernel Makefiles is that they all end
up being one huge Makefile. So i think in principle you can place the
files anywhere, and direct any object file towards any target. So i
think you should be able to put the file for the kernel crate anywhere
in the tree.

I would prefer to keep the Rust file next to the C file to make
Maintenance easier. We are less likely to make breaking changes if
everything is kept together in the source tree.

	Andrew

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Miguel Ojeda]

On Sat, Sep 23, 2023 at 3:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> My limited understanding of the Kernel Makefiles is that they all end
> up being one huge Makefile. So i think in principle you can place the
> files anywhere, and direct any object file towards any target. So i
> think you should be able to put the file for the kernel crate anywhere
> in the tree.

Files composing a Rust crate are automatically found by the compiler
following a pattern starting with the root of the translation unit,
i.e. it is unlike C.

It could still be done hacking things around, but we preferred to keep
things clean and then go for the new build system.

> I would prefer to keep the Rust file next to the C file to make
> Maintenance easier. We are less likely to make breaking changes if
> everything is kept together in the source tree.

Definitely, and as I mentioned, that is our plan. There are other
benefits too, e.g. reusing existing patterns in the `MAINTAINERS` file
(i.e. not having to add extra ones just for Rust).

If everything goes well, I would like to have something for LPC.

Cheers,
Miguel

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Andrew Lunn]

On Sat, Sep 23, 2023 at 12:19:06PM +0200, Miguel Ojeda wrote:
> On Sat, Sep 23, 2023 at 3:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > My limited understanding of the Kernel Makefiles is that they all end
> > up being one huge Makefile. So i think in principle you can place the
> > files anywhere, and direct any object file towards any target. So i
> > think you should be able to put the file for the kernel crate anywhere
> > in the tree.
> 
> Files composing a Rust crate are automatically found by the compiler
> following a pattern starting with the root of the translation unit,
> i.e. it is unlike C.
> 
> It could still be done hacking things around, but we preferred to keep
> things clean and then go for the new build system.

O.K. Fine. We can move it later. We seem to be having quite a lot of
useful discussion with this driver, but no new version yet. It might
actually be that by the time it is ready for merging you have the
build system sorted out.

> If everything goes well, I would like to have something for LPC.

I hope to attend LPC, so i will try to pop into some of the sessions.

       Andrew

Re: [RFC PATCH v1 3/4] MAINTAINERS: add Rust PHY abstractions file to the ETHERNET PHY LIBRARY

[Miguel Ojeda]

On Sat, Sep 23, 2023 at 4:42 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I hope to attend LPC, so i will try to pop into some of the sessions.

That would be great -- thanks Andrew (and for the questions you are
asking!). We will be in the Refereed track as well as the Rust MC.

(Catching up on some emails...)

Cheers,
Miguel

[RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[FUJITA Tomonori]

This is the simpler version of drivers/net/phy/ax88796b.c. The C
driver implements three PHY drivers but this implements only one,
AX88772A PHY. This provides educational information for Rust
abstractions for network PHY drivers.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 samples/rust/Kconfig          |  13 +++++
 samples/rust/Makefile         |   1 +
 samples/rust/rust_ax88796b.rs | 105 ++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 samples/rust/rust_ax88796b.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..c1057f0d26f6 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,19 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_NET_AX88796B_PHY
+	tristate "Asix PHYs"
+	help
+	  This is the simpler version of drivers/net/phy/ax88796b.c.
+	  Currently supports only AX88772A PHY. This provides
+	  educational information for Rust abstractions for network PHY drivers.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_print.
+
+	  If unsure, say N.
+
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea4..0f296e1c5efc 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,5 +2,6 @@
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_NET_AX88796B_PHY)	+= rust_ax88796b.o
 
 subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
diff --git a/samples/rust/rust_ax88796b.rs b/samples/rust/rust_ax88796b.rs
new file mode 100644
index 000000000000..0857329d8133
--- /dev/null
+++ b/samples/rust/rust_ax88796b.rs
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust Asix PHYs driver
+use kernel::c_str;
+use kernel::net::phy;
+use kernel::prelude::*;
+
+module! {
+    type: RustAsixPhy,
+    name: "rust_asix_phy",
+    author: "Rust for Linux Contributors",
+    description: "Rust Asix PHYs driver",
+    license: "GPL",
+}
+
+const MII_BMCR: u32 = 0x00;
+const BMCR_SPEED100: u32 = 0x2000;
+const BMCR_FULLDPLX: u32 = 0x0100;
+
+struct PhyAx88772A {}
+
+impl PhyAx88772A {
+    const PHY_ID_ASIX_AX88772A: u32 = 0x003b1861;
+}
+
+#[vtable]
+impl phy::Driver for PhyAx88772A {
+    const FLAGS: u32 = phy::PHY_IS_INTERNAL;
+
+    fn match_phy_device(dev: &mut phy::Device) -> bool {
+        dev.id() == Self::PHY_ID_ASIX_AX88772A
+    }
+
+    fn read_status(dev: &mut phy::Device) -> Result {
+        dev.update_link()?;
+        if !dev.link() {
+            return Ok(());
+        }
+        let ret = dev.read(MII_BMCR)?;
+
+        if ret as u32 & BMCR_SPEED100 != 0 {
+            dev.set_speed(100);
+        } else {
+            dev.set_speed(10);
+        }
+
+        let duplex = ret as u32 & BMCR_FULLDPLX != 0;
+        dev.set_duplex(duplex);
+
+        dev.read_lpa()?;
+
+        if dev.is_autoneg_enabled() && dev.is_autoneg_completed() {
+            dev.resolve_aneg_linkmode();
+        }
+
+        Ok(())
+    }
+
+    fn suspend(dev: &mut phy::Device) -> Result {
+        dev.suspend()
+    }
+
+    fn resume(dev: &mut phy::Device) -> Result {
+        dev.resume()
+    }
+
+    fn soft_reset(dev: &mut phy::Device) -> Result {
+        dev.write(MII_BMCR, 0)?;
+        dev.soft_reset()
+    }
+
+    fn link_change_notify(dev: &mut phy::Device) {
+        if dev.state() == phy::DeviceState::NoLink {
+            let _ = dev.init_hw();
+            let _ = dev.start_aneg();
+        }
+    }
+}
+
+struct RustAsixPhy {
+    _reg: phy::Registration<1>,
+}
+
+impl kernel::Module for RustAsixPhy {
+    fn init(module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust Asix phy driver\n");
+        let mut reg: phy::Registration<1> = phy::Registration::new(module);
+
+        reg.register(&phy::Adapter::<PhyAx88772A>::new(c_str!(
+            "Asix Electronics AX88772A"
+        )))?;
+        Ok(RustAsixPhy { _reg: reg })
+    }
+}
+
+impl Drop for RustAsixPhy {
+    fn drop(&mut self) {
+        pr_info!("Rust Asix phy driver (exit)\n");
+    }
+}
+
+kernel::phy_module_device_table!(
+    __mod_mdio__asix_tbl_device_table,
+    [(PhyAx88772A::PHY_ID_ASIX_AX88772A, 0xffffffff)]
+);
-- 
2.34.1

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> This is the simpler version of drivers/net/phy/ax88796b.c. The C
> driver implements three PHY drivers but this implements only one,
> AX88772A PHY. This provides educational information for Rust
> abstractions for network PHY drivers.

Rather than being a sample, please make it a real driver. Place it in
drivers/net/phy, add a Kconfig option to select it. Maybe drop the
rust_ prefix, since the .rs makes it clear what sort of file it is.

I'm just back from vacation, and have a backlog of patches to review,
so it make take a while before i can look at the details.

> +const MII_BMCR: u32 = 0x00;
> +const BMCR_SPEED100: u32 = 0x2000;
> +const BMCR_FULLDPLX: u32 = 0x0100;

These should be part of the abstraction. 802.3 defines many of the
registers, including the basic mode control register. They should be
identical for all PHYs. So please add an equivalent of
include/uapi/linux/mii.h, or better still, if possible, actually use
the #defines from include/uapi/linux/mii.h.

Also, PHY registers are u16, not u32. And C22 PHY register addresses
are in the range 0-31, so a u8 could be used, or the equivalent of a C
int, whatever the compile thinks is most efficient.

    Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Greg KH]

On Wed, Sep 13, 2023 at 04:11:00PM +0200, Andrew Lunn wrote:
> On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> > This is the simpler version of drivers/net/phy/ax88796b.c. The C
> > driver implements three PHY drivers but this implements only one,
> > AX88772A PHY. This provides educational information for Rust
> > abstractions for network PHY drivers.
> 
> Rather than being a sample, please make it a real driver. Place it in
> drivers/net/phy, add a Kconfig option to select it. Maybe drop the
> rust_ prefix, since the .rs makes it clear what sort of file it is.

But note, we can not have duplicate drivers for the same hardware, so
the .c file needs to have the support for this device removed from it,
OR you need to add support for the other devices to this driver and then
the .c file can be removed.

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

On Wed, Sep 13, 2023 at 06:53:33PM +0200, Greg KH wrote:
> On Wed, Sep 13, 2023 at 04:11:00PM +0200, Andrew Lunn wrote:
> > On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> > > This is the simpler version of drivers/net/phy/ax88796b.c. The C
> > > driver implements three PHY drivers but this implements only one,
> > > AX88772A PHY. This provides educational information for Rust
> > > abstractions for network PHY drivers.
> > 
> > Rather than being a sample, please make it a real driver. Place it in
> > drivers/net/phy, add a Kconfig option to select it. Maybe drop the
> > rust_ prefix, since the .rs makes it clear what sort of file it is.
> 
> But note, we can not have duplicate drivers for the same hardware, so
> the .c file needs to have the support for this device removed from it,
> OR you need to add support for the other devices to this driver and then
> the .c file can be removed.

I would suggest these Rust drivers support all the devices the C
driver does, and there is some kconfig logic to make them mutually
exclusive.  And to keep the build bots happy, kconfig to disable the
driver on architectures Rust does not support yet.

       Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Greg KH]

On Wed, Sep 13, 2023 at 08:50:18PM +0200, Andrew Lunn wrote:
> On Wed, Sep 13, 2023 at 06:53:33PM +0200, Greg KH wrote:
> > On Wed, Sep 13, 2023 at 04:11:00PM +0200, Andrew Lunn wrote:
> > > On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> > > > This is the simpler version of drivers/net/phy/ax88796b.c. The C
> > > > driver implements three PHY drivers but this implements only one,
> > > > AX88772A PHY. This provides educational information for Rust
> > > > abstractions for network PHY drivers.
> > > 
> > > Rather than being a sample, please make it a real driver. Place it in
> > > drivers/net/phy, add a Kconfig option to select it. Maybe drop the
> > > rust_ prefix, since the .rs makes it clear what sort of file it is.
> > 
> > But note, we can not have duplicate drivers for the same hardware, so
> > the .c file needs to have the support for this device removed from it,
> > OR you need to add support for the other devices to this driver and then
> > the .c file can be removed.
> 
> I would suggest these Rust drivers support all the devices the C
> driver does, and there is some kconfig logic to make them mutually
> exclusive.

No, don't do that, it's horrid and we have been down that road in the
past and we don't want to do it again.  One driver per device please.

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> No, don't do that, it's horrid and we have been down that road in the
> past and we don't want to do it again.  One driver per device please.

Has there been any discussion how we handle Rust only currently
supporting a subset of architectures?

We cannot replace the C driver with the Rust version unless it will
build for all architectures we support in C. Otherwise we are going to
cause regressions.

Are you suggesting Rust is only used for new drivers for hardware
which currently has no support, and so only building for a subset of
architectures is actually progress, not a regression?

	Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Greg KH]

On Wed, Sep 13, 2023 at 10:20:17PM +0200, Andrew Lunn wrote:
> > No, don't do that, it's horrid and we have been down that road in the
> > past and we don't want to do it again.  One driver per device please.
> 
> Has there been any discussion how we handle Rust only currently
> supporting a subset of architectures?

Not yet!

> We cannot replace the C driver with the Rust version unless it will
> build for all architectures we support in C. Otherwise we are going to
> cause regressions.

Agreed.

Which is why I would recommend starting out by doing only new features,
not replacing old ones.

> Are you suggesting Rust is only used for new drivers for hardware
> which currently has no support, and so only building for a subset of
> architectures is actually progress, not a regression?

Yes, that is what I would suggest, regressions are bad.

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Greg KH]

On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> +impl kernel::Module for RustAsixPhy {
> +    fn init(module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Rust Asix phy driver\n");

Meta-comment, when drivers are working properly, they are quiet.

Also, no driver should have pr_*() calls in them, they work on devices,
so the normal dev_*() calls should be used instead, if you have to print
anything out.

> +        let mut reg: phy::Registration<1> = phy::Registration::new(module);
> +
> +        reg.register(&phy::Adapter::<PhyAx88772A>::new(c_str!(
> +            "Asix Electronics AX88772A"
> +        )))?;
> +        Ok(RustAsixPhy { _reg: reg })
> +    }
> +}
> +
> +impl Drop for RustAsixPhy {
> +    fn drop(&mut self) {
> +        pr_info!("Rust Asix phy driver (exit)\n");

Again, drivers need to be quiet :)

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

On Wed, Sep 13, 2023 at 06:56:04PM +0200, Greg KH wrote:
> On Wed, Sep 13, 2023 at 10:36:09PM +0900, FUJITA Tomonori wrote:
> > +impl kernel::Module for RustAsixPhy {
> > +    fn init(module: &'static ThisModule) -> Result<Self> {
> > +        pr_info!("Rust Asix phy driver\n");
> 
> Meta-comment, when drivers are working properly, they are quiet.
> 
> Also, no driver should have pr_*() calls in them, they work on devices,
> so the normal dev_*() calls should be used instead, if you have to print
> anything out.

And for this subsystem, they should use phydev_info(phydev, ...).
Underneath that will use dev_info(), using the struct device within
phydev..

	   Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> +module! {
> +    type: RustAsixPhy,
> +    name: "rust_asix_phy",
> +    author: "Rust for Linux Contributors",

You probably want your name here. You are going to be maintaining this
driver.

> +    fn match_phy_device(dev: &mut phy::Device) -> bool {
> +        dev.id() == Self::PHY_ID_ASIX_AX88772A
> +    }

What is this doing? The phy core will match the driver to the device,
based on the phy_id and phy_id_mask in the phy_driver structure. A
match function is only needed when the vendor made a mess of the IDs,
and has two different PHYs with the same ID. I don't see anything in
ax88796b.c to suggest it has this problem.

> +
> +    fn read_status(dev: &mut phy::Device) -> Result {
> +        dev.update_link()?;
> +        if !dev.link() {
> +            return Ok(());
> +        }
> +        let ret = dev.read(MII_BMCR)?;
> +
> +        if ret as u32 & BMCR_SPEED100 != 0 {

PHY registers are u16. phy_read() returns an int, so it can have
negative return codes. But if it is not negative, the positive values
are in the range of 0 to 65535.

> +    fn suspend(dev: &mut phy::Device) -> Result {
> +        dev.suspend()
> +    }
> +
> +    fn resume(dev: &mut phy::Device) -> Result {
> +        dev.resume()
> +    }

Can we avoid this boilerplate? A lot of phy drivers will make use of
genphy_foo calls in their struct phy_driver. It would be annoying if
each driver needs silly little wrappers like this.

> +
> +    fn soft_reset(dev: &mut phy::Device) -> Result {
> +        dev.write(MII_BMCR, 0)?;
> +        dev.soft_reset()
> +    }

You are missing the comment why genphy_soft_reset() is not
enough. This device appears to break 802.3, so you really should
document how it is FUBAR.

> +
> +    fn link_change_notify(dev: &mut phy::Device) {
> +        if dev.state() == phy::DeviceState::NoLink {
> +            let _ = dev.init_hw();
> +            let _ = dev.start_aneg();
> +        }
> +    }

Again, this is bad behaviour of the PHY, so it should be documented
why it is needed.

    Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[FUJITA Tomonori]

Hi,

On Wed, 13 Sep 2023 21:15:53 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +module! {
>> +    type: RustAsixPhy,
>> +    name: "rust_asix_phy",
>> +    author: "Rust for Linux Contributors",
> 
> You probably want your name here. You are going to be maintaining this
> driver.

Will do in the next round.


>> +    fn match_phy_device(dev: &mut phy::Device) -> bool {
>> +        dev.id() == Self::PHY_ID_ASIX_AX88772A
>> +    }
> 
> What is this doing? The phy core will match the driver to the device,
> based on the phy_id and phy_id_mask in the phy_driver structure. A
> match function is only needed when the vendor made a mess of the IDs,
> and has two different PHYs with the same ID. I don't see anything in
> ax88796b.c to suggest it has this problem.

Sorry, I experimented match_phy_device() and I should have removed this.

>> +
>> +    fn read_status(dev: &mut phy::Device) -> Result {
>> +        dev.update_link()?;
>> +        if !dev.link() {
>> +            return Ok(());
>> +        }
>> +        let ret = dev.read(MII_BMCR)?;
>> +
>> +        if ret as u32 & BMCR_SPEED100 != 0 {
> 
> PHY registers are u16. phy_read() returns an int, so it can have
> negative return codes. But if it is not negative, the positive values
> are in the range of 0 to 65535.

A negative is an error?

If so, this function should be:

fn read_status(dev: &mut phy::Device) -> Result<u16>

>> +    fn suspend(dev: &mut phy::Device) -> Result {
>> +        dev.suspend()
>> +    }
>> +
>> +    fn resume(dev: &mut phy::Device) -> Result {
>> +        dev.resume()
>> +    }
> 
> Can we avoid this boilerplate? A lot of phy drivers will make use of
> genphy_foo calls in their struct phy_driver. It would be annoying if
> each driver needs silly little wrappers like this.

you meant that C style like

{
        .suspend        = genphy_suspend,
        .resume         = genphy_resume,
        .soft_reset     = asix_soft_reset,
}

is clear than Rust style

    fn suspend(dev: &mut phy::Device) -> Result {
        dev.genphy_suspend()
    }

    fn resume(dev: &mut phy::Device) -> Result {
        dev.genphy_resume()
    }

right?

>> +
>> +    fn soft_reset(dev: &mut phy::Device) -> Result {
>> +        dev.write(MII_BMCR, 0)?;
>> +        dev.soft_reset()
>> +    }
> 
> You are missing the comment why genphy_soft_reset() is not
> enough. This device appears to break 802.3, so you really should
> document how it is FUBAR.

I'll copy the comment from the C driver.

>> +
>> +    fn link_change_notify(dev: &mut phy::Device) {
>> +        if dev.state() == phy::DeviceState::NoLink {
>> +            let _ = dev.init_hw();
>> +            let _ = dev.start_aneg();
>> +        }
>> +    }
> 
> Again, this is bad behaviour of the PHY, so it should be documented
> why it is needed.

Ditto.

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> >> +    fn read_status(dev: &mut phy::Device) -> Result {
> >> +        dev.update_link()?;
> >> +        if !dev.link() {
> >> +            return Ok(());
> >> +        }
> >> +        let ret = dev.read(MII_BMCR)?;
> >> +
> >> +        if ret as u32 & BMCR_SPEED100 != 0 {
> > 
> > PHY registers are u16. phy_read() returns an int, so it can have
> > negative return codes. But if it is not negative, the positive values
> > are in the range of 0 to 65535.
> 
> A negative is an error?

phy_read() can return values like -ETIMEDOUT, -ENODEV, -EIO etc to
indicate something when wrong. So you should see the general pattern
of:

        foo = phy_read(phydev, MII_M1111_PHY_EXT_SR);
        if (foo < 0)
                return foo;

After probe, the probability of an error is very low, and generally
there is no recovery. So not all drivers do check the return value.

There is also a second C problem here. Because a PHY register is a
u16, you sometimes see code like:

	u16 reg

	reg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	if (reg < 0)
		return reg;

which compiles cleanly, but is wrong.. The folks running static
checkers then report the bug.

> If so, this function should be:
> 
> fn read_status(dev: &mut phy::Device) -> Result<u16>

No, it needs to return the negative error code, or 0 on success. This
is a very typical pattern in Linux.

> 
> >> +    fn suspend(dev: &mut phy::Device) -> Result {
> >> +        dev.suspend()
> >> +    }
> >> +
> >> +    fn resume(dev: &mut phy::Device) -> Result {
> >> +        dev.resume()
> >> +    }
> > 
> > Can we avoid this boilerplate? A lot of phy drivers will make use of
> > genphy_foo calls in their struct phy_driver. It would be annoying if
> > each driver needs silly little wrappers like this.
> 
> you meant that C style like
> 
> {
>         .suspend        = genphy_suspend,
>         .resume         = genphy_resume,
>         .soft_reset     = asix_soft_reset,
> }
> 
> is clear than Rust style
> 
>     fn suspend(dev: &mut phy::Device) -> Result {
>         dev.genphy_suspend()
>     }
> 
>     fn resume(dev: &mut phy::Device) -> Result {
>         dev.genphy_resume()
>     }
> 
> right?

Yes. If you really must have these wrappers, they are not specific to
a driver, so should be placed somewhere common where any driver can
use them. There are around 60 of these genphy_ functions. Because what
a PHY does is mostly standardised in IEEE 802.3, PHY drivers heavily
use these functions to do the core of the work, and then deal with
vendor extensions and vendor bugs.

    Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[FUJITA Tomonori]

Hi,

On Sun, 17 Sep 2023 17:46:39 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> >> +    fn read_status(dev: &mut phy::Device) -> Result {
>> >> +        dev.update_link()?;
>> >> +        if !dev.link() {
>> >> +            return Ok(());
>> >> +        }
>> >> +        let ret = dev.read(MII_BMCR)?;
>> >> +
>> >> +        if ret as u32 & BMCR_SPEED100 != 0 {
>> > 
>> > PHY registers are u16. phy_read() returns an int, so it can have
>> > negative return codes. But if it is not negative, the positive values
>> > are in the range of 0 to 65535.

Sorry, I should have commented in the previous reply. ret can't be a
negative here due to how to hanle an error in Rust.

let ret = dev.read(MII_BMCR)?;

Here '?' means that if an error happens, return with an error
immediately. So the ret is set only if dev.read() is sucessful.


>> A negative is an error?
>
> phy_read() can return values like -ETIMEDOUT, -ENODEV, -EIO etc to
> indicate something when wrong. So you should see the general pattern
> of:
> 
>         foo = phy_read(phydev, MII_M1111_PHY_EXT_SR);
>         if (foo < 0)
>                 return foo;
> 
> After probe, the probability of an error is very low, and generally
> there is no recovery. So not all drivers do check the return value.
> 
> There is also a second C problem here. Because a PHY register is a
> u16, you sometimes see code like:
> 
> 	u16 reg
> 
> 	reg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 	if (reg < 0)
> 		return reg;
> 
> which compiles cleanly, but is wrong.. The folks running static
> checkers then report the bug.
> 
>> If so, this function should be:
>> 
>> fn read_status(dev: &mut phy::Device) -> Result<u16>
> 
> No, it needs to return the negative error code, or 0 on success. This
> is a very typical pattern in Linux.

This is a way to handle an error in Rust. If a function could return
an error, it returns a Result type, which is either success or
failure. Either could carry an value.

Result<u16> means if success, it carrys u16. If failure, in Rust for
Linux, carrys an error code from C side.


>> >> +    fn suspend(dev: &mut phy::Device) -> Result {
>> >> +        dev.suspend()
>> >> +    }
>> >> +
>> >> +    fn resume(dev: &mut phy::Device) -> Result {
>> >> +        dev.resume()
>> >> +    }
>> > 
>> > Can we avoid this boilerplate? A lot of phy drivers will make use of
>> > genphy_foo calls in their struct phy_driver. It would be annoying if
>> > each driver needs silly little wrappers like this.
>> 
>> you meant that C style like
>> 
>> {
>>         .suspend        = genphy_suspend,
>>         .resume         = genphy_resume,
>>         .soft_reset     = asix_soft_reset,
>> }
>> 
>> is clear than Rust style
>> 
>>     fn suspend(dev: &mut phy::Device) -> Result {
>>         dev.genphy_suspend()
>>     }
>> 
>>     fn resume(dev: &mut phy::Device) -> Result {
>>         dev.genphy_resume()
>>     }
>> 
>> right?
> 
> Yes. If you really must have these wrappers, they are not specific to
> a driver, so should be placed somewhere common where any driver can
> use them. There are around 60 of these genphy_ functions. Because what
> a PHY does is mostly standardised in IEEE 802.3, PHY drivers heavily
> use these functions to do the core of the work, and then deal with
> vendor extensions and vendor bugs.

Understood. I think that there are other places like this in the
kernel, operation function pointers that could use generic functions.

Rust people, Any opinions on a different way than vtable macro?

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> >> >> +    fn read_status(dev: &mut phy::Device) -> Result {
> >> >> +        dev.update_link()?;
> >> >> +        if !dev.link() {
> >> >> +            return Ok(());
> >> >> +        }
> >> >> +        let ret = dev.read(MII_BMCR)?;
> >> >> +
> >> >> +        if ret as u32 & BMCR_SPEED100 != 0 {
> >> > 
> >> > PHY registers are u16. phy_read() returns an int, so it can have
> >> > negative return codes. But if it is not negative, the positive values
> >> > are in the range of 0 to 65535.
> 
> Sorry, I should have commented in the previous reply. ret can't be a
> negative here due to how to hanle an error in Rust.
> 
> let ret = dev.read(MII_BMCR)?;
> 
> Here '?' means that if an error happens, return with an error
> immediately. So the ret is set only if dev.read() is sucessful.

Ah, that is going to take a while to get used to. It is also going to
make reviewing the error paths in drivers more interesting. Good
practice is that when something fails, you need to undo what has
already been done in a function, particularly resource allocations. So
you see the pattern:

    err = foo(bar):
    if (err < 0)
       goto out_foo:

Where the code at out_foo: undoes anything which has happened so far.

      Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[FUJITA Tomonori]

Hi, sorry about the delay,

On Mon, 18 Sep 2023 15:37:12 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> >> >> +    fn read_status(dev: &mut phy::Device) -> Result {
>> >> >> +        dev.update_link()?;
>> >> >> +        if !dev.link() {
>> >> >> +            return Ok(());
>> >> >> +        }
>> >> >> +        let ret = dev.read(MII_BMCR)?;
>> >> >> +
>> >> >> +        if ret as u32 & BMCR_SPEED100 != 0 {
>> >> > 
>> >> > PHY registers are u16. phy_read() returns an int, so it can have
>> >> > negative return codes. But if it is not negative, the positive values
>> >> > are in the range of 0 to 65535.
>> 
>> Sorry, I should have commented in the previous reply. ret can't be a
>> negative here due to how to hanle an error in Rust.
>> 
>> let ret = dev.read(MII_BMCR)?;
>> 
>> Here '?' means that if an error happens, return with an error
>> immediately. So the ret is set only if dev.read() is sucessful.
> 
> Ah, that is going to take a while to get used to. It is also going to
> make reviewing the error paths in drivers more interesting. Good
> practice is that when something fails, you need to undo what has
> already been done in a function, particularly resource allocations. So
> you see the pattern:
> 
>     err = foo(bar):
>     if (err < 0)
>        goto out_foo:
> 
> Where the code at out_foo: undoes anything which has happened so far.

Indeed, this is an interesting topic; different from C's way.

fn probe(dev: &mut Device) -> Result {
   // heap memory allocation; kmalloc
   let a = Box::try_new(MyDeviceData::new())?;

   foo(dev)?;

   dev.driver_data = a;

   Ok(())
}

With the above example, if foo() fails, this function returns
immediately due to '?'. When this function ends, 'a' is dropped since
nobody has reference to it. The destructor of 'a' calls kfree().

fn probe(dev: &mut Device) -> Result {
   // register the device.
   let reg = Registration(dev)?;

   foo(dev)?;

   dev.driver_data = reg;

   Ok(())
}

If you register something and foo() fails, the destructor of
Registraiton object needs to unregister the device.

The registration in the abstractions for PHY drivers is designed in
the same way. The destructor of the registration struct calls
phy_drivers_unregister().

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Miguel Ojeda]

On Sun, Sep 24, 2023 at 11:12 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> With the above example, if foo() fails, this function returns
> immediately due to '?'. When this function ends, 'a' is dropped since
> nobody has reference to it. The destructor of 'a' calls kfree().

I think the confusion may be the "immediately" word -- perhaps Andrew
understood that word implies no other code being run (like `return
-EFOO;` in C).

To clarify further, here is a side-by-side live demo:

    https://godbolt.org/z/fYYvo6a1v

i.e. a function like:

    int f(void)
    {
        int ret;
        struct A a;
        struct B b;

        ret = get_a(&a);
        if (ret < 0)
            goto err;

        ret = get_b(&b);
        if (ret < 0)
            goto err_free_a;

        printf("ok");
        return 0;

    err_free_a:
        puts("free a");

    err:
        return ret;
    }

may look like:

    fn f() -> Result<(), i32> {
        let a = get_a()?;
        let b = get_b()?;
        println!("ok");
        Ok(())
    }

Cheers,
Miguel

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> > Ah, that is going to take a while to get used to. It is also going to
> > make reviewing the error paths in drivers more interesting. Good
> > practice is that when something fails, you need to undo what has
> > already been done in a function, particularly resource allocations. So
> > you see the pattern:
> > 
> >     err = foo(bar):
> >     if (err < 0)
> >        goto out_foo:
> > 
> > Where the code at out_foo: undoes anything which has happened so far.
> 
> Indeed, this is an interesting topic; different from C's way.
> 
> fn probe(dev: &mut Device) -> Result {
>    // heap memory allocation; kmalloc
>    let a = Box::try_new(MyDeviceData::new())?;
> 
>    foo(dev)?;
> 
>    dev.driver_data = a;
> 
>    Ok(())
> }
> 
> With the above example, if foo() fails, this function returns
> immediately due to '?'. When this function ends, 'a' is dropped since
> nobody has reference to it. The destructor of 'a' calls kfree().

This works for resources which are simply allocated and freed,
assuming order it kept.

But not all resources are allocated and go out of scope on error. Just
picking a simple example:

static int __maybe_unused fec_runtime_resume(struct device *dev)
{
	struct net_device *ndev = dev_get_drvdata(dev);
	struct fec_enet_private *fep = netdev_priv(ndev);
	int ret;

	ret = clk_prepare_enable(fep->clk_ahb);
	if (ret)
		return ret;
	ret = clk_prepare_enable(fep->clk_ipg);
	if (ret)
		goto failed_clk_ipg;

	return 0;

failed_clk_ipg:
	clk_disable_unprepare(fep->clk_ahb);
	return ret;
}

If clk_ipg cannot be enabled, the error path undoes the enabling of
the clk_ahb. 

So clk_prepare_enable(fep->clk_ahb) could have a ? postfix. But at the
moment, it is not obvious to me how clk_prepare_enable(fep->clk_ipg)
can have a ?

    Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Miguel Ojeda]

On Sun, Sep 24, 2023 at 5:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> But not all resources are allocated and go out of scope on error. Just
> picking a simple example:
>
> (...)
>
> So clk_prepare_enable(fep->clk_ahb) could have a ? postfix. But at the
> moment, it is not obvious to me how clk_prepare_enable(fep->clk_ipg)
> can have a ?

If you consider "preparedness" as a resource in your example, then in
your function both error paths "remove" that resource. In other words,
it is the same as the example I showed above, i.e. "free a" is the
`clk_disable_unprepare(fep->clk_ahb);` operation.

However, what is different between my example and your last one is the
success path. In yours, I think you want to keep both resources around
(i.e. to not undo the preparation). In mine, `a` and `b` are dropped
instead:

    fn f() -> Result<(), i32> {
        let a = get_a()?; // If failed, return (nothing is dropped).
        let b = get_b()?; // If failed, drop `a` and return.
        println!("ok");
        Ok(())
        // Drop `b`.
        // Drop `a`.
    }

    https://godbolt.org/z/xT1MrshaP

Now, if one wants to keep those resources around, there are different
alternatives (that still allow to use `?`) depending on the case.
Typically, one could return a resource to the caller or keep it
somewhere else, thus it is not dropped by `f`, e.g.
https://godbolt.org/z/9zoco9Mxs

Or, if instead of `get_*()` functions that return a "resource" one has
`do_something_with_*()` functions (i.e. they don't return a "resource"
by themselves), then what one can do is, for instance, use the
`ScopeGuard` type to define an operation to be undone in certain
paths, e.g. https://godbolt.org/z/sa8YzK45G

All those allow to still use the `?` operator, but of course, one can
write the logic by hand too, and in some cases, it could be more
convenient.

Cheers,
Miguel

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Andrew Lunn]

> All those allow to still use the `?` operator, but of course, one can
> write the logic by hand too, and in some cases, it could be more
> convenient.

As somebody who does a lot of code reviews, i like C simplicity for
this. I don't need to know for each method if it has some implicit
undo when something else fails. I just need to see there is a full
collection of goto out; statements calling what looks like reasonable
undo methods.

This could be an interesting research project in a few years
time. Which way results in less bugs in kernel code error paths, and
how does reviews affect this.

	Andrew

Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[Miguel Ojeda]

On Sun, Sep 24, 2023 at 7:44 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> As somebody who does a lot of code reviews, i like C simplicity for
> this. I don't need to know for each method if it has some implicit
> undo when something else fails. I just need to see there is a full
> collection of goto out; statements calling what looks like reasonable
> undo methods.

Yeah, it is a bit shocking at first.

The key here is that, in C, you have to write all those jumps, calling
the right functions and in the right order. Not only that, you have to
do so for all functions using a particular resource.

Worse, you need to maintain that over time. And a bug here easily
leads to a vulnerability or a leak. Even "trivial" refactors can
introduce mistakes.

In Rust, the equivalent function can be way shorter, like in the `f()`
example above. To drive this home, an important use case of Rust in
the kernel showed in Kangrejos a week ago an entire page of kernel
cleanup code in a complex function getting reduced to a single
character, `}`. That is very powerful.

As you say, an study of C vs. Rust kernel code in a few years will be
very interesting. Although, note that destructors are just part of the
whole package, e.g. the fact that these implicit calls can be part of
safe code is what makes things very interesting (e.g. unlike C++).

Cheers,
Miguel

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Trevor Gross]

Some meta design comments - why does registering need to allocate at
all? We should be able to keep the `phy_driver` in statics, like the C
side does.

Also, the size-generic `Registration` seems more complex that is
needed, which Benno was getting at. The C side already provides
`phy_drivers_register` and `phy_drivers_unregister` (drivers plural)
that can handle >1, so I don't think we need to create anything
special here.

I think the API could be simplified quite a bit with these things in
mind. A `Driver` can probably take over the `phy_id`, `phy_id_mask`,
and `name` fields which will more closely mirror `phy_driver` [1]:

    /// Used to do the same thing as `PHY_ID_MATCH_{EXACT, MODEL, VENDOR}`
    enum PhyIdMask {
        Exact,
        Model,
        Vendor,
        Custom(u32)
    }

    trait Driver {
        const PHY_ID: u32,
        const PHY_ID_MASK: PhyIdMask,
        const NAME: &'static CStr;
        // ...
    }

Then you need two const functions that can construct the
`mdio_device_id` table and the `phy_driver` structs (const functions
or macros, but I think functions are easier)

    const fn make_mdio_id<T: Driver>() -> Opaque<bindings::mdio_device_id> {
        let phy_id_mask = /* copy logic from existing C macros */;
        bindings::mdio_device_id { phy_id: T::PHY_ID, phy_id_mask }
    }

    const fn make_phy_device<T: Driver>() -> Opaque<bindings::phy_device> {
        // take this implementation from Adapter::driver
    }

And a type that can be used to unregister the devices on drop. This
doesn't need to be generic over N devices since it only ever needs to
call a single function.

    struct RegisteredHandle {
        unregister: unsafe fn()
    }

    impl Drop for RegisteredHandle {
        fn drop(&mut self) {
            // SAFETY: cannot be called more than once
            unsafe { self.0() };
        }
    }

Then, usage looks like this:

    struct RustAsixPhy {
        _reg: phy::RegisteredHandle,
    }

    impl kernel::Module for RustAsixPhy {
        fn init(module: &'static ThisModule) -> Result<Self> {
            pr_info!("Rust Asix phy driver\n");
            Ok(Self { _reg: register_fn_name()? } )
        }
    }

    // Takes one or more `impl Driver` structs. `modname` would just
be `asix` in real use
    phy_drivers!(modname, register_fn_name,  AsixAx8872a, AsixAx8872c,
AsixAc88796b);

The macro will expand to all of the below:

    /// A function that the user will call to register these modules.
Returns a handle that
    /// will unregister them on drop
    fn register_fn_name(module: &'static ThisModule) ->
Result<RegisteredHandle> {
        // We can use the existing functions that register multiple phys at once
        bindings::phy_drivers_register(
            &__modname_drivers,
            __modname_drivers.len(),
            module
        )?; // <- actually map this error...

        Ok(RegisteredHandle{
            unregister: || bindings::phy_drivers_unregister(
                &__modname_drivers, __modname_drivers.len()
            )
        })
    }

    /// Our `phy_device`s are stored here rather than in an allocation
    /// Don't need to be `#[no_mangle]` since C will never see this directly
    /// Does need to be mutable based on Andrew's comments
    pub static __modname_drivers: [Opaque<bindings::phy_device>; 3] = [
        make_phy_device::<AsixAx8872a>(),
        make_phy_device::<AsixAx8872c>(),
        make_phy_device::<AsixAx88796b>(),
    ];

    /// This will be the same as what exists - I assume this is read by
    /// the C side somehow?
    #[no_mangle]
    pub static __mod_mdio__modname_device_table:
        [Opaque<bindings::mdio_device_id>; 4] = [
        make_mdio_id::<AsixAx8872a>(),
        make_mdio_id::<AsixAx8872c>(),
        make_mdio_id::<AsixAx88796b>(),
        bindings::mdio_device_id { phy_id: 0, phy_id_mask: 0 }
    ];

    /// Bonus: we should be able to write a function that verifies the
phy IDs and
    /// their masks can actually all get matched
    const _: () = assert_correct_phy_ordering(__mod_mdio__modname_device_table);

This eliminates the need for allocation, a const-generic registration
struct, and the `Adapter` type (the associated functions on `Adapter`
could probably move into a `mod adapters` or similar). And
registration gets a bit more straightforward. I think this is a lot
more similar to what happens on the C side.

Any reasons the above wouldn't work?

[1]: @Fujita I know you have mentioned there are times where a driver
does not always have one mask. I can't find anything and am not sure
how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
fields - do you mind linking an example? The above could probably be
adjusted to work with this

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Andrew Lunn]

>     impl kernel::Module for RustAsixPhy {
>         fn init(module: &'static ThisModule) -> Result<Self> {
>             pr_info!("Rust Asix phy driver\n");

Another FYI from the kernel best practices side of things. Don't spam
the kernel log like this when a driver starts. Drivers are supposed to
be silent, unless things go wrong.

>     /// This will be the same as what exists - I assume this is read by
>     /// the C side somehow?
>     #[no_mangle]
>     pub static __mod_mdio__modname_device_table:
>         [Opaque<bindings::mdio_device_id>; 4] = [
>         make_mdio_id::<AsixAx8872a>(),
>         make_mdio_id::<AsixAx8872c>(),
>         make_mdio_id::<AsixAx88796b>(),
>         bindings::mdio_device_id { phy_id: 0, phy_id_mask: 0 }
>     ];

Actually, No. This ends up in a special section in the module .ko
file. The tool depmod(8) then reads that section and uses it to build
/lib/modules/<version>/modules.alias

See:

https://wiki.archlinux.org/title/Modalias

> [1]: @Fujita I know you have mentioned there are times where a driver
> does not always have one mask. I can't find anything and am not sure
> how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
> fields - do you mind linking an example? The above could probably be
> adjusted to work with this

drivers/met/phy/meson-gxl.c does some funky things:

static struct phy_driver meson_gxl_phy[] = {
        {
                PHY_ID_MATCH_EXACT(0x01814400),
                .name           = "Meson GXL Internal PHY",
                /* PHY_BASIC_FEATURES */
                .flags          = PHY_IS_INTERNAL,
                .soft_reset     = genphy_soft_reset,
                .config_init    = meson_gxl_config_init,
                .read_status    = meson_gxl_read_status,
                .config_intr    = smsc_phy_config_intr,
                .handle_interrupt = smsc_phy_handle_interrupt,
                .suspend        = genphy_suspend,
                .resume         = genphy_resume,
                .read_mmd       = genphy_read_mmd_unsupported,
                .write_mmd      = genphy_write_mmd_unsupported,
        }, {
                PHY_ID_MATCH_EXACT(0x01803301),
                .name           = "Meson G12A Internal PHY",
                /* PHY_BASIC_FEATURES */
                .flags          = PHY_IS_INTERNAL,
                .probe          = smsc_phy_probe,
                .config_init    = smsc_phy_config_init,
                .soft_reset     = genphy_soft_reset,
                .read_status    = lan87xx_read_status,
                .config_intr    = smsc_phy_config_intr,
                .handle_interrupt = smsc_phy_handle_interrupt,

                .get_tunable    = smsc_phy_get_tunable,
                .set_tunable    = smsc_phy_set_tunable,

                .suspend        = genphy_suspend,
                .resume         = genphy_resume,
                .read_mmd       = genphy_read_mmd_unsupported,
                .write_mmd      = genphy_write_mmd_unsupported,
        },
};

static struct mdio_device_id __maybe_unused meson_gxl_tbl[] = {
        { PHY_ID_MATCH_VENDOR(0x01814400) },
        { PHY_ID_MATCH_VENDOR(0x01803301) },
        { }

So the phy_driver wants an exact match, but the module is loaded based
on the vendor OUIs.

icplus.c:

static struct mdio_device_id __maybe_unused icplus_tbl[] = {
        { PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
        { PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
        { PHY_ID_MATCH_EXACT(IP101A_PHY_ID) },
        { }
};

The devices seem to not have unique IDs, so:

static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
{
        int ret;

        if (phydev->phy_id != IP101A_PHY_ID)
                return 0;

        /* The IP101A and the IP101G share the same PHY identifier.The IP101G
         * seems to be a successor of the IP101A and implements more functions.
         * Amongst other things there is a page select register, which is not
         * available on the IP101A. Use this to distinguish these two.
         */
        ret = ip101a_g_has_page_register(phydev);
        if (ret < 0)
                return ret;

        return ip101a == !ret;
}

Its a variant on Murphy's law. If vendors can get something wrong,
some vendor will get it wrong at some point.

     Andrew

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Trevor Gross]

On Mon, Sep 18, 2023 at 6:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> >     impl kernel::Module for RustAsixPhy {
> >         fn init(module: &'static ThisModule) -> Result<Self> {
> >             pr_info!("Rust Asix phy driver\n");
>
> Another FYI from the kernel best practices side of things. Don't spam
> the kernel log like this when a driver starts. Drivers are supposed to
> be silent, unless things go wrong.

I just copied this from the original, thanks for pointing it out here

> >     /// This will be the same as what exists - I assume this is read by
> >     /// the C side somehow?
> >     #[no_mangle]
> >     pub static __mod_mdio__modname_device_table:
> > [ ...]
>
> Actually, No. This ends up in a special section in the module .ko
> file. The tool depmod(8) then reads that section and uses it to build
> /lib/modules/<version>/modules.alias
>
> See:
>
> https://wiki.archlinux.org/title/Modalias

I wasn't sure whether we needed to reference this somewhere in one of
the registration structs, appreciate the clarification.

> > [1]: @Fujita I know you have mentioned there are times where a driver
> > does not always have one mask. I can't find anything and am not sure
> > how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
> > fields - do you mind linking an example? The above could probably be
> > adjusted to work with this
>
> drivers/met/phy/meson-gxl.c does some funky things:
>
> static struct phy_driver meson_gxl_phy[] = {
>         {
>                 PHY_ID_MATCH_EXACT(0x01814400),
>                 .name           = "Meson GXL Internal PHY",
> [ ... ]
>         }, {
>                 PHY_ID_MATCH_EXACT(0x01803301),
>                 .name           = "Meson G12A Internal PHY",
> [ ... ]
>         },
> };
>
> static struct mdio_device_id __maybe_unused meson_gxl_tbl[] = {
>         { PHY_ID_MATCH_VENDOR(0x01814400) },
>         { PHY_ID_MATCH_VENDOR(0x01803301) },
>         { }
>
> So the phy_driver wants an exact match, but the module is loaded based
> on the vendor OUIs.
>
> icplus.c:
>
> static struct mdio_device_id __maybe_unused icplus_tbl[] = {
>         { PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
>         { PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
>         { PHY_ID_MATCH_EXACT(IP101A_PHY_ID) },
>         { }
> };
>
> The devices seem to not have unique IDs, so:
>
> static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
> {
> [ ... ]
> }
>
> Its a variant on Murphy's law. If vendors can get something wrong,
> some vendor will get it wrong at some point.
>
>      Andrew

Thanks for the references here, I see I misunderstood exactly the
problem was. In this case, maybe we could do something like this:

    trait Driver {
        const PHY_ID: u32,
        const PHY_ID_MASK: PhyIdMask,
        // Default to the same value
        const PHY_DEVICE_ID_MASK: PhyIdMask = Self::PHY_ID_MASK,
        const NAME: &'static CStr;
        // ...
    }

`PHY_ID_MASK` would be used to create the `phy_driver`,
`PHY_DEVICE_ID_MASK`  creates the `mdio_device`. That can be
overridden in cases like the Meson G where they aren't the same.

I think that the icplus situation is already possible with `match_phy_device`

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

Hi,

On Mon, 18 Sep 2023 19:46:57 -0400
Trevor Gross <tmgross@umich.edu> wrote:

>> > [1]: @Fujita I know you have mentioned there are times where a driver
>> > does not always have one mask. I can't find anything and am not sure
>> > how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
>> > fields - do you mind linking an example? The above could probably be
>> > adjusted to work with this
>>
>> drivers/met/phy/meson-gxl.c does some funky things:
>>
>> static struct phy_driver meson_gxl_phy[] = {
>>         {
>>                 PHY_ID_MATCH_EXACT(0x01814400),
>>                 .name           = "Meson GXL Internal PHY",
>> [ ... ]
>>         }, {
>>                 PHY_ID_MATCH_EXACT(0x01803301),
>>                 .name           = "Meson G12A Internal PHY",
>> [ ... ]
>>         },
>> };
>>
>> static struct mdio_device_id __maybe_unused meson_gxl_tbl[] = {
>>         { PHY_ID_MATCH_VENDOR(0x01814400) },
>>         { PHY_ID_MATCH_VENDOR(0x01803301) },
>>         { }
>>
>> So the phy_driver wants an exact match, but the module is loaded based
>> on the vendor OUIs.
>>
>> icplus.c:
>>
>> static struct mdio_device_id __maybe_unused icplus_tbl[] = {
>>         { PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
>>         { PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
>>         { PHY_ID_MATCH_EXACT(IP101A_PHY_ID) },
>>         { }
>> };
>>
>> The devices seem to not have unique IDs, so:
>>
>> static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
>> {
>> [ ... ]
>> }
>>
>> Its a variant on Murphy's law. If vendors can get something wrong,
>> some vendor will get it wrong at some point.
>>
>>      Andrew
> 
> Thanks for the references here, I see I misunderstood exactly the
> problem was. In this case, maybe we could do something like this:
> 
>     trait Driver {
>         const PHY_ID: u32,
>         const PHY_ID_MASK: PhyIdMask,
>         // Default to the same value
>         const PHY_DEVICE_ID_MASK: PhyIdMask = Self::PHY_ID_MASK,
>         const NAME: &'static CStr;
>         // ...
>     }
> 
> `PHY_ID_MASK` would be used to create the `phy_driver`,
> `PHY_DEVICE_ID_MASK`  creates the `mdio_device`. That can be
> overridden in cases like the Meson G where they aren't the same.
> 
> I think that the icplus situation is already possible with `match_phy_device`

drivers/net/phy/adin1100.c

static struct phy_driver adin_driver[] = {
	{
		.phy_id			= PHY_ID_ADIN1100,
		.phy_id_mask		= 0xffffffcf,
		.name			= "ADIN1100",
		.get_features		= adin_get_features,
		.soft_reset		= adin_soft_reset,
		.probe			= adin_probe,
		.config_aneg		= adin_config_aneg,
		.read_status		= adin_read_status,
		.set_loopback		= adin_set_loopback,
		.suspend		= adin_suspend,
		.resume			= adin_resume,
		.get_sqi		= adin_get_sqi,
		.get_sqi_max		= adin_get_sqi_max,
	},
};

module_phy_driver(adin_driver);

static struct mdio_device_id __maybe_unused adin_tbl[] = {
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1100) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1110) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN2111) },
	{ }
};

This registers one driver but registers three mdio_device_ids.

bcm87xx.c registers two drivers but registers no
mdio_device_id. realtek.c registers 10 drivers but only one
mdio_device_id. There are some drivers that do unique.

I think that to separate registations of drivers and mdio_device_id is
the cleanest way.

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Trevor Gross]

On Tue, Sep 19, 2023 at 2:24 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> This registers one driver but registers three mdio_device_ids.
>
> bcm87xx.c registers two drivers but registers no
> mdio_device_id. realtek.c registers 10 drivers but only one
> mdio_device_id. There are some drivers that do unique.
>
> I think that to separate registations of drivers and mdio_device_id is
> the cleanest way.

Thanks for pointing it out, agreed in that case.

Some sort of `struct MdioDeviceId { phy_id: ..., phy_id_mask: ... }`
might still be useful. Maybe the `Driver` could have an
`Option<MdioDevice>` that gets set in the `phy_device` if that exists?

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Andrew Lunn]

> bcm87xx.c registers two drivers but registers no
> mdio_device_id.

Which is odd. I suspect this driver cannot be loaded by hotplug as a
module. It is probably only ever used built in.

But i agree with you about the principle, struct phy_device and struct
mdio_device_id have a very loose coupling.

	Andrew

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[FUJITA Tomonori]

Hi,

On Mon, 18 Sep 2023 18:23:23 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> Then, usage looks like this:
> 
>     struct RustAsixPhy {
>         _reg: phy::RegisteredHandle,
>     }
> 
>     impl kernel::Module for RustAsixPhy {
>         fn init(module: &'static ThisModule) -> Result<Self> {
>             pr_info!("Rust Asix phy driver\n");
>             Ok(Self { _reg: register_fn_name()? } )
>         }
>     }
> 
>     // Takes one or more `impl Driver` structs. `modname` would just
> be `asix` in real use
>     phy_drivers!(modname, register_fn_name,  AsixAx8872a, AsixAx8872c,
> AsixAc88796b);
> 
> The macro will expand to all of the below:

This code will be executed in the module, right?

>     /// A function that the user will call to register these modules.
> Returns a handle that
>     /// will unregister them on drop
>     fn register_fn_name(module: &'static ThisModule) ->
> Result<RegisteredHandle> {
>         // We can use the existing functions that register multiple phys at once
>         bindings::phy_drivers_register(
>             &__modname_drivers,
>             __modname_drivers.len(),
>             module
>         )?; // <- actually map this error...

module.0 is necessary instead of module, which is private.

>         Ok(RegisteredHandle{
>             unregister: || bindings::phy_drivers_unregister(
>                 &__modname_drivers, __modname_drivers.len()
>             )
>         })
>     }
> 
>     /// Our `phy_device`s are stored here rather than in an allocation
>     /// Don't need to be `#[no_mangle]` since C will never see this directly
>     /// Does need to be mutable based on Andrew's comments
>     pub static __modname_drivers: [Opaque<bindings::phy_device>; 3] = [
>         make_phy_device::<AsixAx8872a>(),
>         make_phy_device::<AsixAx8872c>(),
>         make_phy_device::<AsixAx88796b>(),
>     ];

static needs `Sync` but Opaque isn't `Sync`?

We could use `static mut` here but it needs unsafe. So a macro could
generate something like:  

static mut DRIVERS: [Opaque<bindings::phy_driver>; 2] = [
    phy::Adapter::<PhyAX88772A>::make_phy_driver(),
    phy::Adapter::<PhyAX88772C>::make_phy_driver(),
];
	
impl kernel::Module for RustAsixPhy {
    fn init(module: &'static ThisModule) -> Result<Self> {
        let mut reg: phy::Registration = phy::Registration::new(module);
        unsafe {
            reg.register(&DRIVERS)?;
        }
        Ok(RustAsixPhy { _reg: reg })
    }
}
					    
I guess that module! macro generates some unsafe code so we can use
such macros?

Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Trevor Gross]

On Tue, Sep 19, 2023 at 2:16 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Hi,
>
> On Mon, 18 Sep 2023 18:23:23 -0400
> Trevor Gross <tmgross@umich.edu> wrote:
>
> > Then, usage looks like this:
> >
> >     struct RustAsixPhy {
> >         _reg: phy::RegisteredHandle,
> >     }
> >
> >     impl kernel::Module for RustAsixPhy {
> >         fn init(module: &'static ThisModule) -> Result<Self> {
> >             pr_info!("Rust Asix phy driver\n");
> >             Ok(Self { _reg: register_fn_name()? } )
> >         }
> >     }
> >
> >     // Takes one or more `impl Driver` structs. `modname` would just
> > be `asix` in real use
> >     phy_drivers!(modname, register_fn_name,  AsixAx8872a, AsixAx8872c,
> > AsixAc88796b);
> >
> > The macro will expand to all of the below:
>
> This code will be executed in the module, right?

Right - I think we could have one macro that creates the `static
struct phy_driver` and the `MODULE_DEVICE_TABLE` used on the C side.

> >     /// Our `phy_device`s are stored here rather than in an allocation
> >     /// Don't need to be `#[no_mangle]` since C will never see this directly
> >     /// Does need to be mutable based on Andrew's comments
> >     pub static __modname_drivers: [Opaque<bindings::phy_device>; 3] = [
> >         make_phy_device::<AsixAx8872a>(),
> >         make_phy_device::<AsixAx8872c>(),
> >         make_phy_device::<AsixAx88796b>(),
> >     ];
>
> static needs `Sync` but Opaque isn't `Sync`?
>
> We could use `static mut` here but it needs unsafe. So a macro could
> generate something like:
>
> static mut DRIVERS: [Opaque<bindings::phy_driver>; 2] = [
>     phy::Adapter::<PhyAX88772A>::make_phy_driver(),
>     phy::Adapter::<PhyAX88772C>::make_phy_driver(),
> ];
>
> impl kernel::Module for RustAsixPhy {
>     fn init(module: &'static ThisModule) -> Result<Self> {
>         let mut reg: phy::Registration = phy::Registration::new(module);
>         unsafe {
>             reg.register(&DRIVERS)?;
>         }
>         Ok(RustAsixPhy { _reg: reg })
>     }
> }
>
> I guess that module! macro generates some unsafe code so we can use
> such macros?

Good catch with the static - I think you can just do

    /// SAFETY: `phy_driver` is designed to be shared and provides its own
    /// synchronization on needed fields
    unsafe impl Sync for Opaque<bindings::phy_driver> {}

Or make a wrapper type but that's not needed if we never refer to it.

Registration can be safe since the only precondition to
`phy_drivers_register` is valid pointers and correct length. Since
registration is only doing one thing, it can be one function instead
of splitting into `new()` and `register(...)`.