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: Sat, 21 Oct 2023 14:47:04 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Benno Lossin <benno.lossin@...ton.me>, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, 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

On Fri, Oct 20, 2023 at 8:42 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> 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.

I don't see how exposing `phy_device` in the documentation (note: not
the implementation) helps with that. If someone has to add things to
the abstraction, then at that point they need to be reading the
implementation of the abstraction, and thus they should read the
comments.

That is why the helpers should in general not be mentioned in the
documentation, i.e. a Rust API user should not care / need to know
about them.

If we mix things up in the docs, then it actually becomes harder later
on to properly split it; and in the Rust side we want to maintain this
 "API documentation" vs. "implementation comments" split. Thus it is
important to do it right in the first examples we will have in-tree.

And if an API is not abstracted yet, it should not be documented. APIs
and their docs should be added together, in the same patch, wherever
possible. Of course, implementation comments are different, and
possibly a designer of an abstraction may establish some rules or
guidelines for future APIs added -- that is fine, but if the user does
not need to know, it should not be in the docs, even if it is added
early.

Regarding this, part of the `phy` module documentation (i.e. the three
paragraphs) in this patch currently sounds more like an implementation
comment to me. It should probably be rewritten/split properly in docs
vs. comments.

> 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

Agreed.

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

For the getters, there is
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter.

For "actual actions" that are non-trivial, starting with a prefix that
is not `set_*` would probably be ideal, i.e. things like read, load,
push, init, register, attach, resolve, link, lock, add, create,
find...

Cheers,
Miguel