On Sun, 8 Oct 2023 02:07:44 -0400
Trevor Gross [off-list ref] wrote:

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

quoted

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

Device? I don't think so. If we make Device pub(crate), we need to
make trait Driver pub(crate) too.

quoted

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

Ok. As I commented on the RFC reviewing, I don't think that we
need try_from conversion for values from PHYLIB. Implementing bindings
for untrusted stuff doesn't make sense.

https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/ (local)

On the other hand, I think that it might worth to use try_from for
set_speed() because its about the bindings and Rust PHY drivers.
However, I leave it alone since likely setting a wrong value doesn't
break anything.