Message-ID: <85d5c498-efbc-4c1a-8d12-f1eca63c45cf@proton.me>
Date: Fri, 13 Oct 2023 21:31:16 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org
Cc: rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY drivers

On 12.10.23 14:53, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>   init/Kconfig                    |   8 +
>   rust/Makefile                   |   1 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 679 ++++++++++++++++++++++++++++++++
>   6 files changed, 700 insertions(+)
>   create mode 100644 rust/kernel/net.rs
>   create mode 100644 rust/kernel/net/phy.rs
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 6d35728b94b2..0fc6f5568748 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1903,6 +1903,14 @@ config RUST
> 
>   	  If unsure, say N.
> 
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "PHYLIB abstractions support"
> +        depends on RUST
> +        depends on PHYLIB=y
> +        help
> +          Adds support needed for PHY drivers written in Rust. It provides
> +          a wrapper around the C phylib core.
> +

I find it a bit weird that this is its own option under "General". I think
it would be reasonable to put it under "Rust", since that would also scale
better when other subsystems do this.

>   config RUSTC_VERSION_TEXT
>   	string
>   	depends on RUST
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
>         cmd_bindgen = \
>   	$(BINDGEN) $< $(bindgen_target_flags) \
>   		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> +		--rustified-enum phy_state\

Please change this to Miguel's solution.

>   		--no-debug '.*' \
>   		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
>   		$(bindgen_target_cflags) $(bindgen_target_extra)

[...]

> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function can be called only in the callbacks in `phy_driver`. PHYLIB guarantees

"can be called in" -> "must only be called from"

> +    /// the exclusive access for the duration of the lifetime `'a`.

In some other thread you mentioned that no lock is held for
`resume`/`suspend`, how does this interact with it?

> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> +        // `Device` type being transparent makes the cast ok.
> +        unsafe { &mut *ptr.cast() }

please refactor to

     // CAST: ...
     let ptr = ptr.cast::<Self>();
     // SAFETY: ...
     unsafe { &mut *ptr }

> +    }

[...]

> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&self, speed: u32) {

This function modifies state, but is `&self`?

> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let mut phydev = unsafe { *self.0.get() };
> +        phydev.speed = speed as i32;
> +    }
> +
> +    /// Sets duplex mode.
> +    pub fn set_duplex(&self, mode: DuplexMode) {

This function modifies state, but is `&self`?

> +        let v = match mode {
> +            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
> +            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
> +            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
> +        };
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let mut phydev = unsafe { *self.0.get() };
> +        phydev.duplex = v;

Note that this piece of code will actually not do the correct thing. It
will create a copy of `phydev` on the stack and modify that instead of the
pointee of `self`. I think the code was fine before this change.

> +    }
> +
> +    /// Reads a given C22 PHY register.
> +    pub fn read(&self, regnum: u16) -> Result<u16> {

No idea if this function should be `&mut self` or `&self`. Would
it be ok for mutltiple threads to call this function concurrently?
If yes, then leave it as `&self`, if no then change it to `&mut self`.

> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe {
> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Writes a given C22 PHY register.
> +    pub fn write(&self, regnum: u16, val: u16) -> Result {

This should probably be `&mut self`, but not sure.

> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe {
> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> +        })
> +    }
> +
> +    /// Reads a paged register.
> +    pub fn read_paged(&self, page: u16, regnum: u16) -> Result<u16> {

Again same question (also for all other functions below that call the C
side).

> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }

[...]

> +}
> +
> +/// Defines certain other features this PHY supports (like interrupts).

Maybe add a link where these flags can be used.

> +pub mod flags {
> +    /// PHY is internal.
> +    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
> +    /// PHY needs to be reset after the refclk is enabled.
> +    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
> +    /// Polling is used to detect PHY status changes.
> +    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
> +    /// Don't suspend.
> +    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
> +}

[...]

> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[vtable]
> +pub trait Driver {
> +    /// Defines certain other features this PHY supports.
> +    /// It is a combination of the flags in the [`flags`] module.
> +    const FLAGS: u32 = 0;

What would happen if I set this to some value that is not a combination of
the flag values above? I expect that bits that are not part of the flag
values above to be ignored.

> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;
> +
> +    /// This driver only works for PHYs with IDs which match this field.

Mention that the default value is 0.

> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);

[...]

> +}
> +
> +/// Registration structure for a PHY driver.
> +///
> +/// # Invariants
> +///
> +/// All elements of the `drivers` slice are valid and currently registered
> +/// to the kernel via `phy_drivers_register`.

Since `DriverType` is now safe a wrapper type, this invariant should be
moved to that type instead.

> +pub struct Registration {
> +    drivers: &'static [DriverType],
> +}
> +
> +impl Registration {
> +    /// Registers a PHY driver.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The values of the `drivers` array must be initialized properly.

With the above change you do not need this (since all instances of
`DriverType` are always initialized). But I am not sure if it would be
fine to call `phy_driver_register` multiple times with the same driver
without unregistering it first.

I thought about this implementation of `Registration` a bit and I think
there are two possible ways to avoid both this `unsafe` function and the
mutable static in the `module_phy_driver` macro from patch 2:

Option 1:
- make the constructor of `DriverType` register the driver and the `Drop`
   impl unregister the driver.
- remove the `Registration` type.
- in the `module_phy_driver` macro create the array of `DriverType`s inside
   of the `Module` struct.

The disadvantage of this solution is that `phy_drivers_register` is called
for every driver (and also `phy_drivers_unregister`). But if you and Andrew
think that that is fine, then go with this option. The other advantage of
this solution is that it also becomes safely usable without the
`module_phy_driver` macro. For exmaple when a more complex `Module`
struct is needed by a driver.

Option 2:
- remove the `Registration` type.
- in the `module_phy_driver` macro: create the array of `DriverType`s inside
   of the `Module` struct.
- in the `module_phy_driver` macro: register the drivers in the
   `Module::init` function and unregister them in the `Drop` impl of
   `Module`.

This approach only has one call to `phy_drivers_(un)register`, but cannot
really be used safely without the `module_phy_driver` macro.

> +    pub unsafe fn register(
> +        module: &'static crate::ThisModule,
> +        drivers: &'static [DriverType],
> +    ) -> Result<Self> {
> +        if drivers.is_empty() {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: The safety requirements of the function ensure that `drivers` array are initialized properly.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe {
> +            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
> +        })?;
> +        // INVARIANT: The safety requirements of the function and the success of `phy_drivers_register` ensure
> +        // the invariants.
> +        Ok(Registration { drivers })
> +    }
> +}

[...]

> +
> +    /// Get a `mask` as u32.
> +    pub const fn mask_as_int(&self) -> u32 {
> +        self.mask.as_int()
> +    }
> +
> +    // macro use only
> +    #[doc(hidden)]
> +    pub const fn as_mdio_device_id(&self) -> bindings::mdio_device_id {

I would name this just `mdio_device_id`.

-- 
Cheers,
Benno

> +        bindings::mdio_device_id {
> +            phy_id: self.id,
> +            phy_id_mask: self.mask.as_int(),
> +        }
> +    }
> +}
> +
> +enum DeviceMask {
> +    Exact,
> +    Model,
> +    Vendor,
> +    Custom(u32),
> +}
> +
> +impl DeviceMask {
> +    const MASK_EXACT: u32 = !0;
> +    const MASK_MODEL: u32 = !0 << 4;
> +    const MASK_VENDOR: u32 = !0 << 10;
> +
> +    const fn as_int(&self) -> u32 {
> +        match self {
> +            DeviceMask::Exact => Self::MASK_EXACT,
> +            DeviceMask::Model => Self::MASK_MODEL,
> +            DeviceMask::Vendor => Self::MASK_VENDOR,
> +            DeviceMask::Custom(mask) => *mask,
> +        }
> +    }
> +}
> --
> 2.34.1
>

Powered by blists - more mailing lists