netdev - Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

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: Fri, 20 Oct 2023 20:42:10 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
	tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com,
	greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY
 drivers

> > +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
> > +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
> > +//! a hardware register and updates the stats.
> 
> I would use [`Device`] instead of `phy_device`, since the Rust reader
> might not be aware what wraps `phy_device`.

We don't want to hide phy_device too much, since at the moment, the
abstraction is very minimal. Anybody writing a driver is going to need
a good understanding of the C code in order to find the helpers they
need, and then add them to the abstraction. So i would say we need to
explain the relationship between the C structure and the Rust
structure, to aid developers.

> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&self) -> bool {
> 
> I still think this name should be changed. My response at [1] has not yet
> been replied to. This has already been discussed before:
> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
> - https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/
> - https://lore.kernel.org/rust-for-linux/CALNs47syMxiZBUwKLk3vKxzmCbX0FS5A37FjwUzZO9Fn-iPaoA@mail.gmail.com/
> 
> And I want to suggest to change it to `is_link_up`.
> 
> Reasons why I do not like the name:
> - `get_`/`put_` are used for ref counting on the C side, I would like to
>    avoid confusion.
> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>    such as a hashmap, so I expect the call to do some computation.
> - getters in Rust usually are not prefixed with `get_`, but rather are
>    just the name of the field.
> - in this case I like the name `is_link_up` much better, since code becomes
>    a lot more readable with that.
> - I do not want this pattern as an example for other drivers.
> 
> [1]: https://lore.kernel.org/rust-for-linux/f5878806-5ba2-d932-858d-dda3f55ceb67@proton.me/
> 
> > +        const LINK_IS_UP: u32 = 1;
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        let phydev = unsafe { *self.0.get() };
> > +        phydev.link() == LINK_IS_UP
> > +    }

During the reviews we have had a lot of misunderstanding what this
actually does, given its name. Some thought it poked around in
registers to get the current state of the link. Some thought it
triggered the PHY to establish a link. When in fact its just a dumb
getter. And we have a few other dumb getters and setters.

So i would prefer something which indicates its a dumb getter. If the
norm of Rust is just the field name, lets just use the field name. But
we should do that for all the getters and setters. Is there a naming
convention for things which take real actions?

And maybe we need to add a comment: Get the current link state, as
stored in the [`Device`]. Set the duplex value in [`Device`], etc.

   Andrew