* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
@ 2023-09-24 12:56 ` Wedson Almeida Filho
2023-09-24 13:39 ` Benno Lossin
` (3 more replies)
2023-09-24 13:19 ` Benno Lossin
` (4 subsequent siblings)
5 siblings, 4 replies; 47+ messages in thread
From: Wedson Almeida Filho @ 2023-09-24 12:56 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis
On Sun, 24 Sept 2023 at 03:49, FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/Makefile | 1 +
> rust/bindings/bindings_helper.h | 3 +
> rust/kernel/lib.rs | 3 +
> rust/kernel/net.rs | 6 +
> rust/kernel/net/phy.rs | 760 ++++++++++++++++++++++++++++++++
> 5 files changed, 773 insertions(+)
> create mode 100644 rust/kernel/net.rs
> create mode 100644 rust/kernel/net/phy.rs
>
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
> cmd_bindgen = \
> $(BINDGEN) $< $(bindgen_target_flags) \
> --use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> + --rustified-enum phy_state\
> --no-debug '.*' \
> -o $@ -- $(bindgen_c_flags_final) -DMODULE \
> $(bindgen_target_cflags) $(bindgen_target_extra)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
>
> #include <kunit/test.h>
> #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> #include <linux/slab.h>
> #include <linux/refcount.h>
> #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
> #![no_std]
> #![feature(allocator_api)]
> #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
> #![feature(dispatch_from_dyn)]
> #![feature(new_uninit)]
> #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
> pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
> pub mod prelude;
> pub mod print;
> mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..b49b052969e5
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_PHYLIB)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..3b96b1e0f9ef
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,760 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> + /// PHY device and driver are not ready for anything.
> + Down,
> + /// PHY is ready to send and receive packets.
> + Ready,
> + /// PHY is up, but no polling or interrupts are done.
> + Halted,
> + /// PHY is up, but is in an error state.
> + Error,
> + /// PHY and attached device are ready to do work.
> + Up,
> + /// PHY is currently running.
> + Running,
> + /// PHY is up, but not currently plugged in.
> + NoLink,
> + /// PHY is performing a cable test.
> + CableTest,
> +}
> +
> +/// Represents duplex mode
nit: please add a period to the end of sentences. If nothing else, for
consistency with the rest of the Rust code.
> +pub enum DuplexMode {
> + /// Full-duplex mode
> + Half,
> + /// Half-duplex mode
> + Full,
> + /// Unknown
> + Unknown,
> +}
> +
> +/// Wraps the kernel's `struct phy_device`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> + /// Creates a new [`Device`] instance from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> + /// may read or write to the `phy_device` object.
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
Does this function need to be public? Can it be `pub(crate)`?
Is the requirement that nobody else may read or write to `phy_device` realistic?
> + unsafe { &mut *(ptr as *mut Self) }
> + }
> +
> + /// Gets the id of the PHY.
> + pub fn id(&mut self) -> u32 {
> + let phydev = Opaque::get(&self.0);
I don't think you need this verbose syntax, you could just do:
let phydev = self.0.get();
The same apply to all instances below.
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).phy_id }
> + }
> +
> + /// Gets the state of the PHY.
> + pub fn state(&mut self) -> DeviceState {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + let state = unsafe { (*phydev).state };
> + match state {
> + 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,
> + }
Wouldn't it make more sense to implement `TryFrom<T>` for
`DeviceState` (where `T` is the type of `phydev.state`)? This way
separate the concerns: `TryFrom` does the conversion and `state()`
fetches it from a PHY.
> + }
> +
> + /// Returns true if the PHY has no link.
> + pub fn get_link(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).link() != 0 }
> + }
> +
> + /// Returns true if auto-negotiation is enabled.
> + pub fn is_autoneg_enabled(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
> + }
> +
> + /// Returns true if auto-negotiation is completed.
> + pub fn is_autoneg_completed(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).autoneg_complete() != 0 }
> + }
> +
> + /// Sets the speed of the PHY.
> + pub fn set_speed(&mut self, speed: i32) {
Are negative speeds meaningful? If not, we should make `speed`
unsigned. (And convert appropriately when storing in phydev.speed.)
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe {
> + (*phydev).speed = speed;
> + }
> + }
> +
> + /// Sets duplex mode.
> + pub fn set_duplex(&mut self, mode: DuplexMode) {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe {
> + match mode {
> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
> + }
> + }
To avoid repeating `(*phydev).duplex =` and to reduce the amount of
code inside the unsafe block, I suggest you do something like this
here:
let v = match mode {
DuplexMode::Full => bindings::DUPLEX_FULL,
DuplexMode::Half => bindings::DUPLEX_HALF,
DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
};
// SAFETY: This object is initialized by the `from_raw` constructor,
so it's valid.
unsafe { (*phydev).duplex = v};
In fact, for this enum, you wan to use #[repr(i32)] and assign values
from `bindings` so that you can avoid the match as convert using just
`mode as i32`. See for example:
https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
> + }
> +
> + /// Reads a given C22 PHY register.
> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
> + };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret as u16)
> + }
> + }
Is there any reason why you can't use `to_result` here?
I'm thinking something along the lines of:
// SAFETY: ...
let ret = to_result(unsafe { bindings::mdiobus_read(...) })?;
Ok(ret as u16)
The same applies to similar code below.
> +
> + /// Writes a given C22 PHY register.
> + pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32, val)
> + };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Reads a paged register.
> + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_read_paged(phydev, page as i32, regnum as u32) };
> + if ret < 0 {
> + return 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 = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + unsafe {
> + bindings::phy_resolve_aneg_linkmode(phydev);
> + }
> + }
> +
> + /// Executes software reset the PHY via BMCR_RESET bit.
> + pub fn genphy_soft_reset(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_soft_reset(phydev) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Initializes the PHY.
> + pub fn init_hw(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_init_hw(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Starts auto-negotiation.
> + pub fn start_aneg(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_start_aneg(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Resumes the PHY via BMCR_PDOWN bit.
> + pub fn genphy_resume(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_resume(phydev) };
> + if ret != 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Suspends the PHY via BMCR_PDOWN bit.
> + pub fn genphy_suspend(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_suspend(phydev) };
> + if ret != 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Checks the link status and updates current link state.
> + pub fn genphy_read_status(&mut self) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_status(phydev) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret as u16)
> + }
> + }
> +
> + /// Updates the link status.
> + pub fn genphy_update_link(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_update_link(phydev) };
> + if ret < 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Reads Link partner ability.
> + pub fn genphy_read_lpa(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_lpa(phydev) };
> + if ret < 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Reads PHY abilities.
> + pub fn genphy_read_abilities(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_abilities(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +}
> +
> +// Will be replaced with the bitflags macro once it's merged in upstream.
> +const fn bit(shift: u8) -> u32 {
> + 1 << shift
> +}
> +/// PHY is internal.
> +pub const PHY_IS_INTERNAL: u32 = bit(0);
> +/// PHY needs to be reset after the refclk is enabled.
> +pub const PHY_RST_AFTER_CLK_EN: u32 = bit(1);
> +/// Polling is used to detect PHY status changes.
> +pub const PHY_POLL_CABLE_TEST: u32 = bit(2);
> +/// Don't suspend.
> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = bit(3);
To group these consts together so that it's clear they're related, we
normally create a module: see for example these file flags:
https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/file.rs#L25
/// Flags associated with a [`File`].
pub mod flags {
/// File is opened in append mode.
pub const O_APPEND: u32 = bindings::O_APPEND;
/...
}
> +/// An adapter for the registration of a PHY driver.
> +pub struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> + /// Creates a new `Adapter` instance.
> + pub const fn new() -> Adapter<T> {
> + Self { _p: PhantomData }
> + }
> +
> + unsafe extern "C" fn soft_reset_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
I'm sure the C the API guarantees that it's valid. But does it
guarantee that nobody else is reading or writing to phydev? I haven't
looked at the code but I'd be very surprised if it did.
In any case, this is part of the safety requirements of `from_raw`.
You need to address this likely unsoundness here and in the functions
below.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::soft_reset(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn get_features_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::get_features(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::suspend(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::resume(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn config_aneg_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::config_aneg(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn read_status_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::read_status(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn match_phy_device_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::match_phy_device(dev) as i32
> + }
> +
> + unsafe extern "C" fn read_mmd_callback(
> + phydev: *mut bindings::phy_device,
> + devnum: i32,
> + regnum: u16,
> + ) -> i32 {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
For devnum, I think it makes more sense to use `try_into` to convert
it to u8 and fail if it doesn't fit. Or have `T::read_mmd` take an
`i32` as argument.
> + Ok(ret.into())
> + })
> + }
> +
> + unsafe extern "C" fn write_mmd_callback(
> + phydev: *mut bindings::phy_device,
> + devnum: i32,
> + regnum: u16,
> + val: u16,
> + ) -> i32 {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::write_mmd(dev, devnum as u8, regnum, val)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::link_change_notify(dev);
> + }
> +
> + /// Creates the kernel's `phy_driver` instance.
> + ///
> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
> + Opaque::new(bindings::phy_driver {
> + name: T::NAME.as_char_ptr() as *mut i8,
> + flags: <T>::FLAGS,
I don't think you need these brackets around `T`, i.e., `T::FLAGS`
should suffice.
The same applies to all instances below.
> + phy_id: <T>::PHY_ID,
> + phy_id_mask: <T>::PHY_ID_MASK,
> + soft_reset: if <T>::HAS_SOFT_RESET {
> + Some(Self::soft_reset_callback)
> + } else {
> + None
> + },
> + get_features: if <T>::HAS_GET_FEATURES {
> + Some(Self::get_features_callback)
> + } else {
> + None
> + },
> + match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
> + Some(Self::match_phy_device_callback)
> + } else {
> + None
> + },
> + suspend: if <T>::HAS_SUSPEND {
> + Some(Self::suspend_callback)
> + } else {
> + None
> + },
> + resume: if <T>::HAS_RESUME {
> + Some(Self::resume_callback)
> + } else {
> + None
> + },
> + config_aneg: if <T>::HAS_CONFIG_ANEG {
> + Some(Self::config_aneg_callback)
> + } else {
> + None
> + },
> + read_status: if <T>::HAS_READ_STATUS {
> + Some(Self::read_status_callback)
> + } else {
> + None
> + },
> + read_mmd: if <T>::HAS_READ_MMD {
> + Some(Self::read_mmd_callback)
> + } else {
> + None
> + },
> + write_mmd: if <T>::HAS_WRITE_MMD {
> + Some(Self::write_mmd_callback)
> + } else {
> + None
> + },
> + link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
> + Some(Self::link_change_notify_callback)
> + } else {
> + None
> + },
> + // 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() }
I think you could just use bindings::phy_driver::default() here.
> + })
> + }
> +}
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +#[vtable]
We use this only when the C side offers some default non-trivial
behaviour for some of the callbacks (in which case it doesn't make
sense to replicate in Rust). But if the default behaviour is similar
to our default implementations below (e.g., return `code::ENOTSUPP`),
then we can simply drop this tag and simplify the code above by
dropping the `HAS_XXX` checks.
Do you actually need vtable in PHY?
> +pub trait Driver {
> + /// Corresponds to `flags` in `struct phy_driver`.
Please document this properly. It's to ok to explain what it refers
to, but we should also say what it does and what the accepted values
are. (For example, but putting the valid flags in a module -- as I
suggest above -- and say here that valid values are combinations of
the value in module `X`).
Part of the reason we're introducing Rust is to make it simpler (and
safer) to write kernel code. This includes better documentation and
not having to go read the C code to understand how things work.
In other words, if someone is writing a PHY driver in Rust, ideally
they should be able to just read the Rust documentation and know what
they need to do.
I know it's painful to do this (I've done it several times), but
that's part of the work we need to put in while building these
abstractions.
> + const FLAGS: u32 = 0;
> + /// Corresponds to `name` in `struct phy_driver`.
> + const NAME: &'static CStr;
> + /// Corresponds to `phy_id` in `struct phy_driver`.
> + const PHY_ID: u32 = 0;
> + /// Corresponds to `phy_id_mask` in `struct phy_driver`.
> + const PHY_ID_MASK: u32 = 0;
> +
> + /// Corresponds to `soft_reset` in `struct phy_driver`.
> + fn soft_reset(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `get_features` in `struct phy_driver`.
> + fn get_features(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `match_phy_device` in `struct phy_driver`.
> + fn match_phy_device(_dev: &mut Device) -> bool {
> + false
> + }
> +
> + /// Corresponds to `config_aneg` in `struct phy_driver`.
> + fn config_aneg(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `read_status` in `struct phy_driver`.
> + fn read_status(_dev: &mut Device) -> Result<u16> {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `suspend` in `struct phy_driver`.
> + fn suspend(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `resume` in `struct phy_driver`.
> + fn resume(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `read_mmd` in `struct phy_driver`.
> + fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `write_mmd` in `struct phy_driver`.
> + fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `link_change_notify` in `struct phy_driver`.
> + fn link_change_notify(_dev: &mut Device) {}
> +}
> +
> +/// Registration structure for a PHY driver.
> +pub struct Registration {
> + module: &'static crate::ThisModule,
> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> +}
> +
> +impl Registration {
> + /// Creates a new `Registration` instance.
> + pub fn new(module: &'static crate::ThisModule) -> Self {
> + Registration {
> + module,
> + drivers: None,
> + }
> + }
> +
> + /// Registers a PHY driver.
> + pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
> + if drivers.len() > 0 {
> + // SAFETY: Just an FFI call.
This is not a good-enough justification.
There's a bunch of pointers passed in. You need to justify why they're
good now and why they'll remain good. Does `phy_drivers_register`
require that pointers remain alive just during the call or until
they're unregistered? If the latter (which I assume to be the case),
you need to justify how it is that they remain valid. If they need to
be pinned, how are you guaranteeing that?
> + let ret = unsafe {
> + bindings::phy_drivers_register(
> + drivers[0].get(),
> + drivers.len() as i32,
> + self.module.0,
> + )
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> + self.drivers = Some(drivers);
> + Ok(())
> + } else {
> + Err(code::EINVAL)
> + }
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + if let Some(drv) = self.drivers.take() {
> + // SAFETY: Just an FFI call.
Again, this isn't really a good justification.
At the very least, these must have been previously registered (and not
unregistered since then).
> + unsafe {
> + bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32);
> + }
> + }
> + }
> +}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}
> +
> +const DEVICE_MASK_EXACT: u32 = !0;
> +const DEVICE_MASK_MODEL: u32 = !0 << 4;
> +const DEVICE_MASK_VENDOR: u32 = !0 << 10;
> +
> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> + /// Corresponds to `phy_id` in `struct mdio_device_id`.
> + pub id: u32,
> + mask: DeviceMask,
> +}
> +
> +impl DeviceId {
> + /// Creates a new instance with the exact match mask.
> + pub const fn new_with_exact_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Exact,
> + }
> + }
> +
> + /// Creates a new instance with the model match mask.
> + pub const fn new_with_model_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Model,
> + }
> + }
> +
> + /// Creates a new instance with the vendor match mask.
> + pub const fn new_with_vendor_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Vendor,
> + }
> + }
> +
> + /// Creates a new instance with a custom match mask.
> + pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Custom(mask),
> + }
> + }
> +
> + /// Creates a new instance from [`Driver`].
> + pub const fn new_with_driver<T: Driver>() -> Self {
> + DeviceId {
> + id: T::PHY_ID,
> + mask: DeviceMask::new(T::PHY_ID_MASK),
> + }
> + }
> +
> + /// Get a mask as u32.
> + pub const fn mask_as_int(self) -> u32 {
> + match self.mask {
> + DeviceMask::Exact => DEVICE_MASK_EXACT,
> + DeviceMask::Model => DEVICE_MASK_MODEL,
> + DeviceMask::Vendor => DEVICE_MASK_VENDOR,
> + DeviceMask::Custom(mask) => mask,
> + }
> + }
> +}
> +
> +enum DeviceMask {
> + Exact,
> + Model,
> + Vendor,
> + Custom(u32),
> +}
> +
> +impl DeviceMask {
> + const fn new(mask: u32) -> Self {
> + match mask {
> + DEVICE_MASK_EXACT => DeviceMask::Exact,
> + DEVICE_MASK_MODEL => DeviceMask::Model,
> + DEVICE_MASK_VENDOR => DeviceMask::Vendor,
> + _ => DeviceMask::Custom(mask),
> + }
> + }
> +}
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver] and registers it.
Typo: missing backtick.
> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +/// device_table: [
> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
> +/// DeviceId::new_with_driver::<PhyAX88796B>()
> +/// ],
> +/// type: RustAsixPhy,
> +/// name: "rust_asix_phy",
> +/// author: "Rust for Linux Contributors",
> +/// description: "Rust Asix PHYs driver",
> +/// license: "GPL",
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! module_phy_driver {
> + (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> + (@count_devices $($x:expr),*) => {
> + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> + };
> +
> + (@device_table $name:ident, [$($dev:expr),+]) => {
> + ::kernel::macros::paste! {
> + #[no_mangle]
> + static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
> + [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
> + }
> + };
> +
> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
> + struct Module<$modname> {
> + _reg: kernel::net::phy::Registration,
> + _p: core::marker::PhantomData<$modname>,
> + }
> +
> + type ModuleType = Module<$modname>;
> +
> + $crate::prelude::module! {
> + type: ModuleType,
> + $($f)*
> + }
> +
> + static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
> + $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
> + ];
> +
> + impl kernel::Module for Module<$modname> {
> + fn init(module: &'static ThisModule) -> Result<Self> {
> + let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
> + // SAFETY: static `DRIVERS` array is used only in the C side.
> + unsafe {
> + reg.register(&DRIVERS)?;
> + }
> +
> + Ok(Module {
> + _reg: reg,
> + _p: core::marker::PhantomData,
> + })
> + }
> + }
> +
> + $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
> + }
> +}
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 12:56 ` Wedson Almeida Filho
@ 2023-09-24 13:39 ` Benno Lossin
2023-09-24 13:51 ` Wedson Almeida Filho
2023-09-25 11:30 ` FUJITA Tomonori
2023-09-24 15:42 ` Andrew Lunn
` (2 subsequent siblings)
3 siblings, 2 replies; 47+ messages in thread
From: Benno Lossin @ 2023-09-24 13:39 UTC (permalink / raw)
To: Wedson Almeida Filho, FUJITA Tomonori
Cc: rust-for-linux, andrew, tmgross, miguel.ojeda.sandonis
On 24.09.23 14:56, Wedson Almeida Filho wrote:
> On Sun, 24 Sept 2023 at 03:49, FUJITA Tomonori
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe {
>> + (*phydev).speed = speed;
>> + }
>> + }
>> +
>> + /// Sets duplex mode.
>> + pub fn set_duplex(&mut self, mode: DuplexMode) {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe {
>> + match mode {
>> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
>> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
>> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
>> + }
>> + }
>
> To avoid repeating `(*phydev).duplex =` and to reduce the amount of
> code inside the unsafe block, I suggest you do something like this
> here:
>
> let v = match mode {
> DuplexMode::Full => bindings::DUPLEX_FULL,
> DuplexMode::Half => bindings::DUPLEX_HALF,
> DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> };
>
> // SAFETY: This object is initialized by the `from_raw` constructor,
> so it's valid.
> unsafe { (*phydev).duplex = v};
>
>
> In fact, for this enum, you wan to use #[repr(i32)] and assign values
> from `bindings` so that you can avoid the match as convert using just
> `mode as i32`. See for example:
>
> https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
>
I do not like `as`-style casts, they are error prone and no warning/error
is generated if they are used incorrectly. They also make review harder,
because I now have to look if the enum indeed has that correct repr.
I think we should talk about this in one of our meetings.
>> + phy_id: <T>::PHY_ID,
>> + phy_id_mask: <T>::PHY_ID_MASK,
>> + soft_reset: if <T>::HAS_SOFT_RESET {
>> + Some(Self::soft_reset_callback)
>> + } else {
>> + None
>> + },
>> + get_features: if <T>::HAS_GET_FEATURES {
>> + Some(Self::get_features_callback)
>> + } else {
>> + None
>> + },
>> + match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
>> + Some(Self::match_phy_device_callback)
>> + } else {
>> + None
>> + },
>> + suspend: if <T>::HAS_SUSPEND {
>> + Some(Self::suspend_callback)
>> + } else {
>> + None
>> + },
>> + resume: if <T>::HAS_RESUME {
>> + Some(Self::resume_callback)
>> + } else {
>> + None
>> + },
>> + config_aneg: if <T>::HAS_CONFIG_ANEG {
>> + Some(Self::config_aneg_callback)
>> + } else {
>> + None
>> + },
>> + read_status: if <T>::HAS_READ_STATUS {
>> + Some(Self::read_status_callback)
>> + } else {
>> + None
>> + },
>> + read_mmd: if <T>::HAS_READ_MMD {
>> + Some(Self::read_mmd_callback)
>> + } else {
>> + None
>> + },
>> + write_mmd: if <T>::HAS_WRITE_MMD {
>> + Some(Self::write_mmd_callback)
>> + } else {
>> + None
>> + },
>> + link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
>> + Some(Self::link_change_notify_callback)
>> + } else {
>> + None
>> + },
>> + // 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() }
>
> I think you could just use bindings::phy_driver::default() here.
That is not possible, since it is not a const function.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 13:39 ` Benno Lossin
@ 2023-09-24 13:51 ` Wedson Almeida Filho
2023-09-25 11:30 ` FUJITA Tomonori
1 sibling, 0 replies; 47+ messages in thread
From: Wedson Almeida Filho @ 2023-09-24 13:51 UTC (permalink / raw)
To: Benno Lossin
Cc: FUJITA Tomonori, rust-for-linux, andrew, tmgross,
miguel.ojeda.sandonis
On Sun, 24 Sept 2023 at 10:40, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 24.09.23 14:56, Wedson Almeida Filho wrote:
> > On Sun, 24 Sept 2023 at 03:49, FUJITA Tomonori
> >> + let phydev = Opaque::get(&self.0);
> >> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> + unsafe {
> >> + (*phydev).speed = speed;
> >> + }
> >> + }
> >> +
> >> + /// Sets duplex mode.
> >> + pub fn set_duplex(&mut self, mode: DuplexMode) {
> >> + let phydev = Opaque::get(&self.0);
> >> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> + unsafe {
> >> + match mode {
> >> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
> >> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
> >> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
> >> + }
> >> + }
> >
> > To avoid repeating `(*phydev).duplex =` and to reduce the amount of
> > code inside the unsafe block, I suggest you do something like this
> > here:
> >
> > let v = match mode {
> > DuplexMode::Full => bindings::DUPLEX_FULL,
> > DuplexMode::Half => bindings::DUPLEX_HALF,
> > DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> > };
> >
> > // SAFETY: This object is initialized by the `from_raw` constructor,
> > so it's valid.
> > unsafe { (*phydev).duplex = v};
> >
> >
> > In fact, for this enum, you wan to use #[repr(i32)] and assign values
> > from `bindings` so that you can avoid the match as convert using just
> > `mode as i32`. See for example:
> >
> > https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
> >
>
> I do not like `as`-style casts, they are error prone and no warning/error
> is generated if they are used incorrectly. They also make review harder,
> because I now have to look if the enum indeed has that correct repr.
> I think we should talk about this in one of our meetings.
Yes, let's discuss this with the rest of the team in a meeting.
I think the main advantage is that we avoid the big match statement
and additional code that it entails. Perhaps we could have a `TryFrom`
implementation that uses `as` and that's what callers use. Then we
only have to review/verify the `as` usage once when the enum is
defined.
Another option is to keep the big match statement if we can show that
the compiler can optimise it out.
>
> >> + phy_id: <T>::PHY_ID,
> >> + phy_id_mask: <T>::PHY_ID_MASK,
> >> + soft_reset: if <T>::HAS_SOFT_RESET {
> >> + Some(Self::soft_reset_callback)
> >> + } else {
> >> + None
> >> + },
> >> + get_features: if <T>::HAS_GET_FEATURES {
> >> + Some(Self::get_features_callback)
> >> + } else {
> >> + None
> >> + },
> >> + match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
> >> + Some(Self::match_phy_device_callback)
> >> + } else {
> >> + None
> >> + },
> >> + suspend: if <T>::HAS_SUSPEND {
> >> + Some(Self::suspend_callback)
> >> + } else {
> >> + None
> >> + },
> >> + resume: if <T>::HAS_RESUME {
> >> + Some(Self::resume_callback)
> >> + } else {
> >> + None
> >> + },
> >> + config_aneg: if <T>::HAS_CONFIG_ANEG {
> >> + Some(Self::config_aneg_callback)
> >> + } else {
> >> + None
> >> + },
> >> + read_status: if <T>::HAS_READ_STATUS {
> >> + Some(Self::read_status_callback)
> >> + } else {
> >> + None
> >> + },
> >> + read_mmd: if <T>::HAS_READ_MMD {
> >> + Some(Self::read_mmd_callback)
> >> + } else {
> >> + None
> >> + },
> >> + write_mmd: if <T>::HAS_WRITE_MMD {
> >> + Some(Self::write_mmd_callback)
> >> + } else {
> >> + None
> >> + },
> >> + link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
> >> + Some(Self::link_change_notify_callback)
> >> + } else {
> >> + None
> >> + },
> >> + // 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() }
> >
> > I think you could just use bindings::phy_driver::default() here.
>
> That is not possible, since it is not a const function.
Ah, yeah, this is in const context. Nevermind.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 13:39 ` Benno Lossin
2023-09-24 13:51 ` Wedson Almeida Filho
@ 2023-09-25 11:30 ` FUJITA Tomonori
2023-09-25 13:14 ` Andrew Lunn
1 sibling, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-25 11:30 UTC (permalink / raw)
To: benno.lossin
Cc: wedsonaf, fujita.tomonori, rust-for-linux, andrew, tmgross,
miguel.ojeda.sandonis
Hi,
On Sun, 24 Sep 2023 13:39:55 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
> On 24.09.23 14:56, Wedson Almeida Filho wrote:
>> On Sun, 24 Sept 2023 at 03:49, FUJITA Tomonori
>>> + let phydev = Opaque::get(&self.0);
>>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>>> + unsafe {
>>> + (*phydev).speed = speed;
>>> + }
>>> + }
>>> +
>>> + /// Sets duplex mode.
>>> + pub fn set_duplex(&mut self, mode: DuplexMode) {
>>> + let phydev = Opaque::get(&self.0);
>>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>>> + unsafe {
>>> + match mode {
>>> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
>>> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
>>> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
>>> + }
>>> + }
>>
>> To avoid repeating `(*phydev).duplex =` and to reduce the amount of
>> code inside the unsafe block, I suggest you do something like this
>> here:
>>
>> let v = match mode {
>> DuplexMode::Full => bindings::DUPLEX_FULL,
>> DuplexMode::Half => bindings::DUPLEX_HALF,
>> DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
>> };
>>
>> // SAFETY: This object is initialized by the `from_raw` constructor,
>> so it's valid.
>> unsafe { (*phydev).duplex = v};
>>
>>
>> In fact, for this enum, you wan to use #[repr(i32)] and assign values
>> from `bindings` so that you can avoid the match as convert using just
>> `mode as i32`. See for example:
>>
>> https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
>>
>
> I do not like `as`-style casts, they are error prone and no warning/error
> is generated if they are used incorrectly. They also make review harder,
> because I now have to look if the enum indeed has that correct repr.
> I think we should talk about this in one of our meetings.
Can we try to continue to discuss on the mailing list? I think that
this topic (enum conversion) is also important for developers who work
on mainly C now but are interested in Rust support.
About the above duplex-mode code, I'm not sure if there is the better way.
It's conversion from enum like const values on C side.
#define DUPLEX_HALF 0x00
#define DUPLEX_FULL 0x01
#define DUPLEX_UNKNOWN 0xff
We can't automatically generate rustified enum from them so we have to
convert them by hand, match or repr.
We don't need to think about divergence. Also we convert from Rust
enum to C value so no conversion error.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 11:30 ` FUJITA Tomonori
@ 2023-09-25 13:14 ` Andrew Lunn
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 13:14 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: benno.lossin, wedsonaf, rust-for-linux, tmgross,
miguel.ojeda.sandonis
> It's conversion from enum like const values on C side.
>
> #define DUPLEX_HALF 0x00
> #define DUPLEX_FULL 0x01
> #define DUPLEX_UNKNOWN 0xff
>
> We can't automatically generate rustified enum from them so we have to
> convert them by hand, match or repr.
If it helps, you could submit a patch changing them to an enum. I've
no problem changing C code to make Rust/C maintenance issues simpler.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 12:56 ` Wedson Almeida Filho
2023-09-24 13:39 ` Benno Lossin
@ 2023-09-24 15:42 ` Andrew Lunn
2023-09-25 1:13 ` FUJITA Tomonori
2023-09-25 6:47 ` Alice Ryhl
2023-09-26 1:19 ` FUJITA Tomonori
3 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-09-24 15:42 UTC (permalink / raw)
To: Wedson Almeida Filho
Cc: FUJITA Tomonori, rust-for-linux, tmgross, miguel.ojeda.sandonis
> > + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > + /// may read or write to the `phy_device` object.
> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>
> Is the requirement that nobody else may read or write to `phy_device` realistic?
In what scope?
The phylib core will use phy_device members. The MAC driver has a
callback which should use phy_device members. However, the phylib core
also has a mutex to protect phy_device members. Nearly all calls into
this driver and all calls into the MAC driver are made with this mutex
held. And phylib core also uses the mutex.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 15:42 ` Andrew Lunn
@ 2023-09-25 1:13 ` FUJITA Tomonori
2023-09-25 13:25 ` Andrew Lunn
0 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-25 1:13 UTC (permalink / raw)
To: andrew
Cc: wedsonaf, fujita.tomonori, rust-for-linux, tmgross,
miguel.ojeda.sandonis
On Sun, 24 Sep 2023 17:42:16 +0200
Andrew Lunn <andrew@lunn.ch> wrote:
>> > + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> > + /// may read or write to the `phy_device` object.
>> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>
>> Is the requirement that nobody else may read or write to `phy_device` realistic?
>
> In what scope?
Inside a function in phy_driver like suspend().
> The phylib core will use phy_device members. The MAC driver has a
> callback which should use phy_device members. However, the phylib core
> also has a mutex to protect phy_device members. Nearly all calls into
> this driver and all calls into the MAC driver are made with this mutex
> held. And phylib core also uses the mutex.
Thanks for clarification.
I thought that inside calls in phy_driver, exclusive access to
phy_device is guaranteed. Please let me know if any exceptions
exist. I need to think about locking when adding such to the Rust
bingings.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 1:13 ` FUJITA Tomonori
@ 2023-09-25 13:25 ` Andrew Lunn
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 13:25 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: wedsonaf, rust-for-linux, tmgross, miguel.ojeda.sandonis
On Mon, Sep 25, 2023 at 10:13:16AM +0900, FUJITA Tomonori wrote:
> On Sun, 24 Sep 2023 17:42:16 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
>
> >> > + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >> > + /// may read or write to the `phy_device` object.
> >> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >>
> >> Is the requirement that nobody else may read or write to `phy_device` realistic?
> >
> > In what scope?
>
> Inside a function in phy_driver like suspend().
>
> > The phylib core will use phy_device members. The MAC driver has a
> > callback which should use phy_device members. However, the phylib core
> > also has a mutex to protect phy_device members. Nearly all calls into
> > this driver and all calls into the MAC driver are made with this mutex
> > held. And phylib core also uses the mutex.
>
> Thanks for clarification.
>
> I thought that inside calls in phy_driver, exclusive access to
> phy_device is guaranteed. Please let me know if any exceptions
> exist. I need to think about locking when adding such to the Rust
> bingings.
You actually picked the one method which is not locked. suspend and
resume don't hold the phydev lock. I don't remember the details. There
is either a lock inversion, or a true deadlock. There was a discussion
about it a few years ago, Russell King found the issue, maybe with
mvneta.
There are also PHY drivers which do more then PHY. e.g. the marvell.c
driver has a hwmon driver embedded within it. That code is not covered
by the mutex because the entry points are not via phylib. So it needs
to consider locking itself for those parts of the driver. This locking
is always going to be custom to the driver.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 12:56 ` Wedson Almeida Filho
2023-09-24 13:39 ` Benno Lossin
2023-09-24 15:42 ` Andrew Lunn
@ 2023-09-25 6:47 ` Alice Ryhl
2023-09-26 1:19 ` FUJITA Tomonori
3 siblings, 0 replies; 47+ messages in thread
From: Alice Ryhl @ 2023-09-25 6:47 UTC (permalink / raw)
To: wedsonaf
Cc: andrew, fujita.tomonori, miguel.ojeda.sandonis, rust-for-linux,
tmgross
Wedson Almeida Filho <wedsonaf@gmail.com> writes:
>> +/// Wraps the kernel's `struct phy_device`.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> + /// may read or write to the `phy_device` object.
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>
> Does this function need to be public? Can it be `pub(crate)`?
I would generally prefer that we keep these methods as "pub". Otherwise,
drivers may need to make a bunch of changes of the form `pub(crate)` to
`pub`, which I think causes unnecessary noise.
Alice
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 12:56 ` Wedson Almeida Filho
` (2 preceding siblings ...)
2023-09-25 6:47 ` Alice Ryhl
@ 2023-09-26 1:19 ` FUJITA Tomonori
2023-09-26 2:50 ` Andrew Lunn
3 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-26 1:19 UTC (permalink / raw)
To: wedsonaf
Cc: fujita.tomonori, rust-for-linux, andrew, tmgross,
miguel.ojeda.sandonis
Hi, thanks for reviewing!
On Sun, 24 Sep 2023 09:56:29 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>> +/// Corresponds to the kernel's `enum phy_state`.
>> +#[derive(PartialEq)]
>> +pub enum DeviceState {
>> + /// PHY device and driver are not ready for anything.
>> + Down,
>> + /// PHY is ready to send and receive packets.
>> + Ready,
>> + /// PHY is up, but no polling or interrupts are done.
>> + Halted,
>> + /// PHY is up, but is in an error state.
>> + Error,
>> + /// PHY and attached device are ready to do work.
>> + Up,
>> + /// PHY is currently running.
>> + Running,
>> + /// PHY is up, but not currently plugged in.
>> + NoLink,
>> + /// PHY is performing a cable test.
>> + CableTest,
>> +}
>> +
>> +/// Represents duplex mode
>
> nit: please add a period to the end of sentences. If nothing else, for
> consistency with the rest of the Rust code.
Oops, fixed.
>> +pub enum DuplexMode {
>> + /// Full-duplex mode
>> + Half,
>> + /// Half-duplex mode
>> + Full,
>> + /// Unknown
>> + Unknown,
>> +}
>> +
>> +/// Wraps the kernel's `struct phy_device`.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> + /// may read or write to the `phy_device` object.
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>
> Does this function need to be public? Can it be `pub(crate)`?
Yeah, pub could be dropped. Should I?
> Is the requirement that nobody else may read or write to `phy_device` realistic?
Yes. The Device objects are used only inside the callbacks in
phy_driver. PHY drivers assume exclusive access.
As Andrew pointed out, resume/suspend dosn't hold phy_device's mutex
but still looks like PHY drivers assume exclusive access. After
googling, I leant that they have been trying to fix this locking
issue. I'll update the code accordingly.
>> + unsafe { &mut *(ptr as *mut Self) }
>> + }
>> +
>> + /// Gets the id of the PHY.
>> + pub fn id(&mut self) -> u32 {
>> + let phydev = Opaque::get(&self.0);
>
> I don't think you need this verbose syntax, you could just do:
>
> let phydev = self.0.get();
>
> The same apply to all instances below.
Nice, fixed.
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe { (*phydev).phy_id }
>> + }
>> +
>> + /// Gets the state of the PHY.
>> + pub fn state(&mut self) -> DeviceState {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + let state = unsafe { (*phydev).state };
>> + match state {
>> + 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,
>> + }
>
> Wouldn't it make more sense to implement `TryFrom<T>` for
> `DeviceState` (where `T` is the type of `phydev.state`)? This way
> separate the concerns: `TryFrom` does the conversion and `state()`
> fetches it from a PHY.
Not sure. This is one of discussed issues on v1; what can/should we
return when TryFrom fails? If it fails, the phylib state machine is
already broken.
>> + /// Sets the speed of the PHY.
>> + pub fn set_speed(&mut self, speed: i32) {
>
> Are negative speeds meaningful? If not, we should make `speed`
> unsigned. (And convert appropriately when storing in phydev.speed.)
Fixed.
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe {
>> + (*phydev).speed = speed;
>> + }
>> + }
>> +
>> + /// Sets duplex mode.
>> + pub fn set_duplex(&mut self, mode: DuplexMode) {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe {
>> + match mode {
>> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
>> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
>> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
>> + }
>> + }
>
> To avoid repeating `(*phydev).duplex =` and to reduce the amount of
> code inside the unsafe block, I suggest you do something like this
> here:
>
> let v = match mode {
> DuplexMode::Full => bindings::DUPLEX_FULL,
> DuplexMode::Half => bindings::DUPLEX_HALF,
> DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> };
>
> // SAFETY: This object is initialized by the `from_raw` constructor,
> so it's valid.
> unsafe { (*phydev).duplex = v};
Done.
> In fact, for this enum, you wan to use #[repr(i32)] and assign values
> from `bindings` so that you can avoid the match as convert using just
> `mode as i32`. See for example:
>
> https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
I slightly prefer match but either works for me. As I wrote in another
mail, I'm not sure if we can handle nicely.
>> + }
>> +
>> + /// Reads a given C22 PHY register.
>> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe {
>> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
>> + };
>> + if ret < 0 {
>> + Err(Error::from_errno(ret))
>> + } else {
>> + Ok(ret as u16)
>> + }
>> + }
>
> Is there any reason why you can't use `to_result` here?
>
> I'm thinking something along the lines of:
>
> // SAFETY: ...
> let ret = to_result(unsafe { bindings::mdiobus_read(...) })?;
> Ok(ret as u16)
Can we? to_result() doens't return a value on success.
pub fn to_result(err: core::ffi::c_int) -> Result {
if err < 0 {
Err(Error::from_errno(err))
} else {
Ok(())
}
}
something like to_result_with_value() might be useful, I guess.
> The same applies to similar code below.
Fixed where to_result() can be used.
>> +/// PHY is internal.
>> +pub const PHY_IS_INTERNAL: u32 = bit(0);
>> +/// PHY needs to be reset after the refclk is enabled.
>> +pub const PHY_RST_AFTER_CLK_EN: u32 = bit(1);
>> +/// Polling is used to detect PHY status changes.
>> +pub const PHY_POLL_CABLE_TEST: u32 = bit(2);
>> +/// Don't suspend.
>> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = bit(3);
>
> To group these consts together so that it's clear they're related, we
> normally create a module: see for example these file flags:
> https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/file.rs#L25
>
> /// Flags associated with a [`File`].
> pub mod flags {
> /// File is opened in append mode.
> pub const O_APPEND: u32 = bindings::O_APPEND;
>
> /...
> }
I see, fixed.
>> +/// An adapter for the registration of a PHY driver.
>> +pub struct Adapter<T: Driver> {
>> + _p: PhantomData<T>,
>> +}
>> +
>> +impl<T: Driver> Adapter<T> {
>> + /// Creates a new `Adapter` instance.
>> + pub const fn new() -> Adapter<T> {
>> + Self { _p: PhantomData }
>> + }
>> +
>> + unsafe extern "C" fn soft_reset_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>
> I'm sure the C the API guarantees that it's valid. But does it
> guarantee that nobody else is reading or writing to phydev? I haven't
> looked at the code but I'd be very surprised if it did.
>
> In any case, this is part of the safety requirements of `from_raw`.
> You need to address this likely unsoundness here and in the functions
> below.
As explained above, seems that that's how PHY drivers are implemented.
>> + unsafe extern "C" fn read_mmd_callback(
>> + phydev: *mut bindings::phy_device,
>> + devnum: i32,
>> + regnum: u16,
>> + ) -> i32 {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
>
> For devnum, I think it makes more sense to use `try_into` to convert
> it to u8 and fail if it doesn't fit. Or have `T::read_mmd` take an
> `i32` as argument.
phylib makes sure that devnum is in a range before this funcation is
called. it needs to be 0..31, not all u8 . We still need to use
try_into() here?
>> + /// Creates the kernel's `phy_driver` instance.
>> + ///
>> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
>> + Opaque::new(bindings::phy_driver {
>> + name: T::NAME.as_char_ptr() as *mut i8,
>> + flags: <T>::FLAGS,
>
> I don't think you need these brackets around `T`, i.e., `T::FLAGS`
> should suffice.
>
> The same applies to all instances below.
Ah, fixed.
>> +/// Corresponds to functions in `struct phy_driver`.
>> +#[vtable]
>
> We use this only when the C side offers some default non-trivial
> behaviour for some of the callbacks (in which case it doesn't make
> sense to replicate in Rust). But if the default behaviour is similar
> to our default implementations below (e.g., return `code::ENOTSUPP`),
> then we can simply drop this tag and simplify the code above by
> dropping the `HAS_XXX` checks.
>
> Do you actually need vtable in PHY?
I might misunderstand vtable.
What PHY bindings want is: there are many callbacks in PHY drivers but
most of PHY drivers implement only some. The callbacks are not
implemented in a driver must be NULL.
>> +pub trait Driver {
>> + /// Corresponds to `flags` in `struct phy_driver`.
>
> Please document this properly. It's to ok to explain what it refers
> to, but we should also say what it does and what the accepted values
> are. (For example, but putting the valid flags in a module -- as I
> suggest above -- and say here that valid values are combinations of
> the value in module `X`).
>
> Part of the reason we're introducing Rust is to make it simpler (and
> safer) to write kernel code. This includes better documentation and
> not having to go read the C code to understand how things work.
>
> In other words, if someone is writing a PHY driver in Rust, ideally
> they should be able to just read the Rust documentation and know what
> they need to do.
>
> I know it's painful to do this (I've done it several times), but
> that's part of the work we need to put in while building these
> abstractions.
Sure, I could copy-and-paste comments in phylib.
>> +/// Registration structure for a PHY driver.
>> +pub struct Registration {
>> + module: &'static crate::ThisModule,
>> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
>> +}
>> +
>> +impl Registration {
>> + /// Creates a new `Registration` instance.
>> + pub fn new(module: &'static crate::ThisModule) -> Self {
>> + Registration {
>> + module,
>> + drivers: None,
>> + }
>> + }
>> +
>> + /// Registers a PHY driver.
>> + pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
>> + if drivers.len() > 0 {
>> + // SAFETY: Just an FFI call.
>
> This is not a good-enough justification.
>
> There's a bunch of pointers passed in. You need to justify why they're
> good now and why they'll remain good. Does `phy_drivers_register`
> require that pointers remain alive just during the call or until
> they're unregistered? If the latter (which I assume to be the case),
> you need to justify how it is that they remain valid. If they need to
> be pinned, how are you guaranteeing that?
Requiring a static array doesn't guarantee that?
>> + let ret = unsafe {
>> + bindings::phy_drivers_register(
>> + drivers[0].get(),
>> + drivers.len() as i32,
>> + self.module.0,
>> + )
>> + };
>> + if ret != 0 {
>> + return Err(Error::from_errno(ret));
>> + }
>> + self.drivers = Some(drivers);
>> + Ok(())
>> + } else {
>> + Err(code::EINVAL)
>> + }
>> + }
>> +}
>> +
>> +impl Drop for Registration {
>> + fn drop(&mut self) {
>> + if let Some(drv) = self.drivers.take() {
>> + // SAFETY: Just an FFI call.
>
> Again, this isn't really a good justification.
>
> At the very least, these must have been previously registered (and not
> unregistered since then).
Will try to improve.
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver] and registers it.
>
> Typo: missing backtick.
Oops, fixed.
Thanks,
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-26 1:19 ` FUJITA Tomonori
@ 2023-09-26 2:50 ` Andrew Lunn
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-26 2:50 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: wedsonaf, rust-for-linux, tmgross, miguel.ojeda.sandonis
> > I'm sure the C the API guarantees that it's valid. But does it
> > guarantee that nobody else is reading or writing to phydev? I haven't
> > looked at the code but I'd be very surprised if it did.
> >
> > In any case, this is part of the safety requirements of `from_raw`.
> > You need to address this likely unsoundness here and in the functions
> > below.
>
> As explained above, seems that that's how PHY drivers are implemented.
If you really want to be paranoid, you could check:
mutex_is_locked(&phydev->lock)
is true. If it is not true, dump a stack trace and keep going.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
2023-09-24 12:56 ` Wedson Almeida Filho
@ 2023-09-24 13:19 ` Benno Lossin
2023-09-25 10:24 ` FUJITA Tomonori
2023-09-24 17:00 ` Andrew Lunn
` (3 subsequent siblings)
5 siblings, 1 reply; 47+ messages in thread
From: Benno Lossin @ 2023-09-24 13:19 UTC (permalink / raw)
To: FUJITA Tomonori, rust-for-linux; +Cc: andrew, tmgross, miguel.ojeda.sandonis
On 24.09.23 08:49, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/Makefile | 1 +
> rust/bindings/bindings_helper.h | 3 +
> rust/kernel/lib.rs | 3 +
> rust/kernel/net.rs | 6 +
> rust/kernel/net/phy.rs | 760 ++++++++++++++++++++++++++++++++
> 5 files changed, 773 insertions(+)
> create mode 100644 rust/kernel/net.rs
> create mode 100644 rust/kernel/net/phy.rs
>
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
> cmd_bindgen = \
> $(BINDGEN) $< $(bindgen_target_flags) \
> --use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> + --rustified-enum phy_state\
> --no-debug '.*' \
> -o $@ -- $(bindgen_c_flags_final) -DMODULE \
> $(bindgen_target_cflags) $(bindgen_target_extra)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
>
> #include <kunit/test.h>
> #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> #include <linux/slab.h>
> #include <linux/refcount.h>
> #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
> #![no_std]
> #![feature(allocator_api)]
> #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
> #![feature(dispatch_from_dyn)]
> #![feature(new_uninit)]
> #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
> pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
> pub mod prelude;
> pub mod print;
> mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..b49b052969e5
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_PHYLIB)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..3b96b1e0f9ef
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,760 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> + /// PHY device and driver are not ready for anything.
> + Down,
> + /// PHY is ready to send and receive packets.
> + Ready,
> + /// PHY is up, but no polling or interrupts are done.
> + Halted,
> + /// PHY is up, but is in an error state.
> + Error,
> + /// PHY and attached device are ready to do work.
> + Up,
> + /// PHY is currently running.
> + Running,
> + /// PHY is up, but not currently plugged in.
> + NoLink,
> + /// PHY is performing a cable test.
> + CableTest,
> +}
> +
> +/// Represents duplex mode
> +pub enum DuplexMode {
> + /// Full-duplex mode
> + Half,
> + /// Half-duplex mode
> + Full,
> + /// Unknown
> + Unknown,
> +}
> +
> +/// Wraps the kernel's `struct phy_device`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
This struct should have a type invariant saying that "`self.0` is
always in a valid state.".
> +
> +impl Device {
> + /// Creates a new [`Device`] instance from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> + /// may read or write to the `phy_device` object.
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> + unsafe { &mut *(ptr as *mut Self) }
> + }
> +
> + /// Gets the id of the PHY.
> + pub fn id(&mut self) -> u32 {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
These kinds of safety comments should be replaced by "`phydev` is
pointing to a valid object by the type invariant of `Self`."
Also for the other safety comments below.
> + unsafe { (*phydev).phy_id }
> + }
> +
> + /// Gets the state of the PHY.
> + pub fn state(&mut self) -> DeviceState {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + let state = unsafe { (*phydev).state };
> + match state {
> + bindings::phy_state::PHY_DOWN => DeviceState::Down,
> + bindings::phy_state::PHY_READY => DeviceState::Ready,
> + bindings::phy_state::PHY_HALTED => DeviceState::Halted,
> + bindings::phy_state::PHY_ERROR => DeviceState::Error,
> + bindings::phy_state::PHY_UP => DeviceState::Up,
> + bindings::phy_state::PHY_RUNNING => DeviceState::Running,
> + bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
> + bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
> + }
> + }
> +
> + /// Returns true if the PHY has no link.
> + pub fn get_link(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).link() != 0 }
> + }
> +
> + /// Returns true if auto-negotiation is enabled.
> + pub fn is_autoneg_enabled(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
> + }
> +
> + /// Returns true if auto-negotiation is completed.
> + pub fn is_autoneg_completed(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).autoneg_complete() != 0 }
> + }
> +
> + /// Sets the speed of the PHY.
> + pub fn set_speed(&mut self, speed: i32) {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe {
> + (*phydev).speed = speed;
> + }
> + }
> +
> + /// Sets duplex mode.
> + pub fn set_duplex(&mut self, mode: DuplexMode) {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe {
> + match mode {
> + DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
> + DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
> + DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
> + }
> + }
> + }
> +
> + /// Reads a given C22 PHY register.
> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
I have no knowledge of the driver and the device, but is
it really fine to supply any 16 bit integer value here?
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
> + };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret as u16)
> + }
Not sure if this is nicer, but you could use
`to_result(ret).map(|()| ret as u16)` instead.
> + }
> +
> + /// Writes a given C22 PHY register.
> + pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32, val)
> + };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
Use `error::to_result` instead.
> + }
> +
> + /// Reads a paged register.
> + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_read_paged(phydev, page as i32, regnum as u32) };
> + if ret < 0 {
> + return 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 = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + unsafe {
> + bindings::phy_resolve_aneg_linkmode(phydev);
> + }
> + }
> +
> + /// Executes software reset the PHY via BMCR_RESET bit.
> + pub fn genphy_soft_reset(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_soft_reset(phydev) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
Use `error::to_result` instead.
> + }
> +
> + /// Initializes the PHY.
> + pub fn init_hw(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_init_hw(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
Use `error::to_result` instead. And below...
> + }
> +
> + /// Starts auto-negotiation.
> + pub fn start_aneg(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::phy_start_aneg(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Resumes the PHY via BMCR_PDOWN bit.
> + pub fn genphy_resume(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_resume(phydev) };
> + if ret != 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Suspends the PHY via BMCR_PDOWN bit.
> + pub fn genphy_suspend(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_suspend(phydev) };
> + if ret != 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Checks the link status and updates current link state.
> + pub fn genphy_read_status(&mut self) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_status(phydev) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret as u16)
> + }
> + }
> +
> + /// Updates the link status.
> + pub fn genphy_update_link(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_update_link(phydev) };
> + if ret < 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Reads Link partner ability.
> + pub fn genphy_read_lpa(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_lpa(phydev) };
> + if ret < 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Reads PHY abilities.
> + pub fn genphy_read_abilities(&mut self) -> Result {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe { bindings::genphy_read_abilities(phydev) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + } else {
> + Ok(())
> + }
> + }
> +}
> +
> +// Will be replaced with the bitflags macro once it's merged in upstream.
> +const fn bit(shift: u8) -> u32 {
> + 1 << shift
> +}
> +/// PHY is internal.
> +pub const PHY_IS_INTERNAL: u32 = bit(0);
> +/// PHY needs to be reset after the refclk is enabled.
> +pub const PHY_RST_AFTER_CLK_EN: u32 = bit(1);
> +/// Polling is used to detect PHY status changes.
> +pub const PHY_POLL_CABLE_TEST: u32 = bit(2);
> +/// Don't suspend.
> +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = bit(3);
> +
> +/// An adapter for the registration of a PHY driver.
> +pub struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}
I think this type can be private.
> +
> +impl<T: Driver> Adapter<T> {
> + /// Creates a new `Adapter` instance.
> + pub const fn new() -> Adapter<T> {
> + Self { _p: PhantomData }
> + }
> +
> + unsafe extern "C" fn soft_reset_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::soft_reset(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn get_features_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::get_features(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::suspend(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::resume(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn config_aneg_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::config_aneg(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn read_status_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::read_status(dev)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn match_phy_device_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::match_phy_device(dev) as i32
> + }
> +
> + unsafe extern "C" fn read_mmd_callback(
> + phydev: *mut bindings::phy_device,
> + devnum: i32,
> + regnum: u16,
> + ) -> i32 {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> + Ok(ret.into())
> + })
> + }
> +
> + unsafe extern "C" fn write_mmd_callback(
> + phydev: *mut bindings::phy_device,
> + devnum: i32,
> + regnum: u16,
> + val: u16,
> + ) -> i32 {
> + from_result(|| {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::write_mmd(dev, devnum as u8, regnum, val)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::link_change_notify(dev);
> + }
> +
> + /// Creates the kernel's `phy_driver` instance.
> + ///
> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
In order to make `Adapter` a private type, you have
to make this function standalone (and generic).
Or create a standalone function that just calls this.
> + Opaque::new(bindings::phy_driver {
> + name: T::NAME.as_char_ptr() as *mut i8,
> + flags: <T>::FLAGS,
> + phy_id: <T>::PHY_ID,
> + phy_id_mask: <T>::PHY_ID_MASK,
> + soft_reset: if <T>::HAS_SOFT_RESET {
> + Some(Self::soft_reset_callback)
> + } else {
> + None
> + },
> + get_features: if <T>::HAS_GET_FEATURES {
> + Some(Self::get_features_callback)
> + } else {
> + None
> + },
> + match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
> + Some(Self::match_phy_device_callback)
> + } else {
> + None
> + },
> + suspend: if <T>::HAS_SUSPEND {
> + Some(Self::suspend_callback)
> + } else {
> + None
> + },
> + resume: if <T>::HAS_RESUME {
> + Some(Self::resume_callback)
> + } else {
> + None
> + },
> + config_aneg: if <T>::HAS_CONFIG_ANEG {
> + Some(Self::config_aneg_callback)
> + } else {
> + None
> + },
> + read_status: if <T>::HAS_READ_STATUS {
> + Some(Self::read_status_callback)
> + } else {
> + None
> + },
> + read_mmd: if <T>::HAS_READ_MMD {
> + Some(Self::read_mmd_callback)
> + } else {
> + None
> + },
> + write_mmd: if <T>::HAS_WRITE_MMD {
> + Some(Self::write_mmd_callback)
> + } else {
> + None
> + },
> + link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
> + Some(Self::link_change_notify_callback)
> + } else {
> + None
> + },
> + // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
> + // sets `Option<&F>` to be `None`.
> + ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> + })
> + }
> +}
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +#[vtable]
> +pub trait Driver {
> + /// Corresponds to `flags` in `struct phy_driver`.
> + const FLAGS: u32 = 0;
> + /// Corresponds to `name` in `struct phy_driver`.
> + const NAME: &'static CStr;
> + /// Corresponds to `phy_id` in `struct phy_driver`.
> + const PHY_ID: u32 = 0;
> + /// Corresponds to `phy_id_mask` in `struct phy_driver`.
> + const PHY_ID_MASK: u32 = 0;
> +
> + /// Corresponds to `soft_reset` in `struct phy_driver`.
> + fn soft_reset(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `get_features` in `struct phy_driver`.
> + fn get_features(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `match_phy_device` in `struct phy_driver`.
> + fn match_phy_device(_dev: &mut Device) -> bool {
> + false
> + }
> +
> + /// Corresponds to `config_aneg` in `struct phy_driver`.
> + fn config_aneg(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `read_status` in `struct phy_driver`.
> + fn read_status(_dev: &mut Device) -> Result<u16> {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `suspend` in `struct phy_driver`.
> + fn suspend(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `resume` in `struct phy_driver`.
> + fn resume(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `read_mmd` in `struct phy_driver`.
> + fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `write_mmd` in `struct phy_driver`.
> + fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
> + Err(code::ENOTSUPP)
> + }
> +
> + /// Corresponds to `link_change_notify` in `struct phy_driver`.
> + fn link_change_notify(_dev: &mut Device) {}
> +}
> +
> +/// Registration structure for a PHY driver.
> +pub struct Registration {
> + module: &'static crate::ThisModule,
> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> +}
> +
> +impl Registration {
> + /// Creates a new `Registration` instance.
> + pub fn new(module: &'static crate::ThisModule) -> Self {
> + Registration {
> + module,
> + drivers: None,
> + }
> + }
> +
> + /// Registers a PHY driver.
> + pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
> + if drivers.len() > 0 {
> + // SAFETY: Just an FFI call.
> + let ret = unsafe {
> + bindings::phy_drivers_register(
> + drivers[0].get(),
> + drivers.len() as i32,
> + self.module.0,
> + )
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> + self.drivers = Some(drivers);
> + Ok(())
> + } else {
> + Err(code::EINVAL)
> + }
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + if let Some(drv) = self.drivers.take() {
> + // SAFETY: Just an FFI call.
> + unsafe {
> + bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32);
> + }
> + }
> + }
> +}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}
> +
> +const DEVICE_MASK_EXACT: u32 = !0;
> +const DEVICE_MASK_MODEL: u32 = !0 << 4;
> +const DEVICE_MASK_VENDOR: u32 = !0 << 10;
> +
> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> + /// Corresponds to `phy_id` in `struct mdio_device_id`.
> + pub id: u32,
> + mask: DeviceMask,
> +}
> +
> +impl DeviceId {
> + /// Creates a new instance with the exact match mask.
> + pub const fn new_with_exact_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Exact,
> + }
> + }
> +
> + /// Creates a new instance with the model match mask.
> + pub const fn new_with_model_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Model,
> + }
> + }
> +
> + /// Creates a new instance with the vendor match mask.
> + pub const fn new_with_vendor_mask(id: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Vendor,
> + }
> + }
> +
> + /// Creates a new instance with a custom match mask.
> + pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
> + DeviceId {
> + id,
> + mask: DeviceMask::Custom(mask),
> + }
> + }
> +
> + /// Creates a new instance from [`Driver`].
> + pub const fn new_with_driver<T: Driver>() -> Self {
> + DeviceId {
> + id: T::PHY_ID,
> + mask: DeviceMask::new(T::PHY_ID_MASK),
> + }
> + }
> +
> + /// Get a mask as u32.
> + pub const fn mask_as_int(self) -> u32 {
> + match self.mask {
> + DeviceMask::Exact => DEVICE_MASK_EXACT,
> + DeviceMask::Model => DEVICE_MASK_MODEL,
> + DeviceMask::Vendor => DEVICE_MASK_VENDOR,
> + DeviceMask::Custom(mask) => mask,
> + }
> + }
> +}
> +
> +enum DeviceMask {
> + Exact,
> + Model,
> + Vendor,
> + Custom(u32),
> +}
> +
> +impl DeviceMask {
> + const fn new(mask: u32) -> Self {
> + match mask {
> + DEVICE_MASK_EXACT => DeviceMask::Exact,
> + DEVICE_MASK_MODEL => DeviceMask::Model,
> + DEVICE_MASK_VENDOR => DeviceMask::Vendor,
> + _ => DeviceMask::Custom(mask),
> + }
> + }
> +}
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver] and registers it.
> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +/// device_table: [
> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
> +/// DeviceId::new_with_driver::<PhyAX88796B>()
> +/// ],
> +/// type: RustAsixPhy,
> +/// name: "rust_asix_phy",
> +/// author: "Rust for Linux Contributors",
> +/// description: "Rust Asix PHYs driver",
> +/// license: "GPL",
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! module_phy_driver {
> + (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> + (@count_devices $($x:expr),*) => {
> + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
> + };
> +
> + (@device_table $name:ident, [$($dev:expr),+]) => {
> + ::kernel::macros::paste! {
> + #[no_mangle]
> + static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
> + [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
> + }
> + };
> +
> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
> + struct Module<$modname> {
Why do you use `$modname` as a generic parameter? Can't
you just use `$modname` as the name of this struct?
> + _reg: kernel::net::phy::Registration,
> + _p: core::marker::PhantomData<$modname>,
> + }
> +
> + type ModuleType = Module<$modname>;
> +
> + $crate::prelude::module! {
> + type: ModuleType,
> + $($f)*
> + }
> +
> + static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
> + $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
> + ];
Not sure if a static is the best way to do this. You
might also be able to just store this array inside
of the `Module<$modname>` struct.
> +
> + impl kernel::Module for Module<$modname> {
> + fn init(module: &'static ThisModule) -> Result<Self> {
> + let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
> + // SAFETY: static `DRIVERS` array is used only in the C side.
This safety comment should address why you are allowed
to access this mutable static here.
> + unsafe {
> + reg.register(&DRIVERS)?;
> + }
> +
> + Ok(Module {
> + _reg: reg,
> + _p: core::marker::PhantomData,
> + })
> + }
> + }
> +
> + $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
> + }
> +}
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 13:19 ` Benno Lossin
@ 2023-09-25 10:24 ` FUJITA Tomonori
2023-09-25 15:41 ` Miguel Ojeda
0 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-25 10:24 UTC (permalink / raw)
To: benno.lossin
Cc: fujita.tomonori, rust-for-linux, andrew, tmgross,
miguel.ojeda.sandonis
On Sun, 24 Sep 2023 13:19:21 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
>> +/// Wraps the kernel's `struct phy_device`.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>
> This struct should have a type invariant saying that "`self.0` is
> always in a valid state.".
Added.
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> + /// may read or write to the `phy_device` object.
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> + unsafe { &mut *(ptr as *mut Self) }
>> + }
>> +
>> + /// Gets the id of the PHY.
>> + pub fn id(&mut self) -> u32 {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>
> These kinds of safety comments should be replaced by "`phydev` is
> pointing to a valid object by the type invariant of `Self`."
> Also for the other safety comments below.
Fixed all these comments.
>> + /// Reads a given C22 PHY register.
>> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
>
> I have no knowledge of the driver and the device, but is
> it really fine to supply any 16 bit integer value here?
No, I assume that the C side returns an error in such case.
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe {
>> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
>> + };
>> + if ret < 0 {
>> + Err(Error::from_errno(ret))
>> + } else {
>> + Ok(ret as u16)
>> + }
>
> Not sure if this is nicer, but you could use
> `to_result(ret).map(|()| ret as u16)` instead.
I'm not sure either. I have no preference though.
>> +
>> + /// Writes a given C22 PHY register.
>> + pub fn write(&mut self, regnum: u16, val: u16) -> Result {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe {
>> + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32, val)
>> + };
>> + if ret < 0 {
>> + Err(Error::from_errno(ret))
>> + } else {
>> + Ok(())
>> + }
>
> Use `error::to_result` instead.
Fixed.
>> + }
>> +
>> + /// Reads a paged register.
>> + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe { bindings::phy_read_paged(phydev, page as i32, regnum as u32) };
>> + if ret < 0 {
>> + return 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 = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + unsafe {
>> + bindings::phy_resolve_aneg_linkmode(phydev);
>> + }
>> + }
>> +
>> + /// Executes software reset the PHY via BMCR_RESET bit.
>> + pub fn genphy_soft_reset(&mut self) -> Result {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe { bindings::genphy_soft_reset(phydev) };
>> + if ret < 0 {
>> + Err(Error::from_errno(ret))
>> + } else {
>> + Ok(())
>> + }
>
> Use `error::to_result` instead.
Fixed.
>> + }
>> +
>> + /// Initializes the PHY.
>> + pub fn init_hw(&mut self) -> Result {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe { bindings::phy_init_hw(phydev) };
>> + if ret != 0 {
>> + return Err(Error::from_errno(ret));
>> + } else {
>> + Ok(())
>> + }
>
> Use `error::to_result` instead. And below...
Fixed all.
>> +/// An adapter for the registration of a PHY driver.
>> +pub struct Adapter<T: Driver> {
>> + _p: PhantomData<T>,
>> +}
>
> I think this type can be private.
>
>> +
>> +impl<T: Driver> Adapter<T> {
>> + /// Creates a new `Adapter` instance.
>> + pub const fn new() -> Adapter<T> {
>> + Self { _p: PhantomData }
>> + }
>> +
>> + unsafe extern "C" fn soft_reset_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::soft_reset(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn get_features_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::get_features(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::suspend(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::resume(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn config_aneg_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::config_aneg(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn read_status_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::read_status(dev)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn match_phy_device_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::match_phy_device(dev) as i32
>> + }
>> +
>> + unsafe extern "C" fn read_mmd_callback(
>> + phydev: *mut bindings::phy_device,
>> + devnum: i32,
>> + regnum: u16,
>> + ) -> i32 {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
>> + Ok(ret.into())
>> + })
>> + }
>> +
>> + unsafe extern "C" fn write_mmd_callback(
>> + phydev: *mut bindings::phy_device,
>> + devnum: i32,
>> + regnum: u16,
>> + val: u16,
>> + ) -> i32 {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::write_mmd(dev, devnum as u8, regnum, val)?;
>> + Ok(0)
>> + })
>> + }
>> +
>> + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::link_change_notify(dev);
>> + }
>> +
>> + /// Creates the kernel's `phy_driver` instance.
>> + ///
>> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
>
> In order to make `Adapter` a private type, you have
> to make this function standalone (and generic).
> Or create a standalone function that just calls this.
done, nice.
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver] and registers it.
>> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
>> +/// for module loading into the module binary file.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +/// device_table: [
>> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
>> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
>> +/// DeviceId::new_with_driver::<PhyAX88796B>()
>> +/// ],
>> +/// type: RustAsixPhy,
>> +/// name: "rust_asix_phy",
>> +/// author: "Rust for Linux Contributors",
>> +/// description: "Rust Asix PHYs driver",
>> +/// license: "GPL",
>> +/// }
>> +/// ```
>> +#[macro_export]
>> +macro_rules! module_phy_driver {
>> + (@replace_expr $_t:tt $sub:expr) => {$sub};
>> +
>> + (@count_devices $($x:expr),*) => {
>> + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> + };
>> +
>> + (@device_table $name:ident, [$($dev:expr),+]) => {
>> + ::kernel::macros::paste! {
>> + #[no_mangle]
>> + static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
>> + [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
>> + }
>> + };
>> +
>> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
>> + struct Module<$modname> {
>
> Why do you use `$modname` as a generic parameter? Can't
> you just use `$modname` as the name of this struct?
I followed module_driver! macro and similr macros like
module_platform_driver! in rust branch; a device driver programmer can
define a struct for Module.
>> + _reg: kernel::net::phy::Registration,
>> + _p: core::marker::PhantomData<$modname>,
>> + }
>> +
>> + type ModuleType = Module<$modname>;
>> +
>> + $crate::prelude::module! {
>> + type: ModuleType,
>> + $($f)*
>> + }
>> +
>> + static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
>> + $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
>> + ];
>
> Not sure if a static is the best way to do this. You
> might also be able to just store this array inside
> of the `Module<$modname>` struct.
With that approach, we can't make the array static, right? I can't
figure out a way to handle lifetime of Registration and
Module<$modname>; Registration needs a pointer to the array for
its drop().
>> + impl kernel::Module for Module<$modname> {
>> + fn init(module: &'static ThisModule) -> Result<Self> {
>> + let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
>> + // SAFETY: static `DRIVERS` array is used only in the C side.
>
> This safety comment should address why you are allowed
> to access this mutable static here.
The Rust sides just passes static `DRIVERS` array to the C side, which
is used only in the C side with appropriate synchronization mechanism.
?
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 10:24 ` FUJITA Tomonori
@ 2023-09-25 15:41 ` Miguel Ojeda
2023-09-26 13:46 ` FUJITA Tomonori
0 siblings, 1 reply; 47+ messages in thread
From: Miguel Ojeda @ 2023-09-25 15:41 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: benno.lossin, rust-for-linux, andrew, tmgross
On Mon, Sep 25, 2023 at 12:24 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I'm not sure either. I have no preference though.
One way we used in other places in the past is:
to_result(...)?;
Ok(...)
By the way, for other kinds of conditions (i.e. if one cannot use
`to_result`), I would suggest the early return style, i.e. the usual C
kernel style:
if condition {
return Err(...);
}
Ok(...)
Sometimes for the last one we do this too when the function is
"symmetric" (personally I wouldn't mind just doing early for those
cases too, to be consistent):
if condition {
Err(...)
} else {
Ok(...)
}
What we should not do, however, is this (which you used in the patch
before Benno's suggestions to use `to_result`), i.e. `return` and
`else`:
if condition {
return Err(...);
} else {
Ok(...)
}
(By the way, I just reported
https://github.com/rust-lang/rust/issues/116150 since codegen seems to
suffer in some cases)
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 15:41 ` Miguel Ojeda
@ 2023-09-26 13:46 ` FUJITA Tomonori
2023-09-27 10:49 ` Miguel Ojeda
0 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-26 13:46 UTC (permalink / raw)
To: miguel.ojeda.sandonis
Cc: fujita.tomonori, benno.lossin, rust-for-linux, andrew, tmgross
Hi,
On Mon, 25 Sep 2023 17:41:41 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Sep 25, 2023 at 12:24 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> I'm not sure either. I have no preference though.
>
> One way we used in other places in the past is:
>
> to_result(...)?;
> Ok(...)
>
> By the way, for other kinds of conditions (i.e. if one cannot use
> `to_result`), I would suggest the early return style, i.e. the usual C
> kernel style:
>
> if condition {
> return Err(...);
> }
>
> Ok(...)
>
> Sometimes for the last one we do this too when the function is
> "symmetric" (personally I wouldn't mind just doing early for those
> cases too, to be consistent):
>
> if condition {
> Err(...)
> } else {
> Ok(...)
> }
>
> What we should not do, however, is this (which you used in the patch
> before Benno's suggestions to use `to_result`), i.e. `return` and
> `else`:
>
> if condition {
> return Err(...);
> } else {
> Ok(...)
> }
Thanks! I updated the code.
No helper function for a function that returns avalue like the
following?
let ret = unsafe {bindings::some_function()};
if ret < 0 {
Err(Error::from_errno(ret))
} else {
Ok(ret)
}
if to_result() returns a value, it can handle both types.
> (By the way, I just reported
> https://github.com/rust-lang/rust/issues/116150 since codegen seems to
> suffer in some cases)
Hmm, how this can happen? Quite surprising!
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-26 13:46 ` FUJITA Tomonori
@ 2023-09-27 10:49 ` Miguel Ojeda
2023-09-27 11:19 ` FUJITA Tomonori
0 siblings, 1 reply; 47+ messages in thread
From: Miguel Ojeda @ 2023-09-27 10:49 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: benno.lossin, rust-for-linux, andrew, tmgross
On Tue, Sep 26, 2023 at 3:46 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Thanks! I updated the code.
My pleasure!
> No helper function for a function that returns avalue like the
> following?
>
> let ret = unsafe {bindings::some_function()};
> if ret < 0 {
> Err(Error::from_errno(ret))
> } else {
> Ok(ret)
> }
>
> if to_result() returns a value, it can handle both types.
Do you mean a function that takes the `ok` value:
let ret = ...;
to_result_ok(ret, ret as u16)
or simply having `to_result` return the value back in the ` Ok` case:
let ret = to_result(...)?;
Ok(ret as u16)
? (which is what Wedson implicitly suggested, but `to_result` does not
return the not-an-error-integer back at the moment in the happy path)?
Or perhaps both?
The returning one has the advantage that we see the explicit `Ok()` at
the end, without having to remember what an extra function does or the
order of its parameters. It also allows one-liners with `?` like the
`map()` approach assuming one really, really needs/wants those:
Ok(to_result(...)? as u16)
It also gives a chance to give a better name to the value when needed,
since at that point it is not an "error-or-value integer" anymore, but
the actual value we want, i.e.:
let my_thing = to_result(...)?;
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-27 10:49 ` Miguel Ojeda
@ 2023-09-27 11:19 ` FUJITA Tomonori
2023-10-09 12:28 ` Miguel Ojeda
0 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-27 11:19 UTC (permalink / raw)
To: miguel.ojeda.sandonis
Cc: fujita.tomonori, benno.lossin, rust-for-linux, andrew, tmgross
On Wed, 27 Sep 2023 12:49:21 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>> No helper function for a function that returns avalue like the
>> following?
>>
>> let ret = unsafe {bindings::some_function()};
>> if ret < 0 {
>> Err(Error::from_errno(ret))
>> } else {
>> Ok(ret)
>> }
>>
>> if to_result() returns a value, it can handle both types.
>
> Do you mean a function that takes the `ok` value:
>
> let ret = ...;
> to_result_ok(ret, ret as u16)
>
> or simply having `to_result` return the value back in the ` Ok` case:
>
> let ret = to_result(...)?;
> Ok(ret as u16)
>
> ? (which is what Wedson implicitly suggested, but `to_result` does not
> return the not-an-error-integer back at the moment in the happy path)?
The latter. I think that to_result() would be more handy if it
returns an integer in the happy path like:
pub fn to_result(err: core::ffi::c_int) -> Result<core::ffi::c_int> {
if err < 0 {
Err(Error::from_errno(err))
} else {
Ok(err)
}
}
> The returning one has the advantage that we see the explicit `Ok()` at
> the end, without having to remember what an extra function does or the
> order of its parameters. It also allows one-liners with `?` like the
> `map()` approach assuming one really, really needs/wants those:
>
> Ok(to_result(...)? as u16)
>
> It also gives a chance to give a better name to the value when needed,
> since at that point it is not an "error-or-value integer" anymore, but
> the actual value we want, i.e.:
>
> let my_thing = to_result(...)?;
Surely.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-27 11:19 ` FUJITA Tomonori
@ 2023-10-09 12:28 ` Miguel Ojeda
0 siblings, 0 replies; 47+ messages in thread
From: Miguel Ojeda @ 2023-10-09 12:28 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: benno.lossin, rust-for-linux, andrew, tmgross
On Wed, Sep 27, 2023 at 1:19 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> The latter. I think that to_result() would be more handy if it
> returns an integer in the happy path like:
>
> pub fn to_result(err: core::ffi::c_int) -> Result<core::ffi::c_int> {
> if err < 0 {
> Err(Error::from_errno(err))
> } else {
> Ok(err)
> }
> }
We discussed this the other week in our weekly meeting and a few
alternatives too -- I will send a patch series on this (and related
bits).
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
2023-09-24 12:56 ` Wedson Almeida Filho
2023-09-24 13:19 ` Benno Lossin
@ 2023-09-24 17:00 ` Andrew Lunn
2023-09-24 18:03 ` Miguel Ojeda
2023-09-25 18:32 ` Andrew Lunn
` (2 subsequent siblings)
5 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-09-24 17:00 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, tmgross, miguel.ojeda.sandonis
On Sun, Sep 24, 2023 at 03:49:00PM +0900, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/Makefile | 1 +
> rust/bindings/bindings_helper.h | 3 +
> rust/kernel/lib.rs | 3 +
> rust/kernel/net.rs | 6 +
> rust/kernel/net/phy.rs | 760 ++++++++++++++++++++++++++++++++
Has there been any discussion about Kconfig and Rust?
I would expect to see some Kconfig changes here.
menuconfig PHYLIB
tristate "PHY Device support and infrastructure"
tristate means it can either be built into the kernel, be a kernel
module, or not built at all.
If it is not built at all, you should not build phy.rs, you are going
to get undefined symbols when you try to link it.
If phylib is a module, the only way it will work is if phy.rs is also
a module, so it can be loaded after the phylib module is loaded.
If phylib is built in, there are no restrictions.
There are different ways to solve this, and i'm not a Kconfig expert,
but maybe you need something like:
rust/Kconfig
config RUST_PHYLIB_ABSTRACTION
tristate "Rust abstraction for PHYLIB"
depends on PHYLIB
depends on RUST
help
Provide a Rust abstraction for phylib, which Rust PHY
drivers can use.
and then in Makefile something like:
ifdef CONFIG_RUST_PHYLIB_ABSTRACTION
obj-$(CONFIG_RUST) += phy.rs
endif
And the Rust PHY driver would then have a
depends on RUST_PHYLIB_ABSTRACTION
When you have something which you think should work, we can maybe ask
Arnd Bergmann to run his randconfig build bot on a tree and see if it
finds any instances when the build breaks.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 17:00 ` Andrew Lunn
@ 2023-09-24 18:03 ` Miguel Ojeda
2023-09-25 13:28 ` Andrew Lunn
2023-09-25 13:43 ` Andrew Lunn
0 siblings, 2 replies; 47+ messages in thread
From: Miguel Ojeda @ 2023-09-24 18:03 UTC (permalink / raw)
To: Andrew Lunn; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
On Sun, Sep 24, 2023 at 7:00 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Has there been any discussion about Kconfig and Rust?
Yes, of course :)
What we do so far is only include `phy.rs` if `CONFIG_PHYLIB` is
enabled, using conditional compilation. That is the code in
`rust/kernel/net.rs` in the patch above:
#[cfg(CONFIG_PHYLIB)]
pub mod phy;
That is, enabling Rust gives you abstractions for everything that the
configuration enables.
If we want to avoid enabling Rust for specific subsystems, then, of
course, we could add per-subsystem Kconfig symbols to gate those, like
you suggest.
> If it is not built at all, you should not build phy.rs, you are going
> to get undefined symbols when you try to link it.
One clarification: in Rust, each `.rs` is not transformed into an
object file. Instead, the "crate" is the translation unit. In
addition, generic and "inline" code is kept in special metadata files
generated by the compiler.
Currently, Rust abstractions like `rust/kernel/net/phy.rs` are not a
crate, but a module within a single crate, `kernel`, that has all the
abstractions (that are enabled) and is always built-in so far. Rust
kernel modules are their own crates already, but not the abstractions
(until the new build system).
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 18:03 ` Miguel Ojeda
@ 2023-09-25 13:28 ` Andrew Lunn
2023-09-25 13:43 ` Andrew Lunn
1 sibling, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 13:28 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
On Sun, Sep 24, 2023 at 08:03:06PM +0200, Miguel Ojeda wrote:
> On Sun, Sep 24, 2023 at 7:00 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Has there been any discussion about Kconfig and Rust?
>
> Yes, of course :)
>
> What we do so far is only include `phy.rs` if `CONFIG_PHYLIB` is
> enabled, using conditional compilation. That is the code in
> `rust/kernel/net.rs` in the patch above:
>
> #[cfg(CONFIG_PHYLIB)]
> pub mod phy;
>
> That is, enabling Rust gives you abstractions for everything that the
> configuration enables.
And this has some magic to solve modules vs built in problems?
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 18:03 ` Miguel Ojeda
2023-09-25 13:28 ` Andrew Lunn
@ 2023-09-25 13:43 ` Andrew Lunn
2023-09-25 15:42 ` Miguel Ojeda
1 sibling, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 13:43 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
> If we want to avoid enabling Rust for specific subsystems, then, of
> course, we could add per-subsystem Kconfig symbols to gate those, like
> you suggest.
I think the end goal should be that if there is no Rust PHY driver
module loaded, the Rust PHY abstraction should also not be loaded. I
think best practice would also be, if the kernel configuration does
not contain a Rust PHY driver, don't even build the Rust PHY
abstraction.
>
> > If it is not built at all, you should not build phy.rs, you are going
> > to get undefined symbols when you try to link it.
>
> One clarification: in Rust, each `.rs` is not transformed into an
> object file. Instead, the "crate" is the translation unit. In
> addition, generic and "inline" code is kept in special metadata files
> generated by the compiler.
>
> Currently, Rust abstractions like `rust/kernel/net/phy.rs` are not a
> crate, but a module within a single crate, `kernel`, that has all the
> abstractions (that are enabled) and is always built-in so far.
This 'always built-in' is the issues i was trying to point to. phylib
can be built as a module. In that case, the symbols phy.rs needs are
not available until the phylib module is loaded. And that just does
not work. The normal solution to this is to use a Kconfig DEPENDS
ON. Kconfig will then remove the Module option, leaving the user to
either make it builtin, or not built at all.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 13:43 ` Andrew Lunn
@ 2023-09-25 15:42 ` Miguel Ojeda
2023-09-25 16:53 ` Andrew Lunn
0 siblings, 1 reply; 47+ messages in thread
From: Miguel Ojeda @ 2023-09-25 15:42 UTC (permalink / raw)
To: Andrew Lunn; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
On Mon, Sep 25, 2023 at 3:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I think the end goal should be that if there is no Rust PHY driver
> module loaded, the Rust PHY abstraction should also not be loaded. I
Agreed, that is the goal.
> think best practice would also be, if the kernel configuration does
> not contain a Rust PHY driver, don't even build the Rust PHY
> abstraction.
I am not sure about this one: a user may want to have abstractions
built-in but the drivers as loadable modules. One also needs to
consider out-of-tree modules even if we know everything that got built
as a module in-tree.
> This 'always built-in' is the issues i was trying to point to. phylib
Yeah, we are aware of the limitation. This was another consequence of
the way we had to build things in a simple way given Kbuild does not
map well to Rust, and thus the need for the new build system.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 15:42 ` Miguel Ojeda
@ 2023-09-25 16:53 ` Andrew Lunn
2023-09-25 17:26 ` Miguel Ojeda
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 16:53 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
On Mon, Sep 25, 2023 at 05:42:51PM +0200, Miguel Ojeda wrote:
> On Mon, Sep 25, 2023 at 3:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > I think the end goal should be that if there is no Rust PHY driver
> > module loaded, the Rust PHY abstraction should also not be loaded. I
>
> Agreed, that is the goal.
>
> > think best practice would also be, if the kernel configuration does
> > not contain a Rust PHY driver, don't even build the Rust PHY
> > abstraction.
>
> I am not sure about this one: a user may want to have abstractions
> built-in but the drivers as loadable modules.
Kconfig should allow this. A module with a DEPENDS ON, just ensures
that the option it points to is available in the configuration in a
way it is usable. The Lack of a DEPENDS ON on an Kconfig symbol does
not force it off. The user can enable it, even if there is no user.
> One also needs to consider out-of-tree modules even if we know
> everything that got built as a module in-tree.
At least in networking, out of tree modules are towards the very
bottom of the feature list. I would be quite happy to break them if
they prevent in kernel features, bloat the kernel, or make maintenance
harder.
> > This 'always built-in' is the issues i was trying to point to. phylib
>
> Yeah, we are aware of the limitation. This was another consequence of
> the way we had to build things in a simple way given Kbuild does not
> map well to Rust, and thus the need for the new build system.
So i think your need to add a DEPENDS ON PHYLIB for the moment, until
the new build system is available. Otherwise we are going to get
broken builds reported by 0-day, and the randconf build bots, assuming
they even try to build Rust at the moment. But at some point, they
will...
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-25 16:53 ` Andrew Lunn
@ 2023-09-25 17:26 ` Miguel Ojeda
0 siblings, 0 replies; 47+ messages in thread
From: Miguel Ojeda @ 2023-09-25 17:26 UTC (permalink / raw)
To: Andrew Lunn; +Cc: FUJITA Tomonori, rust-for-linux, tmgross
On Mon, Sep 25, 2023 at 6:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Kconfig should allow this. A module with a DEPENDS ON, just ensures
> that the option it points to is available in the configuration in a
> way it is usable. The Lack of a DEPENDS ON on an Kconfig symbol does
> not force it off. The user can enable it, even if there is no user.
Ah, so by "if the kernel configuration does not contain a Rust PHY
driver, don't even build the Rust PHY abstraction" you meant from the
user point of view?
I understood you were suggesting doing so automatically, that is why I
said that approach would not be flexible enough.
(We are aware how `depends on` works)
> broken builds reported by 0-day, and the randconf build bots, assuming
> they even try to build Rust at the moment. But at some point, they
> will...
Some good news: KernelCI and 0-day have supported building with Rust
enabled for a while now and they do some runs. Of course,
coverage/configs/... will need to be expanded as Rust grows in the
kernel.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
` (2 preceding siblings ...)
2023-09-24 17:00 ` Andrew Lunn
@ 2023-09-25 18:32 ` Andrew Lunn
2023-09-25 19:15 ` Miguel Ojeda
2023-09-26 6:05 ` Trevor Gross
2023-09-26 6:54 ` Trevor Gross
5 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-09-25 18:32 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, tmgross, miguel.ojeda.sandonis
> + /// Resolves the advertisements into PHY settings.
> + pub fn resolve_aneg_linkmode(&mut self) {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + unsafe {
> + bindings::phy_resolve_aneg_linkmode(phydev);
> + }
> + }
void phy_resolve_aneg_linkmode(struct phy_device *phydev)
{
__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
int i;
linkmode_and(common, phydev->lp_advertising, phydev->advertising);
for (i = 0; i < ARRAY_SIZE(settings); i++)
if (test_bit(settings[i].bit, common)) {
phydev->speed = settings[i].speed;
phydev->duplex = settings[i].duplex;
break;
}
phy_resolve_aneg_pause(phydev);
}
EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
This symbol is exported such that only a GPL licensed module can use
it. So to keep with the spirit of this, i would expect that the Rust
function resolve_aneg_linkmode() also has some sort of markup so that
it can only be used by a GPL licensed module.
Is there any handling of this at the moment? We don't want using a
Rust wrapper being a way to get around the GPL restrictions.
Is this a FAQ? Is so, please point me at the documentation...
Thanks
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
` (3 preceding siblings ...)
2023-09-25 18:32 ` Andrew Lunn
@ 2023-09-26 6:05 ` Trevor Gross
2023-09-26 12:11 ` Andrew Lunn
2023-09-27 3:26 ` FUJITA Tomonori
2023-09-26 6:54 ` Trevor Gross
5 siblings, 2 replies; 47+ messages in thread
From: Trevor Gross @ 2023-09-26 6:05 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, andrew, miguel.ojeda.sandonis
On Sun, Sep 24, 2023 at 2:49 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> +impl Device {
> + /// Creates a new [`Device`] instance from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> + /// may read or write to the `phy_device` object.
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> + unsafe { &mut *(ptr as *mut Self) }
> + }
> +
Prefer .cast() over `as *` because it makes it makes it harder to
accidentally change mut/const
> + /// Returns true if the PHY has no link.
> + pub fn get_link(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).link() != 0 }
> + }
> +
Maybe put a `const LINK_IS_UP: c_uint = 0` in that function to make
the magic number a bit more clear.
Also, "true if phy has NO link" seems opposite to me - wouldn't it
make more sense to return true if there is a link? This is opposite of
C's internal representation but I think that's OK to avoid the double
negative. Or even map it to an enum?
> +
> + /// Returns true if auto-negotiation is completed.
> + pub fn is_autoneg_completed(&mut self) -> bool {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> + unsafe { (*phydev).autoneg_complete() != 0 }
> + }
> +
Probably the same thing as above with a const
> + /// Reads a given C22 PHY register.
> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
> + };
Prefer `from`/`into` instead of `as` for these lossless casts, it just
gives an sanity check when refactoring. A few other functions that
take `regnum` or `page` could use the same change.
I wonder if the there is any benefit to changing the u32 -> u16 casts
into checked conversions and at least printing
> + /// Resolves the advertisements into PHY settings.
> + pub fn resolve_aneg_linkmode(&mut self) {
> + let phydev = Opaque::get(&self.0);
> + // SAFETY: This object is initialized by the `from_raw` constructor,
> + // so an FFI call with a valid pointer.
> + unsafe {
> + bindings::phy_resolve_aneg_linkmode(phydev);
> + }
> + }
Semicolon outside the { } for one liner unsafes so it formats more compact
> + /// Updates the link status.
> + pub fn genphy_update_link(&mut self) -> Result // ...
> +
> + /// Reads Link partner ability.
> + pub fn genphy_read_lpa(&mut self) -> Result // ...
> +
> + /// Reads PHY abilities.
> + pub fn genphy_read_abilities(&mut self) -> Result // ...
Do these functions need to be public? It seems like they are just used
to update internal state, which is wrapped up into more user-facing
API
> +/// An adapter for the registration of a PHY driver.
> +pub struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> + /// Creates a new `Adapter` instance.
> + pub const fn new() -> Adapter<T> {
> + Self { _p: PhantomData }
> + }
> +
> + unsafe extern "C" fn soft_reset_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + [...]
What is the motivation for having an adapter as a type? Since it's
just a dummy struct that has no runtime meaning, I think the code
would be easier to follow if these functions were put in a module
mod phy_registration_helpers {
unsafe extern "C" fn soft_reset_callback<T: Driver>(
phydev: *mut bindings::phy_device,
) -> core::ffi::c_int { /* ... */ }
// ...
}
> + /// Creates the kernel's `phy_driver` instance.
> + ///
> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
This construction looks much better than v1 :)
> +
> +/// Corresponds to functions in `struct phy_driver`.
> +#[vtable]
> +pub trait Driver {
> + /// Corresponds to `flags` in `struct phy_driver`.
> + const FLAGS: u32 = 0;
> + /// Corresponds to `name` in `struct phy_driver`.
> + const NAME: &'static CStr;
> + /// Corresponds to `phy_id` in `struct phy_driver`.
> + const PHY_ID: u32 = 0;
> + /// Corresponds to `phy_id_mask` in `struct phy_driver`.
> + const PHY_ID_MASK: u32 = 0;
This also seems better to me, thanks for updating. Could this just
take a `DeviceMask`?
> +/// Registration structure for a PHY driver.
> +pub struct Registration {
> + module: &'static crate::ThisModule,
> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> +}
This new structure without the generics looks much better, with one note below
> +impl Registration {
> + /// Creates a new `Registration` instance.
> + pub fn new(module: &'static crate::ThisModule) -> Self {
> + Registration {
> + module,
> + drivers: None,
> + }
> + }
> +
> + /// Registers a PHY driver.
> + pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
> + if drivers.len() > 0 {
> + // SAFETY: Just an FFI call.
> + let ret = unsafe {
> + bindings::phy_drivers_register(
> + drivers[0].get(),
> + drivers.len() as i32,
> + self.module.0,
> + )
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> + self.drivers = Some(drivers);
> + Ok(())
> + } else {
> + Err(code::EINVAL)
> + }
> + }
> +}
I don't think there's any need to have `new` and `register` broken
out. This is actually a _bad_ pattern because the deregistration
function gets called (via drop) even if it is never registered.
I believe that `phy_drivers_unregister` should check against this, but
all the same I would strongly suggest:
#[must_use]
pub fn register(
module: &'static crate::ThisModule,
drivers: &'static [Opaque<bindings::phy_driver>]
) -> Result<Self>;
And no other function.
Note the `must_use` to make sure the handle is held rather than
instantly dropped.
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + if let Some(drv) = self.drivers.take() {
> + // SAFETY: Just an FFI call.
> + unsafe {
> + bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32);
> + }
> + }
> + }
> +}
> +
> +const DEVICE_MASK_EXACT: u32 = !0;
> +const DEVICE_MASK_MODEL: u32 = !0 << 4;
> +const DEVICE_MASK_VENDOR: u32 = !0 << 10;
See my comment above about maybe having this be an enum
> +enum DeviceMask {
> + Exact,
> + Model,
> + Vendor,
> + Custom(u32),
> +}
LGTM
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver] and registers it.
> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> +/// for module loading into the module binary file.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +/// device_table: [
> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
> +/// DeviceId::new_with_driver::<PhyAX88796B>()
> +/// ],
> +/// type: RustAsixPhy,
> +/// name: "rust_asix_phy",
> +/// author: "Rust for Linux Contributors",
> +/// description: "Rust Asix PHYs driver",
> +/// license: "GPL",
> +/// }
> +/// ```
I'm glad this worked out, a single macro is nice.
Since almost all devices will just use their standard DeviceId, could
that be done by default unless a specific ID is specified? Something
like
drivers: [
PhyAX88772A,
// register the same phy with a second ID
{ phy: PhyAX88772A, id: some_other_device_id },
PhyAX88772C,
PhyAX88796B
],
type: RustAsixPhy,
// ...
Though there may be a nicer looking syntax
> +#[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 $name:ident, [$($dev:expr),+]) => {
> + ::kernel::macros::paste! {
> + #[no_mangle]
> + static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
> + [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
> + }
> + };
> +
> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
> + struct Module<$modname> {
> + _reg: kernel::net::phy::Registration,
> + _p: core::marker::PhantomData<$modname>,
> + }
> +
> + type ModuleType = Module<$modname>;
> +
> + $crate::prelude::module! {
> + type: ModuleType,
> + $($f)*
> + }
> +
> + static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
> + $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
> + ];
> +
> + impl kernel::Module for Module<$modname> {
> + fn init(module: &'static ThisModule) -> Result<Self> {
> + let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
> + // SAFETY: static `DRIVERS` array is used only in the C side.
> + unsafe {
> + reg.register(&DRIVERS)?;
> + }
> +
> + Ok(Module {
> + _reg: reg,
> + _p: core::marker::PhantomData,
> + })
> + }
> + }
> +
> + $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
> + }
> +}
These have to be hand formatted until rustfmt is able to handle macro
bodies, just try to make it look a bit more like code
(@device_table $name:ident, [$($dev:expr),+]) => {
::kernel::macros::paste! {
#[no_mangle]
static [<__mod_mdio__ $name _device_table>]: [
kernel::bindings::mdio_device_id;
$crate::module_phy_driver!(@count_devices $($dev),+) + 1
] = [
$(kernel::bindings::mdio_device_id {
phy_id: $dev.id,
phy_id_mask: $dev.mask_as_int()
}),+
kernel::bindings::mdio_device_id {
phy_id: 0,
phy_id_mask: 0
}
];
}
};
and
static mut DRIVERS: [
kernel::types::Opaque<kernel::bindings::phy_driver>;
$crate::module_phy_driver!(@count_devices $($driver),+)
] = [
$(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+
];
Note also that `$( ... ),+,` expands the same as `$( ... ),+` so the
second is a bit cleaner without the redundant `,` (at least in my
quick test this worked)
> --
> 2.34.1
>
Thanks for all the updates. Overall the usability is going in the
right direction since v1
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-26 6:05 ` Trevor Gross
@ 2023-09-26 12:11 ` Andrew Lunn
2023-09-27 3:26 ` FUJITA Tomonori
1 sibling, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-26 12:11 UTC (permalink / raw)
To: Trevor Gross; +Cc: FUJITA Tomonori, rust-for-linux, miguel.ojeda.sandonis
> > + /// Updates the link status.
> > + pub fn genphy_update_link(&mut self) -> Result // ...
> > +
> > + /// Reads Link partner ability.
> > + pub fn genphy_read_lpa(&mut self) -> Result // ...
> > +
> > + /// Reads PHY abilities.
> > + pub fn genphy_read_abilities(&mut self) -> Result // ...
>
> Do these functions need to be public? It seems like they are just used
> to update internal state, which is wrapped up into more user-facing
> API
There are C drivers which need these.
Any function starting with genphy_ is based on the 802.3 standard. But
PHY vendors get stuff wrong, and workarounds are needed. That often
means calling one of these helpers, and then doing additional steps to
fix up whatever is broken.
So these probably don't need exporting for the first Rust driver, but
a later driver might need them.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-26 6:05 ` Trevor Gross
2023-09-26 12:11 ` Andrew Lunn
@ 2023-09-27 3:26 ` FUJITA Tomonori
1 sibling, 0 replies; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-27 3:26 UTC (permalink / raw)
To: tmgross; +Cc: fujita.tomonori, rust-for-linux, andrew, miguel.ojeda.sandonis
On Tue, 26 Sep 2023 02:05:41 -0400
Trevor Gross <tmgross@umich.edu> wrote:
> On Sun, Sep 24, 2023 at 2:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> + /// may read or write to the `phy_device` object.
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> + unsafe { &mut *(ptr as *mut Self) }
>> + }
>> +
>
> Prefer .cast() over `as *` because it makes it makes it harder to
> accidentally change mut/const
fixed.
>> + /// Returns true if the PHY has no link.
>> + pub fn get_link(&mut self) -> bool {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe { (*phydev).link() != 0 }
>> + }
>> +
>
> Maybe put a `const LINK_IS_UP: c_uint = 0` in that function to make
> the magic number a bit more clear.
>
> Also, "true if phy has NO link" seems opposite to me - wouldn't it
> make more sense to return true if there is a link? This is opposite of
> C's internal representation but I think that's OK to avoid the double
> negative. Or even map it to an enum?
Oops, the comment is wrong. I updated the function like the following:
/// Returns true if the link is up.
pub fn get_link(&mut self) -> bool {
const LINK_IS_UP: u32 = 1;
let phydev = self.0.get();
// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
unsafe { (*phydev).link() == LINK_IS_UP }
}
>> +
>> + /// Returns true if auto-negotiation is completed.
>> + pub fn is_autoneg_completed(&mut self) -> bool {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> + unsafe { (*phydev).autoneg_complete() != 0 }
>> + }
>> +
>
> Probably the same thing as above with a const
Fixed.
>> + /// Reads a given C22 PHY register.
>> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + let ret = unsafe {
>> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
>> + };
>
> Prefer `from`/`into` instead of `as` for these lossless casts, it just
> gives an sanity check when refactoring. A few other functions that
> take `regnum` or `page` could use the same change.
>
> I wonder if the there is any benefit to changing the u32 -> u16 casts
> into checked conversions and at least printing
>
>> + /// Resolves the advertisements into PHY settings.
>> + pub fn resolve_aneg_linkmode(&mut self) {
>> + let phydev = Opaque::get(&self.0);
>> + // SAFETY: This object is initialized by the `from_raw` constructor,
>> + // so an FFI call with a valid pointer.
>> + unsafe {
>> + bindings::phy_resolve_aneg_linkmode(phydev);
>> + }
>> + }
>
> Semicolon outside the { } for one liner unsafes so it formats more compact
Fixed this and the similar code.
>> + /// Updates the link status.
>> + pub fn genphy_update_link(&mut self) -> Result // ...
>> +
>> + /// Reads Link partner ability.
>> + pub fn genphy_read_lpa(&mut self) -> Result // ...
>> +
>> + /// Reads PHY abilities.
>> + pub fn genphy_read_abilities(&mut self) -> Result // ...
>
> Do these functions need to be public? It seems like they are just used
> to update internal state, which is wrapped up into more user-facing
> API
As Andrew wrote, all genphy_ functions needs to be exported.
>> +/// An adapter for the registration of a PHY driver.
>> +pub struct Adapter<T: Driver> {
>> + _p: PhantomData<T>,
>> +}
>> +
>> +impl<T: Driver> Adapter<T> {
>> + /// Creates a new `Adapter` instance.
>> + pub const fn new() -> Adapter<T> {
>> + Self { _p: PhantomData }
>> + }
>> +
>> + unsafe extern "C" fn soft_reset_callback(
>> + phydev: *mut bindings::phy_device,
>> + ) -> core::ffi::c_int {
>> + [...]
>
> What is the motivation for having an adapter as a type? Since it's
> just a dummy struct that has no runtime meaning, I think the code
> would be easier to follow if these functions were put in a module
>
> mod phy_registration_helpers {
> unsafe extern "C" fn soft_reset_callback<T: Driver>(
> phydev: *mut bindings::phy_device,
> ) -> core::ffi::c_int { /* ... */ }
> // ...
> }
I tried to do in the similar way as rust branch.
As Benno suggested, I made Adapter struct private and
create_phy_driver() a function instead of method. Hopefully, the
changes makes the code clear.
>> + /// Creates the kernel's `phy_driver` instance.
>> + ///
>> + /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> + pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
>
> This construction looks much better than v1 :)
:)
>> +/// Corresponds to functions in `struct phy_driver`.
>> +#[vtable]
>> +pub trait Driver {
>> + /// Corresponds to `flags` in `struct phy_driver`.
>> + const FLAGS: u32 = 0;
>> + /// Corresponds to `name` in `struct phy_driver`.
>> + const NAME: &'static CStr;
>> + /// Corresponds to `phy_id` in `struct phy_driver`.
>> + const PHY_ID: u32 = 0;
>> + /// Corresponds to `phy_id_mask` in `struct phy_driver`.
>> + const PHY_ID_MASK: u32 = 0;
>
> This also seems better to me, thanks for updating. Could this just
> take a `DeviceMask`?
Surely, I think that we can use DeviceId here. Then a PHY driver code
would be more readable:
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);
>> +/// Registration structure for a PHY driver.
>> +pub struct Registration {
>> + module: &'static crate::ThisModule,
>> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
>> +}
>
> This new structure without the generics looks much better, with one note below
>
>> +impl Registration {
>> + /// Creates a new `Registration` instance.
>> + pub fn new(module: &'static crate::ThisModule) -> Self {
>> + Registration {
>> + module,
>> + drivers: None,
>> + }
>> + }
>> +
>> + /// Registers a PHY driver.
>> + pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
>> + if drivers.len() > 0 {
>> + // SAFETY: Just an FFI call.
>> + let ret = unsafe {
>> + bindings::phy_drivers_register(
>> + drivers[0].get(),
>> + drivers.len() as i32,
>> + self.module.0,
>> + )
>> + };
>> + if ret != 0 {
>> + return Err(Error::from_errno(ret));
>> + }
>> + self.drivers = Some(drivers);
>> + Ok(())
>> + } else {
>> + Err(code::EINVAL)
>> + }
>> + }
>> +}
>
> I don't think there's any need to have `new` and `register` broken
> out. This is actually a _bad_ pattern because the deregistration
> function gets called (via drop) even if it is never registered.
>
> I believe that `phy_drivers_unregister` should check against this, but
> all the same I would strongly suggest:
>
> #[must_use]
> pub fn register(
> module: &'static crate::ThisModule,
> drivers: &'static [Opaque<bindings::phy_driver>]
> ) -> Result<Self>;
>
> And no other function.
Sounds good, fixed.
> Note the `must_use` to make sure the handle is held rather than
> instantly dropped.
Nice.
>> +impl Drop for Registration {
>> + fn drop(&mut self) {
>> + if let Some(drv) = self.drivers.take() {
>> + // SAFETY: Just an FFI call.
>> + unsafe {
>> + bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32);
>> + }
>> + }
>> + }
>> +}
>> +
>
>
>> +const DEVICE_MASK_EXACT: u32 = !0;
>> +const DEVICE_MASK_MODEL: u32 = !0 << 4;
>> +const DEVICE_MASK_VENDOR: u32 = !0 << 10;
>
> See my comment above about maybe having this be an enum
With changes to Driver trait, these consts are only inside
DeviceMask. I'm not sure it worths to make them enum. Please take a
look at the next version.
>> +enum DeviceMask {
>> + Exact,
>> + Model,
>> + Vendor,
>> + Custom(u32),
>> +}
>
> LGTM
>
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver] and registers it.
>> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
>> +/// for module loading into the module binary file.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +/// device_table: [
>> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
>> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
>> +/// DeviceId::new_with_driver::<PhyAX88796B>()
>> +/// ],
>> +/// type: RustAsixPhy,
>> +/// name: "rust_asix_phy",
>> +/// author: "Rust for Linux Contributors",
>> +/// description: "Rust Asix PHYs driver",
>> +/// license: "GPL",
>> +/// }
>> +/// ```
>
> I'm glad this worked out, a single macro is nice.
>
> Since almost all devices will just use their standard DeviceId, could
> that be done by default unless a specific ID is specified? Something
> like
>
> drivers: [
> PhyAX88772A,
> // register the same phy with a second ID
> { phy: PhyAX88772A, id: some_other_device_id },
> PhyAX88772C,
> PhyAX88796B
> ],
> type: RustAsixPhy,
> // ...
>
> Though there may be a nicer looking syntax
I thought about something like that but macro would be
complicated and I can't came up with a nice syntax so I kinda prefer
to keep phy_driver and mdio_device_id loose coupling.
>> +#[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 $name:ident, [$($dev:expr),+]) => {
>> + ::kernel::macros::paste! {
>> + #[no_mangle]
>> + static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
>> + [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
>> + }
>> + };
>> +
>> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
>> + struct Module<$modname> {
>> + _reg: kernel::net::phy::Registration,
>> + _p: core::marker::PhantomData<$modname>,
>> + }
>> +
>> + type ModuleType = Module<$modname>;
>> +
>> + $crate::prelude::module! {
>> + type: ModuleType,
>> + $($f)*
>> + }
>> +
>> + static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
>> + $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
>> + ];
>> +
>> + impl kernel::Module for Module<$modname> {
>> + fn init(module: &'static ThisModule) -> Result<Self> {
>> + let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
>> + // SAFETY: static `DRIVERS` array is used only in the C side.
>> + unsafe {
>> + reg.register(&DRIVERS)?;
>> + }
>> +
>> + Ok(Module {
>> + _reg: reg,
>> + _p: core::marker::PhantomData,
>> + })
>> + }
>> + }
>> +
>> + $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
>> + }
>> +}
>
> These have to be hand formatted until rustfmt is able to handle macro
> bodies, just try to make it look a bit more like code
>
> (@device_table $name:ident, [$($dev:expr),+]) => {
> ::kernel::macros::paste! {
> #[no_mangle]
> static [<__mod_mdio__ $name _device_table>]: [
> kernel::bindings::mdio_device_id;
> $crate::module_phy_driver!(@count_devices $($dev),+) + 1
> ] = [
> $(kernel::bindings::mdio_device_id {
> phy_id: $dev.id,
> phy_id_mask: $dev.mask_as_int()
> }),+
I fixed the format but got the following error without ',' here.
error: expected one of `,`, `.`, `?`, `]`, or an operator, found
`kernel`
--> drivers/net/phy/ax88796b_rust.rs:9:1
|
9 | / kernel::module_phy_driver! {
10 | | drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
11 | | device_table: [
12 | | DeviceId::new_with_driver::<PhyAX88772A>(),
... |
20 | | license: "GPL",
21 | | }
| | ^
| | |
| | expected one of `,`, `.`, `?`, `]`, or an operator
| |_unexpected token
| help: missing `,`
> kernel::bindings::mdio_device_id {
> phy_id: 0,
> phy_id_mask: 0
> }
> ];
> }
> };
>
> and
>
> static mut DRIVERS: [
> kernel::types::Opaque<kernel::bindings::phy_driver>;
> $crate::module_phy_driver!(@count_devices $($driver),+)
> ] = [
> $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+
> ];
>
> Note also that `$( ... ),+,` expands the same as `$( ... ),+` so the
> second is a bit cleaner without the redundant `,` (at least in my
> quick test this worked)
>> --
>> 2.34.1
>>
>
> Thanks for all the updates. Overall the usability is going in the
> right direction since v1
Thanks a lot for reviewing!
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-24 6:49 ` [RFC PATCH v2 1/3] rust: core " FUJITA Tomonori
` (4 preceding siblings ...)
2023-09-26 6:05 ` Trevor Gross
@ 2023-09-26 6:54 ` Trevor Gross
2023-09-27 3:39 ` FUJITA Tomonori
5 siblings, 1 reply; 47+ messages in thread
From: Trevor Gross @ 2023-09-26 6:54 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, andrew, miguel.ojeda.sandonis
On 9/24/23, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 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)]
> + // 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() }
> + })
I think we may actually want to avoid using
`const_maybe_uninit_zeroed` and other features that indirectly rely on
`const_mut_refs`, just because some of the internals are still TBD.
That means that the best solution is to create a `const
DEFAULT_DRIVER: bindings::phy_driver` and use that to fill the
remaining fields.
Which really isn't all that bad since it means that if any fields are
ever added to `phy_driver`, we will need to confirm that our default
is OK a single place.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-26 6:54 ` Trevor Gross
@ 2023-09-27 3:39 ` FUJITA Tomonori
2023-09-27 12:21 ` Andrew Lunn
0 siblings, 1 reply; 47+ messages in thread
From: FUJITA Tomonori @ 2023-09-27 3:39 UTC (permalink / raw)
To: tmgross; +Cc: fujita.tomonori, rust-for-linux, andrew, miguel.ojeda.sandonis
On Tue, 26 Sep 2023 02:54:21 -0400
Trevor Gross <tmgross@umich.edu> wrote:
> On 9/24/23, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> 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)]
>
>
>> + // 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() }
>> + })
>
> I think we may actually want to avoid using
> `const_maybe_uninit_zeroed` and other features that indirectly rely on
> `const_mut_refs`, just because some of the internals are still TBD.
> That means that the best solution is to create a `const
> DEFAULT_DRIVER: bindings::phy_driver` and use that to fill the
> remaining fields.
>
> Which really isn't all that bad since it means that if any fields are
> ever added to `phy_driver`, we will need to confirm that our default
> is OK a single place.
Such change to phy_driver would unlikely happen? Wth such change, all
the PHY drivers need to be updated.
const_maybe_uninit_zeroed was suggested during the review of my
bindings patches for NICs. If there is a consensus not to use
const_maybe_uninit_zeroed, I'll initialize all fields by hand.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers
2023-09-27 3:39 ` FUJITA Tomonori
@ 2023-09-27 12:21 ` Andrew Lunn
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-09-27 12:21 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: tmgross, rust-for-linux, miguel.ojeda.sandonis
> > Which really isn't all that bad since it means that if any fields are
> > ever added to `phy_driver`, we will need to confirm that our default
> > is OK a single place.
>
> Such change to phy_driver would unlikely happen? Wth such change, all
> the PHY drivers need to be updated.
We have around 60 PHY driver files with around 250 phy_driver
structures. We are not going to add new members which will force us to
edit them all. It is very unlikely that new members will every be
compulsory. It simply does not scale.
Andrew
^ permalink raw reply [flat|nested] 47+ messages in thread