On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
[off-list ref] wrote:

+/// 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>);

Can you just add `An instance of a PHY` to the docs for reference?

+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.cast() }
+    }

The safety comment here still needs something like

    with the exception of fields that are synchronized via the `lock` mutex

+    /// Gets the id of the PHY.
+    pub fn phy_id(&mut self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Gets the state of the PHY.
+    pub fn state(&mut self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let state = unsafe { (*phydev).state };
+        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,
+        }
+    }

Could you add a comment like `// FIXME:enum-cast` or something? Then
when we have a better solution for enums handling we can revise this.

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

Since we're taking user input, it probably doesn't hurt to do some
sort of sanity check rather than casting. Maybe warn once then return
the biggest nowrapping value

    let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
        warn_once!("excessive speed {speed}");
        i32::MAX
    })
    unsafe { (*phydev).speed = speed_i32 };

+    /// Executes software reset the PHY via BMCR_RESET bit.
+    pub fn genphy_soft_reset(&mut self) -> Result {
+        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::genphy_soft_reset(phydev) })
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        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::phy_init_hw(phydev) })
+    }

Andrew, are there any restrictions about calling phy_init_hw more than
once? Or are there certain things that you are not allowed to do until
you call that function?

If so, maybe a simple typestate would make sense here

+impl<T: Driver> Adapter<T> {
+    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)
+        })
+    }

All of these functions need a `# Safety` doc section, you could
probably just say to follow `Device::from_raw`'s rules. And then you
can update the comments to say caller guarantees preconditions

If you care to, these functions are so similar that you could just use
a macro to make your life easier

    macro_rules! make_phydev_callback{
        ($fn_name:ident, $c_fn_name:ident) => {
            /// ....
            /// # Safety
            /// `phydev` must be valid and registered
            unsafe extern "C" fn $fn_name(
                phydev: *mut ::bindings::phy_device
            ) -> $ret_ty {
                from_result(|| {
                    // SAFETY: Preconditions ensure `phydev` is valid and
                    let dev = unsafe { Device::from_raw(phydev) };
                    T::$c_fn_name(dev)?;
                    Ok(0)
                }
            }
        }
    }

    make_phydev_callback!(get_features_callback, get_features);
    make_phydev_callback!(suspend_callback, suspend);

+    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())
+        })
+    }

Since your're reading a bus, it probably doesn't hurt to do a quick
check when converting

    let devnum_u8 = u8::try_from(devnum).(|_| {
        warn_once!("devnum {devnum} exceeds u8 limits");
        code::EINVAL
    })?
    // ...

+    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)
+        })
+    }

Same as above with the conversion errors

+/// 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<T: Driver>() -> Opaque<bindings::phy_driver> {
+    Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr() as *mut i8,

`.cast_mut()`, just makes the mutability change more clear

I guess the C side could technically be `const char *name`

+        // 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() }
+    })
+}

Btw I double checked and this should be OK to use, hopefully will be
stable in the near future
https://github.com/rust-lang/rust/pull/116218

+/// 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.

Could you add information about the relationship between drivers and
device_table?

+/// # 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 can't find the discussion we had about this, but you said you have
the `type` parameter to be consistent with `module!`, correct?

I think that it is more important to be consistent with C's
`MODULE_PHY_DRIVER` where you don't need to specify anything extra,
since the module doesn't do anything else. And I think it is less
confusing for users if they don't wonder why they need to define a
type they never use.

Why not just remove the field and create an internal type based on
`name` for now? We can always make it an optional field later on if it
turns out there is a use case.

- Trevor