lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <1f61dda0-1e5e-4cdb-991b-1107439ecc99@proton.me> Date: Tue, 24 Oct 2023 16:23:20 +0000 From: Benno Lossin <benno.lossin@...ton.me> To: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org Cc: rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, greg@...ah.com Subject: Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers 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@...il.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@...il.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 > + } [...]
Powered by blists - more mailing lists