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

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Sat, 7 Oct 2023 03:19:20 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.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?

Sure.

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

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

Once this is merged, I'll think about APIs. I need to add more
bindings anyway.


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

Sure.

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


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

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

I'll check out what other drivers do in the similar situations.


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

Thanks!