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

Commit Message

FUJITA Tomonori Oct. 9, 2023, 1:39 a.m. UTC

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

This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.

Signed-off-by: FUJITA Tomonori <[e-mail]>
---
 init/Kconfig                    |   8 +
 rust/Makefile                   |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 733 ++++++++++++++++++++++++++++++++
 6 files changed, 754 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

Comments

Trevor Gross Oct. 9, 2023, 3:17 a.m. UTC | #1

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

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

Reviewed-by: Trevor Gross <[e-mail]>

Benno Lossin Oct. 9, 2023, 12:19 p.m. UTC | #2

On 09.10.23 03:39, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.
> 
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> ---
>   init/Kconfig                    |   8 +
>   rust/Makefile                   |   1 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 733 ++++++++++++++++++++++++++++++++
>   6 files changed, 754 insertions(+)
>   create mode 100644 rust/kernel/net.rs
>   create mode 100644 rust/kernel/net/phy.rs
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 6d35728b94b2..7ea415c9b144 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1903,6 +1903,14 @@ config RUST
> 
>   	  If unsure, say N.
> 
> +config RUST_PHYLIB_BINDINGS
> +        bool "PHYLIB bindings support"
> +        depends on RUST
> +        depends on PHYLIB=y
> +        help
> +          Adds support needed for PHY drivers written in Rust. It provides
> +          a wrapper around the C phlib core.
> +
>   config RUSTC_VERSION_TEXT
>   	string
>   	depends on RUST
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
>         cmd_bindgen = \
>   	$(BINDGEN) $< $(bindgen_target_flags) \
>   		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> +		--rustified-enum phy_state\
>   		--no-debug '.*' \
>   		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
>   		$(bindgen_target_cflags) $(bindgen_target_extra)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
> 
>   #include <kunit/test.h>
>   #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
>   #include <linux/slab.h>
>   #include <linux/refcount.h>
>   #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
>   #![no_std]
>   #![feature(allocator_api)]
>   #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
>   #![feature(dispatch_from_dyn)]
>   #![feature(new_uninit)]
>   #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
>   pub mod ioctl;
>   #[cfg(CONFIG_KUNIT)]
>   pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>   pub mod prelude;
>   pub mod print;
>   mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..cc1de17cd5fa
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_RUST_PHYLIB_BINDINGS)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..f31983bf0460
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,733 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +}
> +
> +/// Represents duplex mode.
> +pub enum DuplexMode {
> +    /// Full-duplex mode
> +    Half,
> +    /// Half-duplex mode
> +    Full,
> +    /// Unknown
> +    Unknown,
> +}
> +
> +/// An instance of a PHY device.
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> +    /// may read or write to the `phy_device` object.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        unsafe { &mut *ptr.cast() }

Missing `SAFETY` comment.

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

I would call this function `is_link_up`.

> +        const LINK_IS_UP: u32 = 1;
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).link() == LINK_IS_UP }

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

> +    }
> +
> +    /// Returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&mut self) -> bool {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
> +    }
> +
> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&mut self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).autoneg_complete() == AUTONEG_COMPLETED }
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&mut self, speed: u32) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).speed = speed as i32 };
> +    }
> +
> +    /// Sets duplex mode.
> +    pub fn set_duplex(&mut self, mode: DuplexMode) {
> +        let phydev = self.0.get();
> +        let v = match mode {
> +            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
> +            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
> +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
> +        };
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).duplex = v };
> +    }
> +
> +    /// Reads a given C22 PHY register.
> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe {
> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> +        };

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

> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Writes a given C22 PHY register.
> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe {
> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> +        })
> +    }
> +
> +    /// Reads a paged register.
> +    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Resolves the advertisements into PHY settings.
> +    pub fn resolve_aneg_linkmode(&mut self) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
> +    }
> +
> +    /// Executes software reset the PHY via BMCR_RESET bit.
> +    pub fn genphy_soft_reset(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
> +    }
> +
> +    /// Initializes the PHY.
> +    pub fn init_hw(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // so an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::phy_init_hw(phydev) })
> +    }
> +
> +    /// Starts auto-negotiation.
> +    pub fn start_aneg(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::phy_start_aneg(phydev) })
> +    }
> +
> +    /// Resumes the PHY via BMCR_PDOWN bit.
> +    pub fn genphy_resume(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_resume(phydev) })
> +    }
> +
> +    /// Suspends the PHY via BMCR_PDOWN bit.
> +    pub fn genphy_suspend(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_suspend(phydev) })
> +    }
> +
> +    /// Checks the link status and updates current link state.
> +    pub fn genphy_read_status(&mut self) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::genphy_read_status(phydev) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Updates the link status.
> +    pub fn genphy_update_link(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_update_link(phydev) })
> +    }
> +
> +    /// Reads Link partner ability.
> +    pub fn genphy_read_lpa(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
> +    }
> +
> +    /// Reads PHY abilities.
> +    pub fn genphy_read_abilities(&mut self) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
> +    }
> +}
> +
> +/// Defines certain other features this PHY supports (like interrupts).
> +pub mod flags {
> +    /// PHY is internal.
> +    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
> +    /// PHY needs to be reset after the refclk is enabled.
> +    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
> +    /// Polling is used to detect PHY status changes.
> +    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
> +    /// Don't suspend.
> +    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
> +}
> +
> +/// An adapter for the registration of a PHY driver.
> +struct Adapter<T: Driver> {
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn soft_reset_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::soft_reset(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn get_features_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::get_features(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::suspend(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::resume(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn config_aneg_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::config_aneg(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn read_status_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::read_status(dev)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn match_phy_device_callback(
> +        phydev: *mut bindings::phy_device,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: Preconditions ensure `phydev` is valid.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::match_phy_device(dev) as i32
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn read_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            // CAST: the C side verifies devnum < 32.
> +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> +            Ok(ret.into())
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn write_mmd_callback(
> +        phydev: *mut bindings::phy_device,
> +        devnum: i32,
> +        regnum: u16,
> +        val: u16,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: Preconditions ensure `phydev` is valid.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::write_mmd(dev, devnum as u8, regnum, val)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> +        // SAFETY: Preconditions ensure `phydev` is valid.
> +        let dev = unsafe { Device::from_raw(phydev) };
> +        T::link_change_notify(dev);
> +    }
> +}
> +
> +/// Creates the kernel's `phy_driver` instance.
> +///
> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.

Missing '`'.

> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
> +    Opaque::new(bindings::phy_driver {
> +        name: T::NAME.as_char_ptr().cast_mut(),
> +        flags: T::FLAGS,
> +        phy_id: T::PHY_DEVICE_ID.id,
> +        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
> +        soft_reset: if T::HAS_SOFT_RESET {
> +            Some(Adapter::<T>::soft_reset_callback)
> +        } else {
> +            None
> +        },
> +        get_features: if T::HAS_GET_FEATURES {
> +            Some(Adapter::<T>::get_features_callback)
> +        } else {
> +            None
> +        },
> +        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
> +            Some(Adapter::<T>::match_phy_device_callback)
> +        } else {
> +            None
> +        },
> +        suspend: if T::HAS_SUSPEND {
> +            Some(Adapter::<T>::suspend_callback)
> +        } else {
> +            None
> +        },
> +        resume: if T::HAS_RESUME {
> +            Some(Adapter::<T>::resume_callback)
> +        } else {
> +            None
> +        },
> +        config_aneg: if T::HAS_CONFIG_ANEG {
> +            Some(Adapter::<T>::config_aneg_callback)
> +        } else {
> +            None
> +        },
> +        read_status: if T::HAS_READ_STATUS {
> +            Some(Adapter::<T>::read_status_callback)
> +        } else {
> +            None
> +        },
> +        read_mmd: if T::HAS_READ_MMD {
> +            Some(Adapter::<T>::read_mmd_callback)
> +        } else {
> +            None
> +        },
> +        write_mmd: if T::HAS_WRITE_MMD {
> +            Some(Adapter::<T>::write_mmd_callback)
> +        } else {
> +            None
> +        },
> +        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
> +            Some(Adapter::<T>::link_change_notify_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
> +        // sets `Option<&F>` to be `None`.
> +        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> +    })
> +}
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[vtable]
> +pub trait Driver {
> +    /// Defines certain other features this PHY supports.
> +    const FLAGS: u32 = 0;
> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;
> +    /// This driver only works for PHYs with IDs which match this field.
> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> +
> +    /// Issues a PHY software reset.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Probes the hardware to determine what abilities it has.
> +    fn get_features(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Returns true if this is a suitable driver for the given phydev.
> +    /// If not implemented, matching is based on [`PHY_DEVICE_ID`].
> +    fn match_phy_device(_dev: &mut Device) -> bool {
> +        false
> +    }
> +
> +    /// Configures the advertisement and resets auto-negotiation
> +    /// if auto-negotiation is enabled.
> +    fn config_aneg(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Determines the negotiated speed and duplex.
> +    fn read_status(_dev: &mut Device) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Suspends the hardware, saving state if needed.
> +    fn suspend(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Resumes the hardware, restoring state if needed.
> +    fn resume(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD read function for reading a MMD register.
> +    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD write function for writing a MMD register.
> +    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Callback for notification of link change.
> +    fn link_change_notify(_dev: &mut Device) {}
> +}
> +
> +/// Registration structure for a PHY driver.
> +///
> +/// # Invariants
> +///
> +/// The `drivers` points to an array of `struct phy_driver`, which is
> +/// registered to the kernel via `phy_drivers_register`.

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

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

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

Why is this an `Option`?

> +}
> +
> +impl Registration {
> +    /// Registers a PHY driver.
> +    #[must_use]
> +    pub fn register(
> +        module: &'static crate::ThisModule,
> +        drivers: &'static [Opaque<bindings::phy_driver>],
> +    ) -> Result<Self> {
> +        if drivers.len() == 0 {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
> +        to_result(unsafe {
> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
> +        })?;

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

> +        Ok(Registration {

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

> +            drivers: Some(drivers),
> +        })
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        if let Some(drv) = self.drivers.take() {
> +            // SAFETY: The type invariants guarantee that self.drivers is valid.
> +            unsafe { bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32) };
> +        }
> +    }
> +}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}

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

> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}
> +
> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
> +    pub id: u32,
> +    mask: DeviceMask,
> +}
> +
> +impl DeviceId {
> +    /// Creates a new instance with the exact match mask.
> +    pub const fn new_with_exact_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Exact,
> +        }
> +    }
> +
> +    /// Creates a new instance with the model match mask.
> +    pub const fn new_with_model_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Model,
> +        }
> +    }
> +
> +    /// Creates a new instance with the vendor match mask.
> +    pub const fn new_with_vendor_mask(id: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Vendor,
> +        }
> +    }
> +
> +    /// Creates a new instance with a custom match mask.
> +    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
> +        DeviceId {
> +            id,
> +            mask: DeviceMask::Custom(mask),
> +        }
> +    }
> +
> +    /// Creates a new instance from [`Driver`].
> +    pub const fn new_with_driver<T: Driver>() -> Self {
> +        T::PHY_DEVICE_ID
> +    }
> +
> +    /// Get a mask as u32.
> +    pub const fn mask_as_int(self) -> u32 {
> +        self.mask.as_int()
> +    }
> +}
> +
> +enum DeviceMask {
> +    Exact,
> +    Model,
> +    Vendor,
> +    Custom(u32),
> +}
> +
> +impl DeviceMask {
> +    const MASK_EXACT: u32 = !0;
> +    const MASK_MODEL: u32 = !0 << 4;
> +    const MASK_VENDOR: u32 = !0 << 10;
> +
> +    const fn as_int(self) -> u32 {
> +        match self {
> +            DeviceMask::Exact => Self::MASK_EXACT,
> +            DeviceMask::Model => Self::MASK_MODEL,
> +            DeviceMask::Vendor => Self::MASK_VENDOR,
> +            DeviceMask::Custom(mask) => mask,
> +        }
> +    }
> +}
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver` and registers it.
> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file. Every driver needs an entry in device_table.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +///     device_table: [
> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> +///         DeviceId::new_with_driver::<PhyAX88796B>()
> +///     ],
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! module_phy_driver {
> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> +    (@count_devices $($x:expr),*) => {
> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> +    };
> +
> +    (@device_table [$($dev:expr),+]) => {
> +        #[no_mangle]
> +        static __mod_mdio__phydev_device_table: [

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

> +            kernel::bindings::mdio_device_id;

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

> +            $crate::module_phy_driver!(@count_devices $($dev),+) + 1
> +        ] = [
> +            $(kernel::bindings::mdio_device_id {
> +                phy_id: $dev.id,
> +                phy_id_mask: $dev.mask_as_int()
> +            }),+,
> +            kernel::bindings::mdio_device_id {
> +                phy_id: 0,
> +                phy_id_mask: 0
> +            }
> +        ];
> +    };
> +
> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
> +        struct Module {
> +            _reg: kernel::net::phy::Registration,
> +        }
> +
> +        $crate::prelude::module! {
> +             type: Module,
> +             $($f)*
> +        }
> +
> +        static mut DRIVERS: [
> +            kernel::types::Opaque<kernel::bindings::phy_driver>;
> +            $crate::module_phy_driver!(@count_devices $($driver),+)
> +        ] = [
> +            $(kernel::net::phy::create_phy_driver::<$driver>()),+
> +        ];
> +
> +        impl kernel::Module for Module {
> +            fn init(module: &'static ThisModule) -> Result<Self> {
> +                // SAFETY: static `DRIVERS` array is used only in the C side.

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

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

You can also mention this in the SAFETY comment.

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

Miguel Ojeda Oct. 9, 2023, 12:59 p.m. UTC | #3

Hi Tomonori,

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

On Mon, Oct 9, 2023 at 3:41 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> +config RUST_PHYLIB_BINDINGS

This should be called ABSTRACTIONS. Please see:

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

Also, could this symbol go elsewhere?

> +        bool "PHYLIB bindings support"

Ditto.

> +          a wrapper around the C phlib core.

Typo.

> +               --rustified-enum phy_state\

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

> +#![feature(const_maybe_uninit_zeroed)]

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

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

Newline missing.

> +    /// Full-duplex mode

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

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

That should be separated.

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

Missing Markdown around the lifetime.

> +        // FIXME: enum-cast

Please explain what needs to be fixed.

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

Markdown missing (multiple instances).

> +    /// Reads Link partner ability.

Why is "link" capitalized here?

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

Broken formatting? Does `rustdoc` complain about it?

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

Perhaps "The `drivers` field"?

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

Markdown.

> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
> +    pub id: u32,
> +    mask: DeviceMask,
> +}

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

> +    /// Get a mask as u32.

Markdown.

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

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

"kernel's" or similar

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

Markdown.

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

Markdown.

> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +///     device_table: [
> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> +///         DeviceId::new_with_driver::<PhyAX88796B>()
> +///     ],
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +/// ```

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

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

Cheers,
Miguel

Andrew Lunn Oct. 9, 2023, 1:02 p.m. UTC | #4

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

Hi Benno

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

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

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

> > +    /// Reads a given C22 PHY register.
> > +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        // So an FFI call with a valid pointer.
> > +        let ret = unsafe {
> > +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> > +        };
> 
> Just a general question, all of these unsafe calls do not have
> additional requirements? So aside from the pointers being
> valid, there are no timing/locking/other safety requirements
> for calling the functions?

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

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

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

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

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

       Andrew

FUJITA Tomonori Oct. 9, 2023, 1:49 p.m. UTC | #5

On Mon, 9 Oct 2023 14:59:19 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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

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

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

Andrew Lunn Oct. 9, 2023, 1:54 p.m. UTC | #6

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

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

e.g:

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

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

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

      Andrew

Benno Lossin Oct. 9, 2023, 1:56 p.m. UTC | #7

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

Sure, I will keep it in mind.

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

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

>>> +    /// Reads a given C22 PHY register.
>>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        // So an FFI call with a valid pointer.
>>> +        let ret = unsafe {
>>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>> +        };
>>
>> Just a general question, all of these unsafe calls do not have
>> additional requirements? So aside from the pointers being
>> valid, there are no timing/locking/other safety requirements
>> for calling the functions?
> 
> Locking has been discussed a number of times already. What do you mean
> by timing?

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

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

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

[1]: https://lore.kernel.org/rust-for-linux/[e-mail]/
[2]: https://doc.rust-lang.org/reference/abi.html#the-no_mangle-attribute

--
Cheers,
Benno

Andrew Lunn Oct. 9, 2023, 2:13 p.m. UTC | #8

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

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

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

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

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

       Andrew

Miguel Ojeda Oct. 9, 2023, 2:32 p.m. UTC | #9

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

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

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

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

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

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

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

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

[1] https://lore.kernel.org/ksummit/CANiq72=99VFE=[e-mail]/

Cheers,
Miguel

Miguel Ojeda Oct. 9, 2023, 2:48 p.m. UTC | #10

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

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

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

Or did I misunderstand you?

Cheers,
Miguel

Greg Kroah-Hartman Oct. 9, 2023, 3:11 p.m. UTC | #11

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

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

thanks,

greg k-h

FUJITA Tomonori Oct. 9, 2023, 3:15 p.m. UTC | #12

On Mon, 9 Oct 2023 16:32:42 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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


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

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

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


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

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

Miguel Ojeda Oct. 9, 2023, 3:19 p.m. UTC | #13

On Mon, Oct 9, 2023 at 5:15 PM FUJITA Tomonori
<[e-mail]> wrote:
>
> It's up to PHY maintainers. I prefer that the patchset are merged very
> soon. Much easier to improve the code in tree.

Yes, sorry, my message was meant for Andrew.

Cheers,
Miguel

FUJITA Tomonori Oct. 9, 2023, 3:24 p.m. UTC | #14

On Mon, 9 Oct 2023 17:11:51 +0200
Greg KH <[e-mail]> wrote:

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

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

Miguel Ojeda Oct. 9, 2023, 3:39 p.m. UTC | #15

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

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

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

Cheers,
Miguel

FUJITA Tomonori Oct. 9, 2023, 3:50 p.m. UTC | #16

On Mon, 9 Oct 2023 17:39:27 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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

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

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

Andrew Lunn Oct. 9, 2023, 5:04 p.m. UTC | #17

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

'supposed to' is often not enough.

The netdev CI results can be seen here:

https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/

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

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

   Andrew

Trevor Gross Oct. 9, 2023, 9:07 p.m. UTC | #18

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

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

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

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

Andrew Lunn Oct. 9, 2023, 9:21 p.m. UTC | #19

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

There is this patch going through review at the moment.

https://lore.kernel.org/netdev/[e-mail]/T/

Which says:

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

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

    Andrew

FUJITA Tomonori Oct. 11, 2023, 7:04 a.m. UTC | #20

On Mon, 9 Oct 2023 17:07:18 -0400
Trevor Gross <[e-mail]> wrote:

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

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

I really apprecate your feedback on the patchset.

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

For me, they are an long-term issue.

Miguel Ojeda Oct. 11, 2023, 9:59 a.m. UTC | #21

On Mon, Oct 9, 2023 at 5:50 PM FUJITA Tomonori
<[e-mail]> wrote:
>
> What feedback? enum stuff? I think that it's a long-term issue.

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

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

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

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

In short, what is the rush?

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

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

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

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

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

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

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

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

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

And from `rustdoc`:

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

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

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

Cheers,
Miguel

FUJITA Tomonori Oct. 11, 2023, 2:16 p.m. UTC | #22

On Mon, 09 Oct 2023 12:19:54 +0000
Benno Lossin <[e-mail]> wrote:

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

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

(snip)

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

Added:

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


>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&mut self) -> u32 {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        unsafe { (*phydev).phy_id }
>> +    }
>> +
>> +    /// Gets the state of the PHY.
>> +    pub fn state(&mut self) -> DeviceState {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let state = unsafe { (*phydev).state };
>> +        // FIXME: enum-cast
>> +        match state {
>> +            bindings::phy_state::PHY_DOWN => DeviceState::Down,
>> +            bindings::phy_state::PHY_READY => DeviceState::Ready,
>> +            bindings::phy_state::PHY_HALTED => DeviceState::Halted,
>> +            bindings::phy_state::PHY_ERROR => DeviceState::Error,
>> +            bindings::phy_state::PHY_UP => DeviceState::Up,
>> +            bindings::phy_state::PHY_RUNNING => DeviceState::Running,
>> +            bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
>> +            bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
>> +        }
>> +    }
>> +
>> +    /// Returns true if the link is up.
>> +    pub fn get_link(&mut self) -> bool {
> 
> I would call this function `is_link_up`.
> 
>> +        const LINK_IS_UP: u32 = 1;
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        unsafe { (*phydev).link() == LINK_IS_UP }
> 
> Can you move the call to `link` and the `==` operation out
> of the `unsafe` block? They are safe operations. (also do
> that below where possible)

Sure, fixed.


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

Fixed.


>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
> 
> Since it is a reference you do not need to explicitly state
> that it points to an array of `struct phy_driver`. Instead I would
> suggest the following invariant:
> 
> All elements of the `drivers` slice are valid and currently registered
> to the kernel via `phy_drivers_register`.

Surely, makes sense. 


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

Oops, removed; leftover of older version.


>> +}
>> +
>> +impl Registration {
>> +    /// Registers a PHY driver.
>> +    #[must_use]
>> +    pub fn register(
>> +        module: &'static crate::ThisModule,
>> +        drivers: &'static [Opaque<bindings::phy_driver>],
>> +    ) -> Result<Self> {
>> +        if drivers.len() == 0 {
>> +            return Err(code::EINVAL);
>> +        }
>> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
>> +        to_result(unsafe {
>> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
>> +        })?;
> 
> This `register` function seems to assume that the values of the
> `drivers` array are initialized and otherwise also considered valid.
> So please change that or make this function `unsafe`.

Understood.


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

Added.


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

Updated.


>> +            $crate::module_phy_driver!(@count_devices $($dev),+) + 1
>> +        ] = [
>> +            $(kernel::bindings::mdio_device_id {
>> +                phy_id: $dev.id,
>> +                phy_id_mask: $dev.mask_as_int()
>> +            }),+,
>> +            kernel::bindings::mdio_device_id {
>> +                phy_id: 0,
>> +                phy_id_mask: 0
>> +            }
>> +        ];
>> +    };
>> +
>> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>> +        struct Module {
>> +            _reg: kernel::net::phy::Registration,
>> +        }
>> +
>> +        $crate::prelude::module! {
>> +             type: Module,
>> +             $($f)*
>> +        }
>> +
>> +        static mut DRIVERS: [
>> +            kernel::types::Opaque<kernel::bindings::phy_driver>;
>> +            $crate::module_phy_driver!(@count_devices $($driver),+)
>> +        ] = [
>> +            $(kernel::net::phy::create_phy_driver::<$driver>()),+
>> +        ];
>> +
>> +        impl kernel::Module for Module {
>> +            fn init(module: &'static ThisModule) -> Result<Self> {
>> +                // SAFETY: static `DRIVERS` array is used only in the C side.
> 
> In order for this SAFETY comment to be correct, you need to ensure
> that nobody else can access the `DRIVERS` static. You can do that by
> placing both the `static mut DRIVERS` and the `impl ::kernel::Module
> for Module` items inside of a `const _: () = {}`, so like this:
> 
>      const _: () = {
>          static mut DRIVERS: [...] = ...;
>          impl ::kernel::Module for Module { ... }
>      };
> 
> You can also mention this in the SAFETY comment.

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

Boqun Feng Oct. 11, 2023, 6:29 p.m. UTC | #23

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

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

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

Regards,
Boqun

> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).phy_id }
> +    }
> +
> +    /// Gets the state of the PHY.
> +    pub fn state(&mut self) -> DeviceState {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let state = unsafe { (*phydev).state };
> +        // FIXME: enum-cast
> +        match state {
> +            bindings::phy_state::PHY_DOWN => DeviceState::Down,
> +            bindings::phy_state::PHY_READY => DeviceState::Ready,
> +            bindings::phy_state::PHY_HALTED => DeviceState::Halted,
> +            bindings::phy_state::PHY_ERROR => DeviceState::Error,
> +            bindings::phy_state::PHY_UP => DeviceState::Up,
> +            bindings::phy_state::PHY_RUNNING => DeviceState::Running,
> +            bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
> +            bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
> +        }
> +    }
> +
[...]

FUJITA Tomonori Oct. 11, 2023, 11:18 p.m. UTC | #24

On Wed, 11 Oct 2023 11:59:01 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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

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


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

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

FUJITA Tomonori Oct. 12, 2023, 12:29 a.m. UTC | #25

On Wed, 11 Oct 2023 11:59:01 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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

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

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

FUJITA Tomonori Oct. 12, 2023, 3:59 a.m. UTC | #26

On Mon, 9 Oct 2023 14:59:19 +0200
Miguel Ojeda <[e-mail]> wrote:

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

Fixed.

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

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


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

Updated.


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

Oops, sorry.


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

I added the following to the commit log.

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


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

Added.


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

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

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

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

Added.


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

Fixed.


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

Added.


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

Can you elaborate?


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

Fixed.


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

Yes, sorry about that.


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

I replaced this with the following comment suggested by Benno.

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


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

Fixed.


>> +/// Represents the kernel's `struct mdio_device_id`.
>> +pub struct DeviceId {
>> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
>> +    pub id: u32,
>> +    mask: DeviceMask,
>> +}
> 
> It would be nice to explain why the field is `pub`.

Added.


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

Fixed.


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

Yeah.


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

Added.


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

Fixed.

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

Fixed.


>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +///     device_table: [
>> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
>> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
>> +///         DeviceId::new_with_driver::<PhyAX88796B>()
>> +///     ],
>> +///     name: "rust_asix_phy",
>> +///     author: "Rust for Linux Contributors",
>> +///     description: "Rust Asix PHYs driver",
>> +///     license: "GPL",
>> +/// }
>> +/// ```
> 
> Please add an example above with the expansion of the macro so that it
> is easy to understand at a glance, see e.g. what Benno did in
> `pin-init` (`rust/init*`).

Added.

Thanks a lot!

Trevor Gross Oct. 12, 2023, 4:43 a.m. UTC | #27

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

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

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

BMCR_RESET -> `BMCR_RESET` I believe

> > +/// Represents the kernel's `struct mdio_device_id`.
> > +pub struct DeviceId {
> > +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
> > +    pub id: u32,
> > +    mask: DeviceMask,
> > +}
>
> It would be nice to explain why the field is `pub`.

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

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

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

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

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

FUJITA Tomonori Oct. 12, 2023, 5:58 a.m. UTC | #28

On Wed, 11 Oct 2023 11:29:45 -0700
Boqun Feng <[e-mail]> wrote:

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

Ah, I can drop all the mut here.

Boqun Feng Oct. 12, 2023, 6:34 a.m. UTC | #29

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

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

A simple type trick comes to me is that

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

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

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

Others may have better idea.

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

Regards,
Boqun

>

FUJITA Tomonori Oct. 12, 2023, 6:44 a.m. UTC | #30

On Wed, 11 Oct 2023 23:34:18 -0700
Boqun Feng <[e-mail]> wrote:

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

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

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

FUJITA Tomonori Oct. 12, 2023, 7:02 a.m. UTC | #31

On Thu, 12 Oct 2023 15:44:44 +0900 (JST)
FUJITA Tomonori <[e-mail]> wrote:

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

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

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

Boqun Feng Oct. 12, 2023, 7:07 a.m. UTC | #32

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

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

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

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

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

Regards,
Boqun

FUJITA Tomonori Oct. 12, 2023, 7:09 a.m. UTC | #33

On Thu, 12 Oct 2023 00:43:00 -0400
Trevor Gross <[e-mail]> wrote:

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

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


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

Thanks, fixed.


>> > +/// Represents the kernel's `struct mdio_device_id`.
>> > +pub struct DeviceId {
>> > +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
>> > +    pub id: u32,
>> > +    mask: DeviceMask,
>> > +}
>>
>> It would be nice to explain why the field is `pub`.
> 
> On this subject, I think it would be good to add
> 
>     impl DeviceId {
>         #[doc(hidden)] // <- macro use only
>         pub const fn as_mdio_device_id(&self) ->
> bindings::mdio_device_id { /* ... */ }
>     }
> 
> That makes more sense when creating the table, and `id` no longer has
> to be public.

Ah, nice.


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

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

Boqun Feng Oct. 12, 2023, 7:13 a.m. UTC | #34

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

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

Regards,
Boqun

Trevor Gross Oct. 12, 2023, 7:32 a.m. UTC | #35

On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <[e-mail]> wrote:

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

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

Andrew said a bit about it in the second comment here:
https://lore.kernel.org/rust-for-linux/[e-mail]/

FUJITA Tomonori Oct. 12, 2023, 7:58 a.m. UTC | #36

On Thu, 12 Oct 2023 03:32:44 -0400
Trevor Gross <[e-mail]> wrote:

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

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

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

Benno Lossin Oct. 12, 2023, 9:10 a.m. UTC | #37

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

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

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

Boqun Feng Oct. 13, 2023, 4:17 a.m. UTC | #38

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

Thank Benno for calling this out.

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

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

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

Regards,
Boqun

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

FUJITA Tomonori Oct. 13, 2023, 5:45 a.m. UTC | #39

On Thu, 12 Oct 2023 21:17:14 -0700
Boqun Feng <[e-mail]> wrote:

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

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

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

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

Benno Lossin Oct. 13, 2023, 7:56 a.m. UTC | #40

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

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

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

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

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

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

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

FUJITA Tomonori Oct. 13, 2023, 9:53 a.m. UTC | #41

On Fri, 13 Oct 2023 07:56:07 +0000
Benno Lossin <[e-mail]> wrote:

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

Agreed.


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

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


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

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

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

What's the practical benefit from the check?


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

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

Benno Lossin Oct. 13, 2023, 10:03 a.m. UTC | #42

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

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

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

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

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

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

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

> What's the practical benefit from the check?

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

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

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

FUJITA Tomonori Oct. 13, 2023, 10:53 a.m. UTC | #43

On Fri, 13 Oct 2023 10:03:43 +0000
Benno Lossin <[e-mail]> wrote:

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

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


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

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


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

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

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

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


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

What sane default? PHY_ERROR?


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

hmm.


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

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

Miguel Ojeda Oct. 13, 2023, 11:59 a.m. UTC | #44

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

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

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

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

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

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

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

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

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

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

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

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

Cheers,
Miguel

[1]

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

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

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

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

FUJITA Tomonori Oct. 13, 2023, 3:15 p.m. UTC | #45

On Fri, 13 Oct 2023 13:59:12 +0200
Miguel Ojeda <[e-mail]> wrote:

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

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

+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+}

Then write match code by hand.

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

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


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

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


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

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

Miguel Ojeda Oct. 13, 2023, 6:33 p.m. UTC | #46

On Fri, Oct 13, 2023 at 5:15 PM FUJITA Tomonori
<[e-mail]> wrote:
>
> I meant that defining Rust's enum corresponding to the kernel's enum
> phy_state like.
>
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +}
>
> Then write match code by hand.

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

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

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

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

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

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

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

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

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

Cheers,
Miguel

Boqun Feng Oct. 14, 2023, 4:11 a.m. UTC | #47

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

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

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

Regards,
Boqun

Benno Lossin Oct. 14, 2023, 7:47 a.m. UTC | #48

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

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

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

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

Just think of the C side having some weird bug.

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

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

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

Sure.

Miguel Ojeda Oct. 14, 2023, 11:59 a.m. UTC | #49

On Fri, Oct 13, 2023 at 11:53 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> I'm not sure the general rules in Rust can be applied to linux kernel.

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

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

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

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

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

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

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

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

Cheers,
Miguel

FUJITA Tomonori Oct. 14, 2023, 12:31 p.m. UTC | #50

On Fri, 13 Oct 2023 20:33:58 +0200
Miguel Ojeda <[e-mail]> wrote:

> On Fri, Oct 13, 2023 at 5:15 PM FUJITA Tomonori
> <[e-mail]> wrote:
>>
>> I meant that defining Rust's enum corresponding to the kernel's enum
>> phy_state like.
>>
>> +pub enum DeviceState {
>> +    /// PHY device and driver are not ready for anything.
>> +    Down,
>> +    /// PHY is ready to send and receive packets.
>> +    Ready,
>> +    /// PHY is up, but no polling or interrupts are done.
>> +    Halted,
>> +    /// PHY is up, but is in an error state.
>> +    Error,
>> +    /// PHY and attached device are ready to do work.
>> +    Up,
>> +    /// PHY is currently running.
>> +    Running,
>> +    /// PHY is up, but not currently plugged in.
>> +    NoLink,
>> +    /// PHY is performing a cable test.
>> +    CableTest,
>> +}
>>
>> Then write match code by hand.
> 
> Yes, but that alone is not enough -- that is what we do normally, but
> we can still diverge with the C side. That is what the `bindgen`
> proposal would solve (plus better maintenance). The workaround also
> solves that, but with more maintenance effort. We could even go
> further, but I don't think it is worth it given that we really want to
> have it in `bindgen`.

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

I'll use your workaround in the next version.

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

Thanks a lot!

Miguel Ojeda Oct. 14, 2023, 4:19 p.m. UTC | #51

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

No, that is all wrong.

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

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

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

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

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

Cheers,
Miguel

Andrew Lunn Oct. 14, 2023, 9:55 p.m. UTC | #52

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

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

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

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

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

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

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

  Andrew

Benno Lossin Oct. 14, 2023, 10:18 p.m. UTC | #53

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

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

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

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

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

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

Andrew Lunn Oct. 14, 2023, 10:33 p.m. UTC | #54

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

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

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

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

	 Andrew

Patch

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