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

Trevor Gross
2023-10-08T07:11:50Z

On Sat, Oct 7, 2023 at 8:10 AM FUJITA Tomonori
<fujita.tomonori@gmail.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?