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
| ||
|
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?
Powered by blists - more mailing lists