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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 8 Oct 2023 03:17:35 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, 
	greg@...ah.com
Subject: Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

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