On Sat, Oct 7, 2023 at 6:59 AM FUJITA Tomonori
[off-list ref] wrote:

quoted

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

Seems good to me. I know that largely we want users to refer to the C
docs, but I think a small hint is good.

Fwiw they can be on the same line, Markdown combines them

quoted

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.

This was in follow up to one of the notes on the RFC patches, I'll
reply more to Andrew's comment about this

quoted

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.

Nevermind, I guess we don't have `warn_once`. Andrew mentioned
something about potentially lossy conversions, I'll follow up there.

quoted

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.

Thanks, no worries in that case

quoted

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.

Miguel mentioned on Zulip that we try to avoid macros (I did not know
this), so I guess it's fine if redundant.

https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.60bool.3A.3Athen.60.20helper/near/395398830

quoted

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.

?

I think something like "Every driver needs an entry in device_table"
is fine, just make it less easy to miss

Thanks for the followup, all of these were very minor