On Sat, 7 Oct 2023 03:19:20 -0400
Trevor Gross [off-list ref] wrote:

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

quoted

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
new file mode 100644
index 000000000000..d11c82a9e847
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs

Maybe want to link to the C version, just for the crossref?

Sure.

quoted

+    fn read_status(dev: &mut phy::Device) -> Result<u16> {
+        dev.genphy_update_link()?;
+        if !dev.get_link() {
+            return Ok(0);
+        }

Looking at this usage, I think `get_link()` should be renamed to just
`link()`. `get_link` makes me think that it is performing an action
like calling `genphy_update_link`, just `link()` sounds more like a
static accessor.

Andrew suggested to rename link() to get_link(), I think.

Then we discussed again last week:

https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/ (local)

Or maybe it's worth replacing `get_link` with a `get_updated_link`
that calls `genphy_update_link` and then returns `link`, the user can
store it if they need to reuse it. This seems somewhat less accident
prone than someone calling `.link()`/`.get_link()` repeatedly and
wondering why their phy isn't coming up.

Once this is merged, I'll think about APIs. I need to add more
bindings anyway.

In any case, please make the docs clear about what behavior is
executed and what the preconditions are, it should be clear what's
going to wait for the bus vs. simple field access.

Sure.

quoted

+        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
+            dev.set_speed(100);
+        } else {
+            dev.set_speed(10);
+        }

Speed should probably actually be an enum since it has defined values.
Something like

    #[non_exhaustive]
    enum Speed {
        Speed10M,
        Speed100M,
        Speed1000M,
        // 2.5G, 5G, 10G, 25G?
    }

    impl Speed {
        fn as_mb(self) -> u32;
    }

ethtool.h says:

/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
 * Update drivers/net/phy/phy.c:phy_speed_to_str() and
 * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
 */

I don't know there are drivers that set such values.

quoted

+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };

BMCR_x and MII_x are generated as `u32` but that's just a bindgen
thing. It seems we should reexport them as the correct types so users
don't need to cast all over:

    pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
    pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
    // (I'd just make a macro for this)

But I'm not sure how to handle that since the uapi crate exposes its
bindings directly. We're probably going to run into this issue with
other uapi items at some point, any thoughts Miguel?

reexporting all the BMCR_ values by hand doesn't sound fun. Can we
automaticall generate such?

quoted

+        dev.genphy_read_lpa()?;

Same question as with the `genphy_update_link`

quoted

+    fn link_change_notify(dev: &mut phy::Device) {
+        // Reset PHY, otherwise MII_LPA will provide outdated information.
+        // This issue is reproducible only with some link partner PHYs.
+        if dev.state() == phy::DeviceState::NoLink {
+            let _ = dev.init_hw();
+            let _ = dev.start_aneg();
+        }
+    }
+}

Is it worth doing anything with these errors? I know that the C driver doesn't.

I'll check out what other drivers do in the similar situations.

The overall driver looks correct to me, most of these comments are
actually about [1/3]

Thanks!