netdev - Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALNs47syMxiZBUwKLk3vKxzmCbX0FS5A37FjwUzZO9Fn-iPaoA@mail.gmail.com>
Date: Sat, 7 Oct 2023 03:19:20 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	miguel.ojeda.sandonis@...il.com, greg@...ah.com
Subject: Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:

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

> +    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.

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.

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.

> +        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;
    }

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

> +        dev.genphy_read_lpa()?;

Same question as with the `genphy_update_link`

> +    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.

---

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

- Trevor

[1]: https://lore.kernel.org/rust-for-linux/baa4cc4b-bcde-4b02-9286-c923404db972@lunn.ch/