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

quoted

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)

Thanks for the link, in that case LGTM

quoted

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.

Andrew replied to this too and an enum wouldn't work. Maybe good to
add uapi/linux/ethtool.h to the bindings and use the SPEED_X defined
there?

quoted

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?

Definitely not by hand, I don't think bindgen allows finer control
over what types are created from `#define` yet. I am not sure what our
policy is on build scripts but the below would work:

    # repeat this with a different prefix (BMCR) and type (u16) as needed
    perl -ne 'print if
s/^#define\s+(BMCR\w+)\s+([0-9xX]+)\s+(?:\/\*(.*)\*\/)?/\/\/\/ \3\npub
const \1: u16 = \2;/' include/uapi/linux/mii.h > somefile.rs

That creates outputs

    ///  MSB of Speed (1000)
    pub const BMCR_SPEED1000: u16 = 0x0040;
    ///  Collision test
    pub const BMCR_CTST: u16 = 0x0080;
    ///  Full duplex
    pub const BMCR_FULLDPLX: u16 = 0x0100;

Miguel, any suggestions here?