On Sat, Oct 7, 2023 at 11:35 AM Andrew Lunn [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

+        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