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

[Andrew Lunn <andrew@lunn.ch>]

On Sat, Oct 07, 2023 at 03:19:20AM -0400, Trevor Gross 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?
> 
> > +    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.

Naming is hard, and i had the exact opposite understanding.

The rust binding seems to impose getter/setters on members of
phydev. So my opinion was, using get_/set_ makes it clear this is just
a dumb getter/setter, and nothing more.

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

You have to be very careful with reading the link state. It is latched
low. Meaning if the link is dropped and then comes back again, the
first read of the link will tell you it went away, and the second read
will give you current status. The core expects the driver to read the
link state only once, when asked what is the state of the link, so it
gets informed about this short link down events.

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

This beings us back to how do you make use of C #defines. All the
values defined here are theoretically valid:

https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1887

#define SPEED_10		10
#define SPEED_100		100
#define SPEED_1000		1000
#define SPEED_2500		2500
#define SPEED_5000		5000
#define SPEED_10000		10000
#define SPEED_14000		14000
#define SPEED_20000		20000
#define SPEED_25000		25000
#define SPEED_40000		40000
#define SPEED_50000		50000
#define SPEED_56000		56000
#define SPEED_100000		100000
#define SPEED_200000		200000
#define SPEED_400000		400000
#define SPEED_800000		800000

and more speeds keep getting added.

Also, the kAPI actually would allow the value 42, not that any
hardware i know of actually supports that.

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

You could do a phydev_err(). But if these fail, the hardware is dead,
and there is not much you can do about that.

    Andrew