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: <CALNs47sxuGVXBwhXZa5NgHQ8F0MH2OoUzsgthAURE+OuTtJ1wQ@mail.gmail.com>
Date: Sun, 8 Oct 2023 03:11:36 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, 
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	greg@...ah.com
Subject: Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

On Sat, Oct 7, 2023 at 8:10 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
> >> +    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/

Thanks for the link, in that case LGTM

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

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