On 12/5/23 02:14, FUJITA Tomonori wrote:

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