On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn [off-list ref] wrote:

quoted

The safety comment here still needs something like

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

I'm not sure that really adds much useful information. Which values
are protected by the lock? More importantly, which are not protected
by the lock?

As a general rule of thumb, driver writers don't understand
locking. Yes, there are some which do, but many don't. So the
workaround to that is make it so they don't need to understand
locking. All the locking happens in the core.

The exception is suspend and resume, which are called without the
lock. So if i was to add a comment about locking, i would only put a
comment on those two.

This doesn't get used by driver implementations, it's only used within
the abstractions here. I think anyone who needs the details can refer
to the C side, I just suggested to note the locking caveat based on
your second comment at
https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/ (local)

Fujita - since this doesn't get exposed, could this be pub(crate)?)

quoted

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?

phy_init_hw can be called multiple times. It used by drivers as a work
around to broken hardware/firmware to get the device back into a good
state. It is also used during resume, since often the PHY looses its
settings when suspended.

Great, thank you for the clarification

quoted

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

I would actually say this is the wrong place to do that. Such checks
should happen in the core, so it checks all drivers, not just the
current one Rust driver. Feel free to submit a C patch adding this.

        Andrew

I guess it does that already:
https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556

Fujita, I think we started doing comments when we know that
lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
but add

    // CAST: the C side verifies devnum < 32

?