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

Trevor Gross
2023-10-08T07:17:49Z

On Sat, Oct 7, 2023 at 11:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > 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.

Thanks for the clarification, I agree that it makes sense as-is then.

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

The enum doesn't make sense but maybe we should just generate bindings
for these defines? I suggested that in my reply to Fujita.

- Trevor