On Sat, 7 Oct 2023 01:06:04 -0400
Trevor Gross [off-list ref] wrote:

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

quoted

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

You meant something like?

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

quoted

+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

    

You meant the following?

/// 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 with the exception of fields that are
/// synchronized via the `lock` mutex.

What this means? We use Device only when an exclusive access is
gurannteed by device->lock. As discussed before, resume/suspend are
called without device->lock locked but still drivers assume an
exclusive access.

quoted

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

Added.

quoted

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

warn_once() is available? I was thinking about adding it after the PHY
patchset.

I'll change set_speed to return Result.

quoted

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

From quick look, you can call it multiple times.

If so, maybe a simple typestate would make sense here

quoted

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

Looks nice. I use the following macro.

macro_rules! make_phydev_callback {
    ($fn_name: ident) => {
        ::kernel::macros::paste! {
            /// # Safety
            ///
            /// `phydev` must be passed by callback functions in `phy_driver`.
            unsafe extern "C" fn [<$fn_name _callback>] (
                phydev: *mut bindings::phy_device
            ) -> core::ffi::c_int {
                from_result(|| {
                    // SAFETY: Preconditions ensure `phydev` is valid.
                    let dev = unsafe { Device::from_raw(phydev) };
                    T::$fn_name(dev)?;
                    Ok(0)
                })
            }
        }
    };
}

quoted

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

Sure.

quoted

+    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

quoted

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

Done.

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

quoted

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

Thanks!

quoted

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

device_table needs to have PHY Ids that one of the drivers can handle.

?

quoted

+/// # 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?

No, `driver!` in rust branch, which is used by platform, amba, etc.

https://github.com/Rust-for-Linux/linux/blob/rust/samples/rust/rust_platform.rs

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.

Sure, I'll try. I have no preference and driver! macro isn't in
upstream.