* [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers
@ 2023-10-24 0:58 FUJITA Tomonori
2023-10-24 0:58 ` [PATCH net-next v6 1/5] rust: core " FUJITA Tomonori
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw)
To: netdev
Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis,
benno.lossin, wedsonaf, greg
This patchset adds Rust abstractions for phylib. It doesn't fully
cover the C APIs yet but I think that it's already useful. I implement
two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems
they work well with real hardware.
The first patch introduces Rust bindings for phylib.
The second patch add a macro to declare a kernel module for PHYs
drivers.
The third patch add Miguel's work to make sure that the C's enum is
sync with Rust sides. If not, compiling fails. Note that this is a
temporary solution. It will be replaced with bindgen when it supports
generating the enum conversion code.
The fourth add the Rust ETHERNET PHY LIBRARY entry to MAINTAINERS
file; adds the binding file and me as a maintainer (as Andrew Lunn
suggested) with Trevor Gross as a reviewer.
The last patch introduces the Rust version of Asix PHY drivers,
drivers/net/phy/ax88796b.c. The features are equivalent to the C
version. You can choose C (by default) or Rust version on kernel
configuration.
v6:
- improves comments
- makes the requirement of phy_drivers_register clear
- fixes Makefile of the third patch
v5: https://lore.kernel.org/all/20231019.094147.1808345526469629486.fujita.tomonori@gmail.com/T/
- drops the rustified-enum option, writes match by hand; no *risk* of UB
- adds Miguel's patch for enum checking
- moves CONFIG_RUST_PHYLIB_ABSTRACTIONS to drivers/net/phy/Kconfig
- adds a new entry for this abstractions in MAINTAINERS
- changes some of Device's methods to take &mut self
- comment improvment
v4: https://lore.kernel.org/netdev/20231012125349.2702474-1-fujita.tomonori@gmail.com/T/
- split the core patch
- making Device::from_raw() private
- comment improvement with code update
- commit message improvement
- avoiding using bindings::phy_driver in public functions
- using an anonymous constant in module_phy_driver macro
v3: https://lore.kernel.org/netdev/20231011.231607.1747074555988728415.fujita.tomonori@gmail.com/T/
- changes the base tree to net-next from rust-next
- makes this feature optional; only enabled with CONFIG_RUST_PHYLIB_BINDINGS=y
- cosmetic code and comment improvement
- adds copyright
v2: https://lore.kernel.org/netdev/20231006094911.3305152-2-fujita.tomonori@gmail.com/T/
- build failure fix
- function renaming
v1: https://lore.kernel.org/netdev/20231002085302.2274260-3-fujita.tomonori@gmail.com/T/
FUJITA Tomonori (4):
rust: core abstractions for network PHY drivers
rust: net::phy add module_phy_driver macro
MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY
net: phy: add Rust Asix PHY driver
Miguel Ojeda (1):
rust: add second `bindgen` pass for enum exhaustiveness checking
MAINTAINERS | 16 +
drivers/net/phy/Kconfig | 16 +
drivers/net/phy/Makefile | 6 +-
drivers/net/phy/ax88796b_rust.rs | 129 +++++
rust/.gitignore | 1 +
rust/Makefile | 14 +
rust/bindings/bindings_enum_check.rs | 36 ++
rust/bindings/bindings_helper.h | 3 +
rust/kernel/lib.rs | 3 +
rust/kernel/net.rs | 6 +
rust/kernel/net/phy.rs | 837 +++++++++++++++++++++++++++
rust/uapi/uapi_helper.h | 2 +
12 files changed, 1068 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/phy/ax88796b_rust.rs
create mode 100644 rust/bindings/bindings_enum_check.rs
create mode 100644 rust/kernel/net.rs
create mode 100644 rust/kernel/net/phy.rs
base-commit: f4dbc2bb7a54d3bff234a9f1915f1b7187bedb1f
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori @ 2023-10-24 0:58 ` FUJITA Tomonori 2023-10-24 16:23 ` Benno Lossin 2023-10-24 0:58 ` [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro FUJITA Tomonori ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw) To: netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, benno.lossin, wedsonaf, greg This patch adds abstractions to implement network PHY drivers; the driver registration and bindings for some of callback functions in struct phy_driver and many genphy_ functions. This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. This patch enables unstable const_maybe_uninit_zeroed feature for kernel crate to enable unsafe code to handle a constant value with uninitialized data. With the feature, the abstractions can initialize a phy_driver structure with zero easily; instead of initializing all the members by hand. It's supposed to be stable in the not so distant future. Link: https://github.com/rust-lang/rust/pull/116218 Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- drivers/net/phy/Kconfig | 8 + rust/bindings/bindings_helper.h | 3 + rust/kernel/lib.rs | 3 + rust/kernel/net.rs | 6 + rust/kernel/net/phy.rs | 708 ++++++++++++++++++++++++++++++++ 5 files changed, 728 insertions(+) create mode 100644 rust/kernel/net.rs create mode 100644 rust/kernel/net/phy.rs diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 421d2b62918f..0faebdb184ca 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -60,6 +60,14 @@ config FIXED_PHY Currently tested with mpc866ads and mpc8349e-mitx. +config RUST_PHYLIB_ABSTRACTIONS + bool "PHYLIB abstractions support" + depends on RUST + depends on PHYLIB=y + help + Adds support needed for PHY drivers written in Rust. It provides + a wrapper around the C phylib core. + config SFP tristate "SFP cage support" depends on I2C && PHYLINK diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index c91a3c24f607..ec4ee09a34ad 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,9 @@ #include <kunit/test.h> #include <linux/errname.h> +#include <linux/ethtool.h> +#include <linux/mdio.h> +#include <linux/phy.h> #include <linux/slab.h> #include <linux/refcount.h> #include <linux/wait.h> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e8811700239a..0588422e273c 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -14,6 +14,7 @@ #![no_std] #![feature(allocator_api)] #![feature(coerce_unsized)] +#![feature(const_maybe_uninit_zeroed)] #![feature(dispatch_from_dyn)] #![feature(new_uninit)] #![feature(receiver_trait)] @@ -36,6 +37,8 @@ pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit; +#[cfg(CONFIG_NET)] +pub mod net; pub mod prelude; pub mod print; mod static_assert; diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs new file mode 100644 index 000000000000..fe415cb369d3 --- /dev/null +++ b/rust/kernel/net.rs @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Networking. + +#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)] +pub mod phy; diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs new file mode 100644 index 000000000000..2d821c2475e1 --- /dev/null +++ b/rust/kernel/net/phy.rs @@ -0,0 +1,708 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> + +//! Network PHY device. +//! +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). + +use crate::{ + bindings, + error::*, + prelude::{vtable, Pin}, + str::CStr, + types::Opaque, +}; +use core::marker::PhantomData; + +/// PHY state machine states. +/// +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state). +/// Some of PHY drivers access to the state of PHY's software state machine. +#[derive(PartialEq)] +pub enum DeviceState { + /// PHY device and driver are not ready for anything. + Down, + /// PHY is ready to send and receive packets. + Ready, + /// PHY is up, but no polling or interrupts are done. + Halted, + /// PHY is up, but is in an error state. + Error, + /// PHY and attached device are ready to do work. + Up, + /// PHY is currently running. + Running, + /// PHY is up, but not currently plugged in. + NoLink, + /// PHY is performing a cable test. + CableTest, +} + +/// A mode of Ethernet communication. +/// +/// PHY drivers get duplex information from hardware and update the current state. +pub enum DuplexMode { + /// PHY is in full-duplex mode. + Full, + /// PHY is in half-duplex mode. + Half, + /// PHY is in unknown duplex mode. + Unknown, +} + +/// An instance of a PHY device. +/// +/// Wraps the kernel's [`struct phy_device`](https://docs.kernel.org/networking/kapi.html#c.phy_device). +/// A [`Device`] instance is created when a callback in [`Driver``] is executed. A PHY driver executes +/// [`Driver`]'s methods during the callback. +/// +/// # Invariants +/// +/// `self.0` is always in a valid state. +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique +// for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock held, +// thus guaranteeing that [`Driver::resume`] has exclusive access to the instance. [`Driver::resume`] and +// [`Driver::suspend`] also are called where only one thread can access to the instance. +#[repr(transparent)] +pub struct Device(Opaque<bindings::phy_device>); + +impl Device { + /// Creates a new [`Device`] instance from a raw pointer. + /// + /// # Safety + /// + /// This function must only be called from the callbacks in `phy_driver`. + unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self { + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`. + let ptr = ptr.cast::<Self>(); + // SAFETY: by the function requirements the pointer is valid and we have unique access for + // the duration of `'a`. + unsafe { &mut *ptr } + } + + /// Gets the id of the PHY. + pub fn phy_id(&self) -> u32 { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).phy_id } + } + + /// Gets the state of the PHY. + pub fn state(&self) -> DeviceState { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + let state = unsafe { (*phydev).state }; + // TODO: this conversion code will be replaced with automatically generated code by bindgen + // when it becomes possible. + // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support). + match state { + bindings::phy_state_PHY_DOWN => DeviceState::Down, + bindings::phy_state_PHY_READY => DeviceState::Ready, + bindings::phy_state_PHY_HALTED => DeviceState::Halted, + bindings::phy_state_PHY_ERROR => DeviceState::Error, + bindings::phy_state_PHY_UP => DeviceState::Up, + bindings::phy_state_PHY_RUNNING => DeviceState::Running, + bindings::phy_state_PHY_NOLINK => DeviceState::NoLink, + bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest, + _ => DeviceState::Error, + } + } + + /// Gets the current link state. It returns true if the link is up. + pub fn get_link(&self) -> bool { + const LINK_IS_UP: u32 = 1; + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + let phydev = unsafe { *self.0.get() }; + phydev.link() == LINK_IS_UP + } + + /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled. + pub fn is_autoneg_enabled(&self) -> bool { + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + let phydev = unsafe { *self.0.get() }; + phydev.autoneg() == bindings::AUTONEG_ENABLE + } + + /// Gets the current auto-negotiation state. It returns true if auto-negotiation is completed. + pub fn is_autoneg_completed(&self) -> bool { + const AUTONEG_COMPLETED: u32 = 1; + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + let phydev = unsafe { *self.0.get() }; + phydev.autoneg_complete() == AUTONEG_COMPLETED + } + + /// Sets the speed of the PHY. + pub fn set_speed(&mut self, speed: u32) { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).speed = speed as i32 }; + } + + /// Sets duplex mode. + pub fn set_duplex(&mut self, mode: DuplexMode) { + let phydev = self.0.get(); + let v = match mode { + DuplexMode::Full => bindings::DUPLEX_FULL as i32, + DuplexMode::Half => bindings::DUPLEX_HALF as i32, + DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32, + }; + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).duplex = v }; + } + + /// Reads a given C22 PHY register. + // This function reads a hardware register and updates the stats so takes `&mut self`. + pub fn read(&mut self, regnum: u16) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) + }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Writes a given C22 PHY register. + pub fn write(&mut self, regnum: u16, val: u16) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val) + }) + } + + /// Reads a paged register. + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Resolves the advertisements into PHY settings. + pub fn resolve_aneg_linkmode(&mut self) { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + unsafe { bindings::phy_resolve_aneg_linkmode(phydev) }; + } + + /// Executes software reset the PHY via `BMCR_RESET` bit. + pub fn genphy_soft_reset(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_soft_reset(phydev) }) + } + + /// Initializes the PHY. + pub fn init_hw(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // so an FFI call with a valid pointer. + to_result(unsafe { bindings::phy_init_hw(phydev) }) + } + + /// Starts auto-negotiation. + pub fn start_aneg(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::phy_start_aneg(phydev) }) + } + + /// Resumes the PHY via `BMCR_PDOWN` bit. + pub fn genphy_resume(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_resume(phydev) }) + } + + /// Suspends the PHY via `BMCR_PDOWN` bit. + pub fn genphy_suspend(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_suspend(phydev) }) + } + + /// Checks the link status and updates current link state. + pub fn genphy_read_status(&mut self) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { bindings::genphy_read_status(phydev) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Updates the link status. + pub fn genphy_update_link(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_update_link(phydev) }) + } + + /// Reads link partner ability. + pub fn genphy_read_lpa(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_read_lpa(phydev) }) + } + + /// Reads PHY abilities. + pub fn genphy_read_abilities(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_read_abilities(phydev) }) + } +} + +/// Defines certain other features this PHY supports (like interrupts). +/// +/// These flag values are used in [`Driver::FLAGS`]. +pub mod flags { + /// PHY is internal. + pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL; + /// PHY needs to be reset after the refclk is enabled. + pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN; + /// Polling is used to detect PHY status changes. + pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST; + /// Don't suspend. + pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND; +} + +/// An adapter for the registration of a PHY driver. +struct Adapter<T: Driver> { + _p: PhantomData<T>, +} + +impl<T: Driver> Adapter<T> { + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn soft_reset_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::soft_reset(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn get_features_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::get_features(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::suspend(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::resume(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn config_aneg_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::config_aneg(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn read_status_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::read_status(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn match_phy_device_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::match_phy_device(dev) as i32 + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn read_mmd_callback( + phydev: *mut bindings::phy_device, + devnum: i32, + regnum: u16, + ) -> i32 { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + // CAST: the C side verifies devnum < 32. + let ret = T::read_mmd(dev, devnum as u8, regnum)?; + Ok(ret.into()) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn write_mmd_callback( + phydev: *mut bindings::phy_device, + devnum: i32, + regnum: u16, + val: u16, + ) -> i32 { + from_result(|| { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::write_mmd(dev, devnum as u8, regnum, val)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) { + // SAFETY: Preconditions ensure `phydev` is valid. + let dev = unsafe { Device::from_raw(phydev) }; + T::link_change_notify(dev); + } +} + +/// Driver structure for a particular PHY type. +/// +/// Wraps the kernel's [`struct phy_driver`](https://docs.kernel.org/networking/kapi.html#c.phy_driver). +/// This is used to register a driver for a particular PHY type with the kernel. +/// +/// # Invariants +/// +/// `self.0` is always in a valid state. +#[repr(transparent)] +pub struct DriverVTable(Opaque<bindings::phy_driver>); + +/// Creates a [`DriverVTable`] instance from [`Driver`]. +/// +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`. +/// +/// [`module_phy_driver`]: crate::module_phy_driver +pub const fn create_phy_driver<T: Driver>() -> DriverVTable { + // INVARIANT: All the fields of `struct phy_driver` are initialized properly. + DriverVTable(Opaque::new(bindings::phy_driver { + name: T::NAME.as_char_ptr().cast_mut(), + flags: T::FLAGS, + phy_id: T::PHY_DEVICE_ID.id, + phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), + soft_reset: if T::HAS_SOFT_RESET { + Some(Adapter::<T>::soft_reset_callback) + } else { + None + }, + get_features: if T::HAS_GET_FEATURES { + Some(Adapter::<T>::get_features_callback) + } else { + None + }, + match_phy_device: if T::HAS_MATCH_PHY_DEVICE { + Some(Adapter::<T>::match_phy_device_callback) + } else { + None + }, + suspend: if T::HAS_SUSPEND { + Some(Adapter::<T>::suspend_callback) + } else { + None + }, + resume: if T::HAS_RESUME { + Some(Adapter::<T>::resume_callback) + } else { + None + }, + config_aneg: if T::HAS_CONFIG_ANEG { + Some(Adapter::<T>::config_aneg_callback) + } else { + None + }, + read_status: if T::HAS_READ_STATUS { + Some(Adapter::<T>::read_status_callback) + } else { + None + }, + read_mmd: if T::HAS_READ_MMD { + Some(Adapter::<T>::read_mmd_callback) + } else { + None + }, + write_mmd: if T::HAS_WRITE_MMD { + Some(Adapter::<T>::write_mmd_callback) + } else { + None + }, + link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY { + Some(Adapter::<T>::link_change_notify_callback) + } else { + None + }, + // SAFETY: The rest is zeroed out to initialize `struct phy_driver`, + // sets `Option<&F>` to be `None`. + ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() } + })) +} + +/// Driver implementation for a particular PHY type. +/// +/// This trait is used to create a [`DriverVTable`]. +#[vtable] +pub trait Driver { + /// Defines certain other features this PHY supports. + /// It is a combination of the flags in the [`flags`] module. + const FLAGS: u32 = 0; + + /// The friendly name of this PHY type. + const NAME: &'static CStr; + + /// This driver only works for PHYs with IDs which match this field. + /// The default id and mask are zero. + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0); + + /// Issues a PHY software reset. + fn soft_reset(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Probes the hardware to determine what abilities it has. + fn get_features(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Returns true if this is a suitable driver for the given phydev. + /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`]. + fn match_phy_device(_dev: &Device) -> bool { + false + } + + /// Configures the advertisement and resets auto-negotiation + /// if auto-negotiation is enabled. + fn config_aneg(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Determines the negotiated speed and duplex. + fn read_status(_dev: &mut Device) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Suspends the hardware, saving state if needed. + fn suspend(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Resumes the hardware, restoring state if needed. + fn resume(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD read function for reading a MMD register. + fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD write function for writing a MMD register. + fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result { + Err(code::ENOTSUPP) + } + + /// Callback for notification of link change. + fn link_change_notify(_dev: &mut Device) {} +} + +/// Registration structure for PHY drivers. +/// +/// Registers [`DriverVTable`] instances with the kernel. They will be unregistered when dropped. +/// +/// # Invariants +/// +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`. +pub struct Registration { + drivers: Pin<&'static mut [DriverVTable]>, +} + +impl Registration { + /// Registers a PHY driver. + pub fn register( + module: &'static crate::ThisModule, + drivers: Pin<&'static mut [DriverVTable]>, + ) -> Result<Self> { + if drivers.is_empty() { + return Err(code::EINVAL); + } + // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice + // are initialized properly. `drivers` will not be moved. So an FFI call with a valid pointer. + to_result(unsafe { + bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0) + })?; + // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`. + Ok(Registration { drivers }) + } +} + +impl Drop for Registration { + fn drop(&mut self) { + // SAFETY: The type invariants guarantee that `self.drivers` is valid. + // So an FFI call with a valid pointer. + unsafe { + bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32) + }; + } +} + +// SAFETY: `Registration` does not expose any of its state across threads. +unsafe impl Send for Registration {} + +// SAFETY: `Registration` does not expose any of its state across threads. +unsafe impl Sync for Registration {} + +/// An identifier for PHY devices on an MDIO/MII bus. +/// +/// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate PHY driver. +pub struct DeviceId { + id: u32, + mask: DeviceMask, +} + +impl DeviceId { + /// Creates a new instance with the exact match mask. + pub const fn new_with_exact_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Exact, + } + } + + /// Creates a new instance with the model match mask. + pub const fn new_with_model_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Model, + } + } + + /// Creates a new instance with the vendor match mask. + pub const fn new_with_vendor_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Vendor, + } + } + + /// Creates a new instance with a custom match mask. + pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Custom(mask), + } + } + + /// Creates a new instance from [`Driver`]. + pub const fn new_with_driver<T: Driver>() -> Self { + T::PHY_DEVICE_ID + } + + /// Get a `mask` as u32. + pub const fn mask_as_int(&self) -> u32 { + self.mask.as_int() + } + + // macro use only + #[doc(hidden)] + pub const fn mdio_device_id(&self) -> bindings::mdio_device_id { + bindings::mdio_device_id { + phy_id: self.id, + phy_id_mask: self.mask.as_int(), + } + } +} + +enum DeviceMask { + Exact, + Model, + Vendor, + Custom(u32), +} + +impl DeviceMask { + const MASK_EXACT: u32 = !0; + const MASK_MODEL: u32 = !0 << 4; + const MASK_VENDOR: u32 = !0 << 10; + + const fn as_int(&self) -> u32 { + match self { + DeviceMask::Exact => Self::MASK_EXACT, + DeviceMask::Model => Self::MASK_MODEL, + DeviceMask::Vendor => Self::MASK_VENDOR, + DeviceMask::Custom(mask) => *mask, + } + } +} -- 2.34.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-24 0:58 ` [PATCH net-next v6 1/5] rust: core " FUJITA Tomonori @ 2023-10-24 16:23 ` Benno Lossin 2023-10-24 16:44 ` Andrew Lunn 2023-10-25 1:10 ` FUJITA Tomonori 0 siblings, 2 replies; 22+ messages in thread From: Benno Lossin @ 2023-10-24 16:23 UTC (permalink / raw) To: FUJITA Tomonori, netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 24.10.23 02:58, 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_ABSTRACTIONS=y. > > This patch enables unstable const_maybe_uninit_zeroed feature for > kernel crate to enable unsafe code to handle a constant value with > uninitialized data. With the feature, the abstractions can initialize > a phy_driver structure with zero easily; instead of initializing all > the members by hand. It's supposed to be stable in the not so distant > future. > > Link: https://github.com/rust-lang/rust/pull/116218 > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> [...] > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > new file mode 100644 > index 000000000000..2d821c2475e1 > --- /dev/null > +++ b/rust/kernel/net/phy.rs > @@ -0,0 +1,708 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> > + > +//! Network PHY device. > +//! > +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). > + > +use crate::{ > + bindings, > + error::*, > + prelude::{vtable, Pin}, > + str::CStr, > + types::Opaque, > +}; > +use core::marker::PhantomData; > + > +/// PHY state machine states. > +/// > +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state). Please use `rustfmt`, this line is 109 characters long. Also it might make sense to use a relative link, since then it also works offline (though you have to build the C docs). > +/// Some of PHY drivers access to the state of PHY's software state machine. > +#[derive(PartialEq)] > +pub enum DeviceState { > + /// PHY device and driver are not ready for anything. > + Down, > + /// PHY is ready to send and receive packets. > + Ready, > + /// PHY is up, but no polling or interrupts are done. > + Halted, > + /// PHY is up, but is in an error state. > + Error, > + /// PHY and attached device are ready to do work. > + Up, > + /// PHY is currently running. > + Running, > + /// PHY is up, but not currently plugged in. > + NoLink, > + /// PHY is performing a cable test. > + CableTest, > +} > + > +/// A mode of Ethernet communication. > +/// > +/// PHY drivers get duplex information from hardware and update the current state. > +pub enum DuplexMode { > + /// PHY is in full-duplex mode. > + Full, > + /// PHY is in half-duplex mode. > + Half, > + /// PHY is in unknown duplex mode. > + Unknown, > +} > + > +/// An instance of a PHY device. > +/// > +/// Wraps the kernel's [`struct phy_device`](https://docs.kernel.org/networking/kapi.html#c.phy_device). > +/// A [`Device`] instance is created when a callback in [`Driver``] is executed. A PHY driver executes > +/// [`Driver`]'s methods during the callback. > +/// > +/// # Invariants > +/// > +/// `self.0` is always in a valid state. > +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique > +// for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for > +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock held, > +// thus guaranteeing that [`Driver::resume`] has exclusive access to the instance. [`Driver::resume`] and > +// [`Driver::suspend`] also are called where only one thread can access to the instance. > +#[repr(transparent)] > +pub struct Device(Opaque<bindings::phy_device>); > + > +impl Device { > + /// Creates a new [`Device`] instance from a raw pointer. > + /// > + /// # Safety > + /// > + /// This function must only be called from the callbacks in `phy_driver`. > + unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self { > + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`. > + let ptr = ptr.cast::<Self>(); > + // SAFETY: by the function requirements the pointer is valid and we have unique access for > + // the duration of `'a`. > + unsafe { &mut *ptr } > + } > + > + /// Gets the id of the PHY. > + pub fn phy_id(&self) -> u32 { > + let phydev = self.0.get(); > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + unsafe { (*phydev).phy_id } > + } > + > + /// Gets the state of the PHY. > + pub fn state(&self) -> DeviceState { > + let phydev = self.0.get(); > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + let state = unsafe { (*phydev).state }; > + // TODO: this conversion code will be replaced with automatically generated code by bindgen > + // when it becomes possible. > + // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support). > + match state { > + bindings::phy_state_PHY_DOWN => DeviceState::Down, > + bindings::phy_state_PHY_READY => DeviceState::Ready, > + bindings::phy_state_PHY_HALTED => DeviceState::Halted, > + bindings::phy_state_PHY_ERROR => DeviceState::Error, > + bindings::phy_state_PHY_UP => DeviceState::Up, > + bindings::phy_state_PHY_RUNNING => DeviceState::Running, > + bindings::phy_state_PHY_NOLINK => DeviceState::NoLink, > + bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest, > + _ => DeviceState::Error, > + } > + } > + > + /// Gets the current link state. It returns true if the link is up. > + pub fn get_link(&self) -> bool { > + const LINK_IS_UP: u32 = 1; > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + let phydev = unsafe { *self.0.get() }; > + phydev.link() == LINK_IS_UP > + } Can we please change this name? I think Tomo is waiting for Andrew to give his OK. All the other getter functions already follow the Rust naming convention, so this one should as well. I think using `is_link_up` would be ideal, since `link()` reads a bit weird in code: if dev.link() { // ... } vs if dev.is_link_up() { // ... } > + > + /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled. Move the second sentence into a new line, it should not be part of the one-line summary. > + pub fn is_autoneg_enabled(&self) -> bool { > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + let phydev = unsafe { *self.0.get() }; > + phydev.autoneg() == bindings::AUTONEG_ENABLE > + } > + > + /// Gets the current auto-negotiation state. It returns true if auto-negotiation is completed. Same here. -- Cheers, Benno > + pub fn is_autoneg_completed(&self) -> bool { > + const AUTONEG_COMPLETED: u32 = 1; > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + let phydev = unsafe { *self.0.get() }; > + phydev.autoneg_complete() == AUTONEG_COMPLETED > + } [...] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-24 16:23 ` Benno Lossin @ 2023-10-24 16:44 ` Andrew Lunn 2023-10-25 1:10 ` FUJITA Tomonori 1 sibling, 0 replies; 22+ messages in thread From: Andrew Lunn @ 2023-10-24 16:44 UTC (permalink / raw) To: Benno Lossin Cc: FUJITA Tomonori, netdev, rust-for-linux, tmgross, miguel.ojeda.sandonis, wedsonaf, greg > Can we please change this name? I think Tomo is waiting for Andrew to > give his OK. All the other getter functions already follow the Rust > naming convention, so this one should as well. I think using > `is_link_up` would be ideal, since `link()` reads a bit weird in code: > > if dev.link() { > // ... > } > > vs > > if dev.is_link_up() { > // ... > } is_link_up() is fine for me. Andrew ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-24 16:23 ` Benno Lossin 2023-10-24 16:44 ` Andrew Lunn @ 2023-10-25 1:10 ` FUJITA Tomonori 2023-10-25 7:24 ` Benno Lossin 1 sibling, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 1:10 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Tue, 24 Oct 2023 16:23:20 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 24.10.23 02:58, 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_ABSTRACTIONS=y. >> >> This patch enables unstable const_maybe_uninit_zeroed feature for >> kernel crate to enable unsafe code to handle a constant value with >> uninitialized data. With the feature, the abstractions can initialize >> a phy_driver structure with zero easily; instead of initializing all >> the members by hand. It's supposed to be stable in the not so distant >> future. >> >> Link: https://github.com/rust-lang/rust/pull/116218 >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > [...] > >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >> new file mode 100644 >> index 000000000000..2d821c2475e1 >> --- /dev/null >> +++ b/rust/kernel/net/phy.rs >> @@ -0,0 +1,708 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> >> + >> +//! Network PHY device. >> +//! >> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). >> + >> +use crate::{ >> + bindings, >> + error::*, >> + prelude::{vtable, Pin}, >> + str::CStr, >> + types::Opaque, >> +}; >> +use core::marker::PhantomData; >> + >> +/// PHY state machine states. >> +/// >> +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state). > > Please use `rustfmt`, this line is 109 characters long. Hmm, `make rustfmt` doesn't warn on my env. `make rustfmtcheck` or `make rustdoc` doesn't. What's the limit? > Also it might make sense to use a relative link, since then it also > works offline (though you have to build the C docs). /// Corresponds to the kernel's [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). 101 characters too long? Then we could write: /// PHY state machine states. /// /// Corresponds to the kernel's /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). /// /// Some of PHY drivers access to the state of PHY's software state machine. >> + /// Gets the current link state. It returns true if the link is up. >> + pub fn get_link(&self) -> bool { >> + const LINK_IS_UP: u32 = 1; >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.link() == LINK_IS_UP >> + } > > Can we please change this name? I think Tomo is waiting for Andrew to > give his OK. All the other getter functions already follow the Rust > naming convention, so this one should as well. I think using > `is_link_up` would be ideal, since `link()` reads a bit weird in code: > > if dev.link() { > // ... > } > > vs > > if dev.is_link_up() { > // ... > } I'll go with is_link_up() >> + /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled. > > Move the second sentence into a new line, it should not be part of the > one-line summary. Oops, make one-line? /// Gets the current auto-negotiation configuration and returns true if auto-negotiation is enabled. Or /// Gets the current auto-negotiation configuration. /// /// It returns true if auto-negotiation is enabled. >> + pub fn is_autoneg_enabled(&self) -> bool { >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.autoneg() == bindings::AUTONEG_ENABLE >> + } >> + >> + /// Gets the current auto-negotiation state. It returns true if auto-negotiation is completed. > Same here. > > -- > Cheers, > Benno > >> + pub fn is_autoneg_completed(&self) -> bool { >> + const AUTONEG_COMPLETED: u32 = 1; >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.autoneg_complete() == AUTONEG_COMPLETED >> + } > [...] > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-25 1:10 ` FUJITA Tomonori @ 2023-10-25 7:24 ` Benno Lossin 2023-10-25 10:57 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-25 7:24 UTC (permalink / raw) To: FUJITA Tomonori Cc: netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 25.10.23 03:10, FUJITA Tomonori wrote: > On Tue, 24 Oct 2023 16:23:20 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 24.10.23 02:58, 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_ABSTRACTIONS=y. >>> >>> This patch enables unstable const_maybe_uninit_zeroed feature for >>> kernel crate to enable unsafe code to handle a constant value with >>> uninitialized data. With the feature, the abstractions can initialize >>> a phy_driver structure with zero easily; instead of initializing all >>> the members by hand. It's supposed to be stable in the not so distant >>> future. >>> >>> Link: https://github.com/rust-lang/rust/pull/116218 >>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> [...] >> >>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>> new file mode 100644 >>> index 000000000000..2d821c2475e1 >>> --- /dev/null >>> +++ b/rust/kernel/net/phy.rs >>> @@ -0,0 +1,708 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> >>> + >>> +//! Network PHY device. >>> +//! >>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). >>> + >>> +use crate::{ >>> + bindings, >>> + error::*, >>> + prelude::{vtable, Pin}, >>> + str::CStr, >>> + types::Opaque, >>> +}; >>> +use core::marker::PhantomData; >>> + >>> +/// PHY state machine states. >>> +/// >>> +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state). >> >> Please use `rustfmt`, this line is 109 characters long. > > Hmm, `make rustfmt` doesn't warn on my env. `make rustfmtcheck` or > `make rustdoc` doesn't. Sorry, my bad, `rustfmt` does not format comments, so you have to do them manually. > What's the limit? The limit is 100 characters. >> Also it might make sense to use a relative link, since then it also >> works offline (though you have to build the C docs). > > /// Corresponds to the kernel's [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). > > 101 characters too long? > > Then we could write: > > /// PHY state machine states. > /// > /// Corresponds to the kernel's > /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). > /// > /// Some of PHY drivers access to the state of PHY's software state machine. That is one way, another would be to do: /// PHY state machine states. /// /// Corresponds to the kernel's [`enum phy_state`]. /// /// Some of PHY drivers access to the state of PHY's software state machine. /// /// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state But as I noted before, then people who only build the rustdoc will not be able to view it. I personally would prefer to have the correct link offline, but do not know about others. >>> + /// Gets the current link state. It returns true if the link is up. I just noticed this as well, here also please split the line. >>> + pub fn get_link(&self) -> bool { >>> + const LINK_IS_UP: u32 = 1; >>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >>> + let phydev = unsafe { *self.0.get() }; >>> + phydev.link() == LINK_IS_UP >>> + } >> >> Can we please change this name? I think Tomo is waiting for Andrew to >> give his OK. All the other getter functions already follow the Rust >> naming convention, so this one should as well. I think using >> `is_link_up` would be ideal, since `link()` reads a bit weird in code: >> >> if dev.link() { >> // ... >> } >> >> vs >> >> if dev.is_link_up() { >> // ... >> } > > I'll go with is_link_up() > > >>> + /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled. >> >> Move the second sentence into a new line, it should not be part of the >> one-line summary. > > Oops, make one-line? > > /// Gets the current auto-negotiation configuration and returns true if auto-negotiation is enabled. > > Or > > /// Gets the current auto-negotiation configuration. > /// > /// It returns true if auto-negotiation is enabled. I would prefer this, since the function name itself already is pretty self-explanatory. If someone really wants to understand it, they probably have to read the source code. -- Cheers, Benno ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-25 7:24 ` Benno Lossin @ 2023-10-25 10:57 ` FUJITA Tomonori 2023-10-25 14:54 ` Benno Lossin 0 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 10:57 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Wed, 25 Oct 2023 07:24:00 +0000 Benno Lossin <benno.lossin@proton.me> wrote: >> /// PHY state machine states. >> /// >> /// Corresponds to the kernel's >> /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). >> /// >> /// Some of PHY drivers access to the state of PHY's software state machine. > > That is one way, another would be to do: This looks nicer. > /// PHY state machine states. > /// > /// Corresponds to the kernel's [`enum phy_state`]. > /// > /// Some of PHY drivers access to the state of PHY's software state machine. > /// > /// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state > > But as I noted before, then people who only build the rustdoc will not > be able to view it. I personally would prefer to have the correct link > offline, but do not know about others. I prefer a link to online docs but either is fine by me. You prefer a link to a header file like? /// [`enum phy_state`]: ../../../include/linux/phy.h >>>> + /// Gets the current link state. It returns true if the link is up. > > I just noticed this as well, here also please split the line. Fixed all. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-25 10:57 ` FUJITA Tomonori @ 2023-10-25 14:54 ` Benno Lossin 2023-10-25 23:19 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-25 14:54 UTC (permalink / raw) To: FUJITA Tomonori Cc: netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 25.10.23 12:57, FUJITA Tomonori wrote: > On Wed, 25 Oct 2023 07:24:00 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >>> /// PHY state machine states. >>> /// >>> /// Corresponds to the kernel's >>> /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). >>> /// >>> /// Some of PHY drivers access to the state of PHY's software state machine. >> >> That is one way, another would be to do: > > This looks nicer. > >> /// PHY state machine states. >> /// >> /// Corresponds to the kernel's [`enum phy_state`]. >> /// >> /// Some of PHY drivers access to the state of PHY's software state machine. >> /// >> /// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state >> >> But as I noted before, then people who only build the rustdoc will not >> be able to view it. I personally would prefer to have the correct link >> offline, but do not know about others. > > I prefer a link to online docs but either is fine by me. You prefer a > link to a header file like? > > /// [`enum phy_state`]: ../../../include/linux/phy.h No. I think the header file should be mentioned for the whole abstraction. I personally always use [1] to search for C symbols, so the name suffices for me. And the documentation is (for me at least) less accessible (if there is a nice tool for that as well, please tell me). [1]: https://elixir.bootlin.com/linux/latest/source -- Cheers, Benno ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 2023-10-25 14:54 ` Benno Lossin @ 2023-10-25 23:19 ` FUJITA Tomonori 0 siblings, 0 replies; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 23:19 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Wed, 25 Oct 2023 14:54:42 +0000 Benno Lossin <benno.lossin@proton.me> wrote: >>> /// PHY state machine states. >>> /// >>> /// Corresponds to the kernel's [`enum phy_state`]. >>> /// >>> /// Some of PHY drivers access to the state of PHY's software state machine. >>> /// >>> /// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state >>> >>> But as I noted before, then people who only build the rustdoc will not >>> be able to view it. I personally would prefer to have the correct link >>> offline, but do not know about others. >> >> I prefer a link to online docs but either is fine by me. You prefer a >> link to a header file like? >> >> /// [`enum phy_state`]: ../../../include/linux/phy.h > > No. I think the header file should be mentioned for the whole > abstraction. I personally always use [1] to search for C symbols, so > the name suffices for me. And the documentation is (for me at least) > less accessible (if there is a nice tool for that as well, please tell me). > > [1]: https://elixir.bootlin.com/linux/latest/source Understood. I use links to the PHY header file like other Rust abstractions do. IMHO, adding a link to kerneldoc would be nice but consistency in all the abstractions is needed. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 1/5] rust: core " FUJITA Tomonori @ 2023-10-24 0:58 ` FUJITA Tomonori 2023-10-24 16:28 ` Benno Lossin 2023-10-24 0:58 ` [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking FUJITA Tomonori ` (2 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw) To: netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, benno.lossin, wedsonaf, greg This macro creates an array of kernel's `struct phy_driver` and registers it. This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information for module loading into the module binary file. A PHY driver should use this macro. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 2d821c2475e1..f346b2b4d3cb 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { } } } + +/// Declares a kernel module for PHYs drivers. +/// +/// This creates a static array of kernel's `struct phy_driver` and registers it. +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. +/// +/// # Examples +/// +/// ```ignore +/// use kernel::c_str; +/// use kernel::net::phy::{self, DeviceId}; +/// use kernel::prelude::*; +/// +/// kernel::module_phy_driver! { +/// drivers: [PhyAX88772A], +/// device_table: [ +/// DeviceId::new_with_driver::<PhyAX88772A>(), +/// ], +/// name: "rust_asix_phy", +/// author: "Rust for Linux Contributors", +/// description: "Rust Asix PHYs driver", +/// license: "GPL", +/// } +/// +/// struct PhyAX88772A; +/// +/// impl phy::Driver for PhyAX88772A { +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); +/// } +/// ``` +/// +/// This expands to the following code: +/// +/// ```ignore +/// use kernel::c_str; +/// use kernel::net::phy::{self, DeviceId}; +/// use kernel::prelude::*; +/// +/// struct Module { +/// _reg: ::kernel::net::phy::Registration, +/// } +/// +/// module! { +/// type: Module, +/// name: "rust_asix_phy", +/// author: "Rust for Linux Contributors", +/// description: "Rust Asix PHYs driver", +/// license: "GPL", +/// } +/// +/// const _: () = { +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), +/// ]; +/// +/// impl ::kernel::Module for Module { +/// fn init(module: &'static ThisModule) -> Result<Self> { +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; +/// Ok(Module { _reg: reg }) +/// } +/// } +/// } +/// +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ +/// ::kernel::bindings::mdio_device_id { +/// phy_id: 0x003b1861, +/// phy_id_mask: 0xffffffff, +/// }, +/// ::kernel::bindings::mdio_device_id { +/// phy_id: 0, +/// phy_id_mask: 0, +/// } +/// ]; +/// ``` +#[macro_export] +macro_rules! module_phy_driver { + (@replace_expr $_t:tt $sub:expr) => {$sub}; + + (@count_devices $($x:expr),*) => { + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))* + }; + + (@device_table [$($dev:expr),+]) => { + #[no_mangle] + static __mod_mdio__phydev_device_table: [ + ::kernel::bindings::mdio_device_id; + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 + ] = [ + $($dev.mdio_device_id()),+, + ::kernel::bindings::mdio_device_id { + phy_id: 0, + phy_id_mask: 0 + } + ]; + }; + + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { + struct Module { + _reg: ::kernel::net::phy::Registration, + } + + $crate::prelude::module! { + type: Module, + $($f)* + } + + const _: () = { + static mut DRIVERS: [ + ::kernel::net::phy::DriverVTable; + $crate::module_phy_driver!(@count_devices $($driver),+) + ] = [ + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ + ]; + + impl ::kernel::Module for Module { + fn init(module: &'static ThisModule) -> Result<Self> { + // SAFETY: The anonymous constant guarantees that nobody else can access the `DRIVERS` static. + // The array is used only in the C side. + let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; + Ok(Module { _reg: reg }) + } + } + }; + + $crate::module_phy_driver!(@device_table [$($dev),+]); + } +} -- 2.34.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-24 0:58 ` [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro FUJITA Tomonori @ 2023-10-24 16:28 ` Benno Lossin 2023-10-25 0:02 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-24 16:28 UTC (permalink / raw) To: FUJITA Tomonori, netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 24.10.23 02:58, FUJITA Tomonori wrote: > This macro creates an array of kernel's `struct phy_driver` and > registers it. This also corresponds to the kernel's > `MODULE_DEVICE_TABLE` macro, which embeds the information for module > loading into the module binary file. > > A PHY driver should use this macro. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > index 2d821c2475e1..f346b2b4d3cb 100644 > --- a/rust/kernel/net/phy.rs > +++ b/rust/kernel/net/phy.rs > @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { > } > } > } > + > +/// Declares a kernel module for PHYs drivers. > +/// > +/// This creates a static array of kernel's `struct phy_driver` and registers it. > +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information > +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. > +/// > +/// # Examples > +/// > +/// ```ignore Is this example ignored, because it does not compile? I think Wedson was wrapping his example with `module!` inside of a module, so maybe try that? > +/// use kernel::c_str; > +/// use kernel::net::phy::{self, DeviceId}; > +/// use kernel::prelude::*; > +/// > +/// kernel::module_phy_driver! { > +/// drivers: [PhyAX88772A], > +/// device_table: [ > +/// DeviceId::new_with_driver::<PhyAX88772A>(), > +/// ], > +/// name: "rust_asix_phy", > +/// author: "Rust for Linux Contributors", > +/// description: "Rust Asix PHYs driver", > +/// license: "GPL", > +/// } > +/// > +/// struct PhyAX88772A; > +/// > +/// impl phy::Driver for PhyAX88772A { > +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); > +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); > +/// } > +/// ``` > +/// > +/// This expands to the following code: > +/// > +/// ```ignore > +/// use kernel::c_str; > +/// use kernel::net::phy::{self, DeviceId}; > +/// use kernel::prelude::*; > +/// > +/// struct Module { > +/// _reg: ::kernel::net::phy::Registration, > +/// } > +/// > +/// module! { > +/// type: Module, > +/// name: "rust_asix_phy", > +/// author: "Rust for Linux Contributors", > +/// description: "Rust Asix PHYs driver", > +/// license: "GPL", > +/// } > +/// > +/// const _: () = { > +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ > +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), > +/// ]; > +/// > +/// impl ::kernel::Module for Module { > +/// fn init(module: &'static ThisModule) -> Result<Self> { > +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; Can you please format this as well? > +/// Ok(Module { _reg: reg }) > +/// } > +/// } > +/// } > +/// > +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ > +/// ::kernel::bindings::mdio_device_id { > +/// phy_id: 0x003b1861, > +/// phy_id_mask: 0xffffffff, > +/// }, > +/// ::kernel::bindings::mdio_device_id { > +/// phy_id: 0, > +/// phy_id_mask: 0, > +/// } > +/// ]; > +/// ``` > +#[macro_export] > +macro_rules! module_phy_driver { > + (@replace_expr $_t:tt $sub:expr) => {$sub}; > + > + (@count_devices $($x:expr),*) => { > + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))* > + }; > + > + (@device_table [$($dev:expr),+]) => { > + #[no_mangle] > + static __mod_mdio__phydev_device_table: [ > + ::kernel::bindings::mdio_device_id; > + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 > + ] = [ > + $($dev.mdio_device_id()),+, > + ::kernel::bindings::mdio_device_id { > + phy_id: 0, > + phy_id_mask: 0 > + } > + ]; > + }; > + > + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { > + struct Module { > + _reg: ::kernel::net::phy::Registration, > + } > + > + $crate::prelude::module! { > + type: Module, > + $($f)* > + } > + > + const _: () = { > + static mut DRIVERS: [ > + ::kernel::net::phy::DriverVTable; > + $crate::module_phy_driver!(@count_devices $($driver),+) > + ] = [ > + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ > + ]; > + > + impl ::kernel::Module for Module { > + fn init(module: &'static ThisModule) -> Result<Self> { > + // SAFETY: The anonymous constant guarantees that nobody else can access the `DRIVERS` static. > + // The array is used only in the C side. > + let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; Can you put the safe operations outside of the `unsafe` block? Since `rustfmt` does not work well within `macro_rules`, you have to manually format this code. One trick I use to do this is to replace all meta variables with normal variables and just put it in `rustfmt` and then revert the replacement in the output. -- Cheers, Benno > + Ok(Module { _reg: reg }) > + } > + } > + }; > + > + $crate::module_phy_driver!(@device_table [$($dev),+]); > + } > +} > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-24 16:28 ` Benno Lossin @ 2023-10-25 0:02 ` FUJITA Tomonori 2023-10-25 7:29 ` Benno Lossin 0 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 0:02 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Tue, 24 Oct 2023 16:28:02 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 24.10.23 02:58, FUJITA Tomonori wrote: >> This macro creates an array of kernel's `struct phy_driver` and >> registers it. This also corresponds to the kernel's >> `MODULE_DEVICE_TABLE` macro, which embeds the information for module >> loading into the module binary file. >> >> A PHY driver should use this macro. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> --- >> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 129 insertions(+) >> >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >> index 2d821c2475e1..f346b2b4d3cb 100644 >> --- a/rust/kernel/net/phy.rs >> +++ b/rust/kernel/net/phy.rs >> @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { >> } >> } >> } >> + >> +/// Declares a kernel module for PHYs drivers. >> +/// >> +/// This creates a static array of kernel's `struct phy_driver` and registers it. >> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information >> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. >> +/// >> +/// # Examples >> +/// >> +/// ```ignore > > Is this example ignored, because it does not compile? The old version can't be compiled but the current version can so I'll drop ignore. > I think Wedson was wrapping his example with `module!` inside > of a module, so maybe try that? I'm not sure what you mean. >> +/// use kernel::c_str; >> +/// use kernel::net::phy::{self, DeviceId}; >> +/// use kernel::prelude::*; >> +/// >> +/// kernel::module_phy_driver! { >> +/// drivers: [PhyAX88772A], >> +/// device_table: [ >> +/// DeviceId::new_with_driver::<PhyAX88772A>(), >> +/// ], >> +/// name: "rust_asix_phy", >> +/// author: "Rust for Linux Contributors", >> +/// description: "Rust Asix PHYs driver", >> +/// license: "GPL", >> +/// } >> +/// >> +/// struct PhyAX88772A; >> +/// >> +/// impl phy::Driver for PhyAX88772A { >> +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); >> +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); >> +/// } >> +/// ``` >> +/// >> +/// This expands to the following code: >> +/// >> +/// ```ignore >> +/// use kernel::c_str; >> +/// use kernel::net::phy::{self, DeviceId}; >> +/// use kernel::prelude::*; >> +/// >> +/// struct Module { >> +/// _reg: ::kernel::net::phy::Registration, >> +/// } >> +/// >> +/// module! { >> +/// type: Module, >> +/// name: "rust_asix_phy", >> +/// author: "Rust for Linux Contributors", >> +/// description: "Rust Asix PHYs driver", >> +/// license: "GPL", >> +/// } >> +/// >> +/// const _: () = { >> +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ >> +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), >> +/// ]; >> +/// >> +/// impl ::kernel::Module for Module { >> +/// fn init(module: &'static ThisModule) -> Result<Self> { >> +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; > > Can you please format this as well? Done. >> +/// Ok(Module { _reg: reg }) >> +/// } >> +/// } >> +/// } >> +/// >> +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ >> +/// ::kernel::bindings::mdio_device_id { >> +/// phy_id: 0x003b1861, >> +/// phy_id_mask: 0xffffffff, >> +/// }, >> +/// ::kernel::bindings::mdio_device_id { >> +/// phy_id: 0, >> +/// phy_id_mask: 0, >> +/// } >> +/// ]; >> +/// ``` >> +#[macro_export] >> +macro_rules! module_phy_driver { >> + (@replace_expr $_t:tt $sub:expr) => {$sub}; >> + >> + (@count_devices $($x:expr),*) => { >> + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))* >> + }; >> + >> + (@device_table [$($dev:expr),+]) => { >> + #[no_mangle] >> + static __mod_mdio__phydev_device_table: [ >> + ::kernel::bindings::mdio_device_id; >> + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 >> + ] = [ >> + $($dev.mdio_device_id()),+, >> + ::kernel::bindings::mdio_device_id { >> + phy_id: 0, >> + phy_id_mask: 0 >> + } >> + ]; >> + }; >> + >> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { >> + struct Module { >> + _reg: ::kernel::net::phy::Registration, >> + } >> + >> + $crate::prelude::module! { >> + type: Module, >> + $($f)* >> + } >> + >> + const _: () = { >> + static mut DRIVERS: [ >> + ::kernel::net::phy::DriverVTable; >> + $crate::module_phy_driver!(@count_devices $($driver),+) >> + ] = [ >> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >> + ]; >> + >> + impl ::kernel::Module for Module { >> + fn init(module: &'static ThisModule) -> Result<Self> { >> + // SAFETY: The anonymous constant guarantees that nobody else can access the `DRIVERS` static. >> + // The array is used only in the C side. >> + let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; > > Can you put the safe operations outside of the `unsafe` block? fn init(module: &'static ThisModule) -> Result<Self> { // SAFETY: The anonymous constant guarantees that nobody else can access // the `DRIVERS` static. The array is used only in the C side. let mut reg = ::kernel::net::phy::Registration::register( module, core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), )?; Ok(Module { _reg: reg }) } > Since `rustfmt` does not work well within `macro_rules`, you > have to manually format this code. One trick I use to do this > is to replace all meta variables with normal variables and > just put it in `rustfmt` and then revert the replacement in > the output. Understood, done. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-25 0:02 ` FUJITA Tomonori @ 2023-10-25 7:29 ` Benno Lossin 2023-10-25 7:57 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-25 7:29 UTC (permalink / raw) To: FUJITA Tomonori Cc: netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 25.10.23 02:02, FUJITA Tomonori wrote: > On Tue, 24 Oct 2023 16:28:02 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 24.10.23 02:58, FUJITA Tomonori wrote: >>> This macro creates an array of kernel's `struct phy_driver` and >>> registers it. This also corresponds to the kernel's >>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module >>> loading into the module binary file. >>> >>> A PHY driver should use this macro. >>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >>> --- >>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 129 insertions(+) >>> >>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>> index 2d821c2475e1..f346b2b4d3cb 100644 >>> --- a/rust/kernel/net/phy.rs >>> +++ b/rust/kernel/net/phy.rs >>> @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { >>> } >>> } >>> } >>> + >>> +/// Declares a kernel module for PHYs drivers. >>> +/// >>> +/// This creates a static array of kernel's `struct phy_driver` and registers it. >>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information >>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. >>> +/// >>> +/// # Examples >>> +/// >>> +/// ```ignore >> >> Is this example ignored, because it does not compile? > > The old version can't be compiled but the current version can so I'll > drop ignore. > > >> I think Wedson was wrapping his example with `module!` inside >> of a module, so maybe try that? > > I'm not sure what you mean. Wedson did this [1], note the `# mod module_fs_sample`: /// # Examples /// /// ``` /// # mod module_fs_sample { /// use kernel::prelude::*; /// use kernel::{c_str, fs}; /// /// kernel::module_fs! { /// type: MyFs, /// name: "myfs", /// author: "Rust for Linux Contributors", /// description: "My Rust fs", /// license: "GPL", /// } /// /// struct MyFs; /// impl fs::FileSystem for MyFs { /// const NAME: &'static CStr = c_str!("myfs"); /// } /// # } /// ``` [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 >>> +/// use kernel::c_str; >>> +/// use kernel::net::phy::{self, DeviceId}; >>> +/// use kernel::prelude::*; [...] >>> + const _: () = { >>> + static mut DRIVERS: [ >>> + ::kernel::net::phy::DriverVTable; >>> + $crate::module_phy_driver!(@count_devices $($driver),+) >>> + ] = [ >>> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >>> + ]; >>> + >>> + impl ::kernel::Module for Module { >>> + fn init(module: &'static ThisModule) -> Result<Self> { >>> + // SAFETY: The anonymous constant guarantees that nobody else can access the `DRIVERS` static. >>> + // The array is used only in the C side. >>> + let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; >> >> Can you put the safe operations outside of the `unsafe` block? > > fn init(module: &'static ThisModule) -> Result<Self> { > // SAFETY: The anonymous constant guarantees that nobody else can access > // the `DRIVERS` static. The array is used only in the C side. > let mut reg = ::kernel::net::phy::Registration::register( > module, > core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), > )?; > Ok(Module { _reg: reg }) > } Here the `unsafe` block and the `SAFETY` comment are pretty far away. What about this?: fn init(module: &'static ThisModule) -> Result<Self> { // SAFETY: The anonymous constant guarantees that nobody else can access // the `DRIVERS` static. The array is used only in the C side. let drivers = unsafe { &mut DRIVERS }; let mut reg = ::kernel::net::phy::Registration::register(module, ::core::pin::Pin::static_mut(drivers))?; Ok(Module { _reg: reg }) } Also note that I added `::` to the front of `core`. -- Cheers, Benno ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-25 7:29 ` Benno Lossin @ 2023-10-25 7:57 ` FUJITA Tomonori 2023-10-25 8:08 ` Benno Lossin 0 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 7:57 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Wed, 25 Oct 2023 07:29:32 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 25.10.23 02:02, FUJITA Tomonori wrote: >> On Tue, 24 Oct 2023 16:28:02 +0000 >> Benno Lossin <benno.lossin@proton.me> wrote: >> >>> On 24.10.23 02:58, FUJITA Tomonori wrote: >>>> This macro creates an array of kernel's `struct phy_driver` and >>>> registers it. This also corresponds to the kernel's >>>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module >>>> loading into the module binary file. >>>> >>>> A PHY driver should use this macro. >>>> >>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >>>> --- >>>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 129 insertions(+) >>>> >>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>>> index 2d821c2475e1..f346b2b4d3cb 100644 >>>> --- a/rust/kernel/net/phy.rs >>>> +++ b/rust/kernel/net/phy.rs >>>> @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { >>>> } >>>> } >>>> } >>>> + >>>> +/// Declares a kernel module for PHYs drivers. >>>> +/// >>>> +/// This creates a static array of kernel's `struct phy_driver` and registers it. >>>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information >>>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. >>>> +/// >>>> +/// # Examples >>>> +/// >>>> +/// ```ignore >>> >>> Is this example ignored, because it does not compile? >> >> The old version can't be compiled but the current version can so I'll >> drop ignore. >> >> >>> I think Wedson was wrapping his example with `module!` inside >>> of a module, so maybe try that? >> >> I'm not sure what you mean. > > Wedson did this [1], note the `# mod module_fs_sample`: > > /// # Examples > /// > /// ``` > /// # mod module_fs_sample { > /// use kernel::prelude::*; > /// use kernel::{c_str, fs}; > /// > /// kernel::module_fs! { > /// type: MyFs, > /// name: "myfs", > /// author: "Rust for Linux Contributors", > /// description: "My Rust fs", > /// license: "GPL", > /// } > /// > /// struct MyFs; > /// impl fs::FileSystem for MyFs { > /// const NAME: &'static CStr = c_str!("myfs"); > /// } > /// # } > /// ``` > > [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 You are suggesting like the following? /// # Examples /// /// ``` /// # mod module_phy_driver_sample { ... /// # } /// ``` What benefits? >>>> + const _: () = { >>>> + static mut DRIVERS: [ >>>> + ::kernel::net::phy::DriverVTable; >>>> + $crate::module_phy_driver!(@count_devices $($driver),+) >>>> + ] = [ >>>> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >>>> + ]; >>>> + >>>> + impl ::kernel::Module for Module { >>>> + fn init(module: &'static ThisModule) -> Result<Self> { >>>> + // SAFETY: The anonymous constant guarantees that nobody else can access the `DRIVERS` static. >>>> + // The array is used only in the C side. >>>> + let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; >>> >>> Can you put the safe operations outside of the `unsafe` block? >> >> fn init(module: &'static ThisModule) -> Result<Self> { >> // SAFETY: The anonymous constant guarantees that nobody else can access >> // the `DRIVERS` static. The array is used only in the C side. >> let mut reg = ::kernel::net::phy::Registration::register( >> module, >> core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), >> )?; >> Ok(Module { _reg: reg }) >> } > > Here the `unsafe` block and the `SAFETY` comment are pretty far away. > What about this?: > > fn init(module: &'static ThisModule) -> Result<Self> { > // SAFETY: The anonymous constant guarantees that nobody else can access > // the `DRIVERS` static. The array is used only in the C side. > let drivers = unsafe { &mut DRIVERS }; > let mut reg = > ::kernel::net::phy::Registration::register(module, ::core::pin::Pin::static_mut(drivers))?; > Ok(Module { _reg: reg }) > } > > Also note that I added `::` to the front of `core`. I see. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-25 7:57 ` FUJITA Tomonori @ 2023-10-25 8:08 ` Benno Lossin 2023-10-25 9:13 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-25 8:08 UTC (permalink / raw) To: FUJITA Tomonori Cc: netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On 25.10.23 09:57, FUJITA Tomonori wrote: > On Wed, 25 Oct 2023 07:29:32 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 25.10.23 02:02, FUJITA Tomonori wrote: >>> On Tue, 24 Oct 2023 16:28:02 +0000 >>> Benno Lossin <benno.lossin@proton.me> wrote: >>> >>>> On 24.10.23 02:58, FUJITA Tomonori wrote: >>>>> This macro creates an array of kernel's `struct phy_driver` and >>>>> registers it. This also corresponds to the kernel's >>>>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module >>>>> loading into the module binary file. >>>>> >>>>> A PHY driver should use this macro. >>>>> >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >>>>> --- >>>>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 129 insertions(+) >>>>> >>>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>>>> index 2d821c2475e1..f346b2b4d3cb 100644 >>>>> --- a/rust/kernel/net/phy.rs >>>>> +++ b/rust/kernel/net/phy.rs >>>>> @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 { >>>>> } >>>>> } >>>>> } >>>>> + >>>>> +/// Declares a kernel module for PHYs drivers. >>>>> +/// >>>>> +/// This creates a static array of kernel's `struct phy_driver` and registers it. >>>>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information >>>>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. >>>>> +/// >>>>> +/// # Examples >>>>> +/// >>>>> +/// ```ignore >>>> >>>> Is this example ignored, because it does not compile? >>> >>> The old version can't be compiled but the current version can so I'll >>> drop ignore. >>> >>> >>>> I think Wedson was wrapping his example with `module!` inside >>>> of a module, so maybe try that? >>> >>> I'm not sure what you mean. >> >> Wedson did this [1], note the `# mod module_fs_sample`: >> >> /// # Examples >> /// >> /// ``` >> /// # mod module_fs_sample { >> /// use kernel::prelude::*; >> /// use kernel::{c_str, fs}; >> /// >> /// kernel::module_fs! { >> /// type: MyFs, >> /// name: "myfs", >> /// author: "Rust for Linux Contributors", >> /// description: "My Rust fs", >> /// license: "GPL", >> /// } >> /// >> /// struct MyFs; >> /// impl fs::FileSystem for MyFs { >> /// const NAME: &'static CStr = c_str!("myfs"); >> /// } >> /// # } >> /// ``` >> >> [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 > > You are suggesting like the following? > > /// # Examples > /// > /// ``` > /// # mod module_phy_driver_sample { > ... > /// # } > /// ``` > > What benefits? IIRC Wedson mentioned that without that it did not compile. So if it compiles for you without it, I would not add it. -- Cheers, Benno ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro 2023-10-25 8:08 ` Benno Lossin @ 2023-10-25 9:13 ` FUJITA Tomonori 0 siblings, 0 replies; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 9:13 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg On Wed, 25 Oct 2023 08:08:53 +0000 Benno Lossin <benno.lossin@proton.me> wrote: >>>>>> +/// Declares a kernel module for PHYs drivers. >>>>>> +/// >>>>>> +/// This creates a static array of kernel's `struct phy_driver` and registers it. >>>>>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information >>>>>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`. >>>>>> +/// >>>>>> +/// # Examples >>>>>> +/// >>>>>> +/// ```ignore >>>>> >>>>> Is this example ignored, because it does not compile? >>>> >>>> The old version can't be compiled but the current version can so I'll >>>> drop ignore. >>>> >>>> >>>>> I think Wedson was wrapping his example with `module!` inside >>>>> of a module, so maybe try that? >>>> >>>> I'm not sure what you mean. >>> >>> Wedson did this [1], note the `# mod module_fs_sample`: >>> >>> /// # Examples >>> /// >>> /// ``` >>> /// # mod module_fs_sample { >>> /// use kernel::prelude::*; >>> /// use kernel::{c_str, fs}; >>> /// >>> /// kernel::module_fs! { >>> /// type: MyFs, >>> /// name: "myfs", >>> /// author: "Rust for Linux Contributors", >>> /// description: "My Rust fs", >>> /// license: "GPL", >>> /// } >>> /// >>> /// struct MyFs; >>> /// impl fs::FileSystem for MyFs { >>> /// const NAME: &'static CStr = c_str!("myfs"); >>> /// } >>> /// # } >>> /// ``` >>> >>> [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 >> >> You are suggesting like the following? >> >> /// # Examples >> /// >> /// ``` >> /// # mod module_phy_driver_sample { >> ... >> /// # } >> /// ``` >> >> What benefits? > > IIRC Wedson mentioned that without that it did not compile. So > if it compiles for you without it, I would not add it. Ah, it's necessary. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 1/5] rust: core " FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro FUJITA Tomonori @ 2023-10-24 0:58 ` FUJITA Tomonori 2023-10-24 16:29 ` Benno Lossin 2023-10-24 0:58 ` [PATCH net-next v6 4/5] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 5/5] net: phy: add Rust Asix PHY driver FUJITA Tomonori 4 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw) To: netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, benno.lossin, wedsonaf, greg, Miguel Ojeda From: Miguel Ojeda <ojeda@kernel.org> error[E0005]: refutable pattern in function argument --> rust/bindings/bindings_enum_check.rs:29:6 | 29 | (phy_state::PHY_DOWN | ______^ 30 | | | phy_state::PHY_READY 31 | | | phy_state::PHY_HALTED 32 | | | phy_state::PHY_ERROR ... | 35 | | | phy_state::PHY_NOLINK 36 | | | phy_state::PHY_CABLETEST): phy_state, | |______________________________^ pattern `phy_state::PHY_NEW` not covered | note: `phy_state` defined here --> rust/bindings/bindings_generated_enum_check.rs:60739:10 | 60739 | pub enum phy_state { | ^^^^^^^^^ ... 60745 | PHY_NEW = 5, | ------- not covered = note: the matched value is of type `phy_state` Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/.gitignore | 1 + rust/Makefile | 14 +++++++++++ rust/bindings/bindings_enum_check.rs | 36 ++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 rust/bindings/bindings_enum_check.rs diff --git a/rust/.gitignore b/rust/.gitignore index d3829ffab80b..1a76ad0d6603 100644 --- a/rust/.gitignore +++ b/rust/.gitignore @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 bindings_generated.rs +bindings_generated_enum_check.rs bindings_helpers_generated.rs doctests_kernel_generated.rs doctests_kernel_generated_kunit.c diff --git a/rust/Makefile b/rust/Makefile index 87958e864be0..a622111c8c50 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -15,6 +15,7 @@ always-$(CONFIG_RUST) += libmacros.so no-clean-files += libmacros.so always-$(CONFIG_RUST) += bindings/bindings_generated.rs bindings/bindings_helpers_generated.rs +always-$(CONFIG_RUST) += bindings/bindings_generated_enum_check.rs obj-$(CONFIG_RUST) += alloc.o bindings.o kernel.o always-$(CONFIG_RUST) += exports_alloc_generated.h exports_bindings_generated.h \ exports_kernel_generated.h @@ -341,6 +342,19 @@ $(obj)/bindings/bindings_generated.rs: $(src)/bindings/bindings_helper.h \ $(src)/bindgen_parameters FORCE $(call if_changed_dep,bindgen) +$(obj)/bindings/bindings_generated_enum_check.rs: private bindgen_target_flags = \ + $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters) \ + --default-enum-style rust +$(obj)/bindings/bindings_generated_enum_check.rs: private bindgen_target_extra = ; \ + OBJTREE=$(abspath $(objtree)) $(RUSTC_OR_CLIPPY) $(rust_flags) $(rustc_target_flags) \ + --crate-type rlib -L$(objtree)/$(obj) \ + --emit=dep-info=$(obj)/bindings/.bindings_enum_check.rs.d \ + --emit=metadata=$(obj)/bindings/libbindings_enum_check.rmeta \ + --crate-name enum_check $(srctree)/$(src)/bindings/bindings_enum_check.rs +$(obj)/bindings/bindings_generated_enum_check.rs: $(src)/bindings/bindings_helper.h \ + $(src)/bindings/bindings_enum_check.rs $(src)/bindgen_parameters FORCE + $(call if_changed_dep,bindgen) + $(obj)/uapi/uapi_generated.rs: private bindgen_target_flags = \ $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters) $(obj)/uapi/uapi_generated.rs: $(src)/uapi/uapi_helper.h \ diff --git a/rust/bindings/bindings_enum_check.rs b/rust/bindings/bindings_enum_check.rs new file mode 100644 index 000000000000..eef7e9ca3c54 --- /dev/null +++ b/rust/bindings/bindings_enum_check.rs @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Bindings' enum exhaustiveness check. +//! +//! Eventually, this should be replaced by a safe version of `--rustified-enum`, see +//! https://github.com/rust-lang/rust-bindgen/issues/2646. + +#![no_std] +#![allow( + clippy::all, + dead_code, + missing_docs, + non_camel_case_types, + non_upper_case_globals, + non_snake_case, + improper_ctypes, + unreachable_pub, + unsafe_op_in_unsafe_fn +)] + +include!(concat!( + env!("OBJTREE"), + "/rust/bindings/bindings_generated_enum_check.rs" +)); + +fn check_phy_state( + (phy_state::PHY_DOWN + | phy_state::PHY_READY + | phy_state::PHY_HALTED + | phy_state::PHY_ERROR + | phy_state::PHY_UP + | phy_state::PHY_RUNNING + | phy_state::PHY_NOLINK + | phy_state::PHY_CABLETEST): phy_state, +) { +} -- 2.34.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking 2023-10-24 0:58 ` [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking FUJITA Tomonori @ 2023-10-24 16:29 ` Benno Lossin 2023-10-25 1:33 ` FUJITA Tomonori 0 siblings, 1 reply; 22+ messages in thread From: Benno Lossin @ 2023-10-24 16:29 UTC (permalink / raw) To: FUJITA Tomonori, netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg, Miguel Ojeda On 24.10.23 02:58, FUJITA Tomonori wrote: > From: Miguel Ojeda <ojeda@kernel.org> I think this commit message should also explain what it is doing and not only the error below. -- Cheers, Benno > error[E0005]: refutable pattern in function argument > --> rust/bindings/bindings_enum_check.rs:29:6 > | > 29 | (phy_state::PHY_DOWN > | ______^ > 30 | | | phy_state::PHY_READY > 31 | | | phy_state::PHY_HALTED > 32 | | | phy_state::PHY_ERROR > ... | > 35 | | | phy_state::PHY_NOLINK > 36 | | | phy_state::PHY_CABLETEST): phy_state, > | |______________________________^ pattern `phy_state::PHY_NEW` not covered > | > note: `phy_state` defined here > --> rust/bindings/bindings_generated_enum_check.rs:60739:10 > | > 60739 | pub enum phy_state { > | ^^^^^^^^^ > ... > 60745 | PHY_NEW = 5, > | ------- not covered > = note: the matched value is of type `phy_state` > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- [...] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking 2023-10-24 16:29 ` Benno Lossin @ 2023-10-25 1:33 ` FUJITA Tomonori 2023-10-25 7:36 ` Benno Lossin 0 siblings, 1 reply; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-25 1:33 UTC (permalink / raw) To: benno.lossin Cc: fujita.tomonori, netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg, ojeda On Tue, 24 Oct 2023 16:29:16 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 24.10.23 02:58, FUJITA Tomonori wrote: >> From: Miguel Ojeda <ojeda@kernel.org> > > I think this commit message should also explain what it is > doing and not only the error below. Looks ok? This patch makes sure that the C's enum phy_state is sync with Rust sides. If not, compiling fails. Note that this is a temporary solution. It will be replaced with bindgen when it supports generating the enum conversion code. 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` ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking 2023-10-25 1:33 ` FUJITA Tomonori @ 2023-10-25 7:36 ` Benno Lossin 0 siblings, 0 replies; 22+ messages in thread From: Benno Lossin @ 2023-10-25 7:36 UTC (permalink / raw) To: FUJITA Tomonori Cc: netdev, rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, wedsonaf, greg, ojeda On 25.10.23 03:33, FUJITA Tomonori wrote: > On Tue, 24 Oct 2023 16:29:16 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 24.10.23 02:58, FUJITA Tomonori wrote: >>> From: Miguel Ojeda <ojeda@kernel.org> >> >> I think this commit message should also explain what it is >> doing and not only the error below. > > Looks ok? > > This patch makes sure that the C's enum phy_state is sync with Rust > sides. If not, compiling fails. Note that this is a temporary > solution. It will be replaced with bindgen when it supports generating > the enum conversion code. The solution that is implemented does not only work for `phy_state`, but also other enums (you still have to manually add them). Also it would be good to say that the error below is the error that one will receive when the enum is out of sync/not all C variants are in Rust. -- Cheers, Benno > > 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` > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next v6 4/5] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori ` (2 preceding siblings ...) 2023-10-24 0:58 ` [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking FUJITA Tomonori @ 2023-10-24 0:58 ` FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 5/5] net: phy: add Rust Asix PHY driver FUJITA Tomonori 4 siblings, 0 replies; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw) To: netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, benno.lossin, wedsonaf, greg Adds me as a maintainer and Trevor as a reviewer. The files are placed at rust/kernel/ directory for now but the files are likely to be moved to net/ directory once a new Rust build system is implemented. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 36815d2feb33..568b99d34533 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7818,6 +7818,14 @@ F: include/uapi/linux/mdio.h F: include/uapi/linux/mii.h F: net/core/of_net.c +ETHERNET PHY LIBRARY [RUST] +M: FUJITA Tomonori <fujita.tomonori@gmail.com> +R: Trevor Gross <tmgross@umich.edu> +L: netdev@vger.kernel.org +L: rust-for-linux@vger.kernel.org +S: Maintained +F: rust/kernel/net/phy.rs + EXEC & BINFMT API R: Eric Biederman <ebiederm@xmission.com> R: Kees Cook <keescook@chromium.org> -- 2.34.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next v6 5/5] net: phy: add Rust Asix PHY driver 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori ` (3 preceding siblings ...) 2023-10-24 0:58 ` [PATCH net-next v6 4/5] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY FUJITA Tomonori @ 2023-10-24 0:58 ` FUJITA Tomonori 4 siblings, 0 replies; 22+ messages in thread From: FUJITA Tomonori @ 2023-10-24 0:58 UTC (permalink / raw) To: netdev Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis, benno.lossin, wedsonaf, greg This is the Rust implementation of drivers/net/phy/ax88796b.c. The features are equivalent. You can choose C or Rust versionon kernel configuration. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Trevor Gross <tmgross@umich.edu> Reviewed-by: Benno Lossin <benno.lossin@proton.me> --- MAINTAINERS | 8 ++ drivers/net/phy/Kconfig | 8 ++ drivers/net/phy/Makefile | 6 +- drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++ rust/uapi/uapi_helper.h | 2 + 5 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 drivers/net/phy/ax88796b_rust.rs diff --git a/MAINTAINERS b/MAINTAINERS index 568b99d34533..410b72d3ae37 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3058,6 +3058,14 @@ S: Maintained F: Documentation/devicetree/bindings/net/asix,ax88796c.yaml F: drivers/net/ethernet/asix/ax88796c_* +ASIX PHY DRIVER [RUST] +M: FUJITA Tomonori <fujita.tomonori@gmail.com> +R: Trevor Gross <tmgross@umich.edu> +L: netdev@vger.kernel.org +L: rust-for-linux@vger.kernel.org +S: Maintained +F: drivers/net/phy/ax88796b_rust.rs + ASPEED CRYPTO DRIVER M: Neal Liu <neal_liu@aspeedtech.com> L: linux-aspeed@lists.ozlabs.org (moderated for non-subscribers) diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 0faebdb184ca..11b18370a05b 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -115,6 +115,14 @@ config AX88796B_PHY Currently supports the Asix Electronics PHY found in the X-Surf 100 AX88796B package. +config AX88796B_RUST_PHY + bool "Rust reference driver for Asix PHYs" + depends on RUST_PHYLIB_ABSTRACTIONS && AX88796B_PHY + help + Uses the Rust reference driver for Asix PHYs (ax88796b_rust.ko). + The features are equivalent. It supports the Asix Electronics PHY + found in the X-Surf 100 AX88796B package. + config BROADCOM_PHY tristate "Broadcom 54XX PHYs" select BCM_NET_PHYLIB diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index c945ed9bd14b..58d7dfb095ab 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -41,7 +41,11 @@ aquantia-objs += aquantia_hwmon.o endif obj-$(CONFIG_AQUANTIA_PHY) += aquantia.o obj-$(CONFIG_AT803X_PHY) += at803x.o -obj-$(CONFIG_AX88796B_PHY) += ax88796b.o +ifdef CONFIG_AX88796B_RUST_PHY + obj-$(CONFIG_AX88796B_PHY) += ax88796b_rust.o +else + obj-$(CONFIG_AX88796B_PHY) += ax88796b.o +endif obj-$(CONFIG_BCM54140_PHY) += bcm54140.o obj-$(CONFIG_BCM63XX_PHY) += bcm63xx.o obj-$(CONFIG_BCM7XXX_PHY) += bcm7xxx.o diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs new file mode 100644 index 000000000000..017f817f6f8d --- /dev/null +++ b/drivers/net/phy/ax88796b_rust.rs @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> + +//! Rust Asix PHYs driver +//! +//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c) +use kernel::c_str; +use kernel::net::phy::{self, DeviceId, Driver}; +use kernel::prelude::*; +use kernel::uapi; + +kernel::module_phy_driver! { + drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B], + device_table: [ + DeviceId::new_with_driver::<PhyAX88772A>(), + DeviceId::new_with_driver::<PhyAX88772C>(), + DeviceId::new_with_driver::<PhyAX88796B>() + ], + name: "rust_asix_phy", + author: "FUJITA Tomonori <fujita.tomonori@gmail.com>", + description: "Rust Asix PHYs driver", + license: "GPL", +} + +// Performs a software PHY reset using the standard +// BMCR_RESET bit and poll for the reset bit to be cleared. +// Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation +// such as used on the Individual Computers' X-Surf 100 Zorro card. +fn asix_soft_reset(dev: &mut phy::Device) -> Result { + dev.write(uapi::MII_BMCR as u16, 0)?; + dev.genphy_soft_reset() +} + +struct PhyAX88772A; + +#[vtable] +impl phy::Driver for PhyAX88772A { + const FLAGS: u32 = phy::flags::IS_INTERNAL; + const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); + const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); + + // AX88772A is not working properly with some old switches (NETGEAR EN 108TP): + // after autoneg is done and the link status is reported as active, the MII_LPA + // register is 0. This issue is not reproducible on AX88772C. + fn read_status(dev: &mut phy::Device) -> Result<u16> { + dev.genphy_update_link()?; + if !dev.get_link() { + return Ok(0); + } + // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve + // linkmode so use MII_BMCR as default values. + let ret = dev.read(uapi::MII_BMCR as u16)?; + + if ret as u32 & uapi::BMCR_SPEED100 != 0 { + dev.set_speed(uapi::SPEED_100); + } else { + dev.set_speed(uapi::SPEED_10); + } + + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 { + phy::DuplexMode::Full + } else { + phy::DuplexMode::Half + }; + dev.set_duplex(duplex); + + dev.genphy_read_lpa()?; + + if dev.is_autoneg_enabled() && dev.is_autoneg_completed() { + dev.resolve_aneg_linkmode(); + } + + Ok(0) + } + + fn suspend(dev: &mut phy::Device) -> Result { + dev.genphy_suspend() + } + + fn resume(dev: &mut phy::Device) -> Result { + dev.genphy_resume() + } + + fn soft_reset(dev: &mut phy::Device) -> Result { + asix_soft_reset(dev) + } + + fn link_change_notify(dev: &mut phy::Device) { + // Reset PHY, otherwise MII_LPA will provide outdated information. + // This issue is reproducible only with some link partner PHYs. + if dev.state() == phy::DeviceState::NoLink { + let _ = dev.init_hw(); + let _ = dev.start_aneg(); + } + } +} + +struct PhyAX88772C; + +#[vtable] +impl Driver for PhyAX88772C { + const FLAGS: u32 = phy::flags::IS_INTERNAL; + const NAME: &'static CStr = c_str!("Asix Electronics AX88772C"); + const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1881); + + fn suspend(dev: &mut phy::Device) -> Result { + dev.genphy_suspend() + } + + fn resume(dev: &mut phy::Device) -> Result { + dev.genphy_resume() + } + + fn soft_reset(dev: &mut phy::Device) -> Result { + asix_soft_reset(dev) + } +} + +struct PhyAX88796B; + +#[vtable] +impl Driver for PhyAX88796B { + const NAME: &'static CStr = c_str!("Asix Electronics AX88796B"); + const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_model_mask(0x003b1841); + + fn soft_reset(dev: &mut phy::Device) -> Result { + asix_soft_reset(dev) + } +} diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h index 301f5207f023..08f5e9334c9e 100644 --- a/rust/uapi/uapi_helper.h +++ b/rust/uapi/uapi_helper.h @@ -7,3 +7,5 @@ */ #include <uapi/asm-generic/ioctl.h> +#include <uapi/linux/mii.h> +#include <uapi/linux/ethtool.h> -- 2.34.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-10-25 23:19 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-24 0:58 [PATCH net-next v6 0/5] Rust abstractions for network PHY drivers FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 1/5] rust: core " FUJITA Tomonori 2023-10-24 16:23 ` Benno Lossin 2023-10-24 16:44 ` Andrew Lunn 2023-10-25 1:10 ` FUJITA Tomonori 2023-10-25 7:24 ` Benno Lossin 2023-10-25 10:57 ` FUJITA Tomonori 2023-10-25 14:54 ` Benno Lossin 2023-10-25 23:19 ` FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro FUJITA Tomonori 2023-10-24 16:28 ` Benno Lossin 2023-10-25 0:02 ` FUJITA Tomonori 2023-10-25 7:29 ` Benno Lossin 2023-10-25 7:57 ` FUJITA Tomonori 2023-10-25 8:08 ` Benno Lossin 2023-10-25 9:13 ` FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 3/5] rust: add second `bindgen` pass for enum exhaustiveness checking FUJITA Tomonori 2023-10-24 16:29 ` Benno Lossin 2023-10-25 1:33 ` FUJITA Tomonori 2023-10-25 7:36 ` Benno Lossin 2023-10-24 0:58 ` [PATCH net-next v6 4/5] MAINTAINERS: add Rust PHY abstractions for ETHERNET PHY LIBRARY FUJITA Tomonori 2023-10-24 0:58 ` [PATCH net-next v6 5/5] net: phy: add Rust Asix PHY driver FUJITA Tomonori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).