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

Andrew Lunn
2023-10-07T15:35:12Z

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