Message-ID: <9d38d6f9-3b54-4a6f-a19d-3710db171fed@proton.me>
Date: Thu, 07 Dec 2023 17:25:22 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org
Cc: rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, aliceryhl@...gle.com, boqun.feng@...il.com
Subject: Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY drivers

On 12/5/23 02:14, FUJITA Tomonori wrote:
> @@ -0,0 +1,754 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
> +
> +use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
> +
> +use core::marker::PhantomData;
> +
> +/// PHY state machine states.
> +///
> +/// Corresponds to the kernel's [`enum phy_state`].
> +///
> +/// Some of PHY drivers access to the state of PHY's software state machine.

This sentence reads a bit weird, what are you trying to say?

> +///
> +/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
> +#[derive(PartialEq, Eq)]
> +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,

I took a look at `enum phy_state` and found that you only copied the
first sentence of each state description, why is that?

> +}
> +
> +/// A mode of Ethernet communication.
> +///
> +/// PHY drivers get duplex information from hardware and update the current state.

Are you trying to say that the driver automatically queries the
hardware? You could express this more clearly.

> +pub enum DuplexMode {
> +    /// PHY is in full-duplex mode.
> +    Full,
> +    /// PHY is in half-duplex mode.
> +    Half,
> +    /// PHY is in unknown duplex mode.
> +    Unknown,
> +}
> +
> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's [`struct phy_device`].
> +///
> +/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
> +/// executes [`Driver`]'s methods during the callback.
> +///
> +/// # Invariants
> +///
> +/// Referencing a `phy_device` using this struct asserts that you are in
> +/// a context where all methods defined on this struct are safe to call.

I know that Alice suggested this, but I reading it now, it sounds a
bit weird. When reading this it sounds like a requirement for everyone
using a `Device`. It would be better to phrase it so that it sounds like
something that users of `Device` can rely upon.

Also, I would prefer for this invariant to be a simple one, for example:
"The mutex of `self.0` is held".
The only problem with that are the `resume` and `suspend` methods.
Andrew mentioned that there is some tribal knowledge on this topic, but
I don't see this written down anywhere here. I can't really suggest an
improvement to invariant without knowing the whole picture.

> +/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
> +// to the instance.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
> +    /// and the caller must be in a context where all methods defined on this struct
> +    /// are safe to call.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
> +        // the duration of `'a`.
> +        unsafe { &mut *ptr }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn phy_id(&self) -> u32 {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.

At the moment the invariant only states that "all functions on
`Device` are safe to call". It does not say anything about accessing
fields. I hope this shows why I think the invariant is problematic.

-- 
Cheers,
Benno

Powered by blists - more mailing lists