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] [day] [month] [year] [list]

Date: Tue, 24 Oct 2023 10:48:14 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, benno.lossin@...ton.me, 
	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 Sun, Oct 22, 2023 at 5:34 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Sun, Oct 22, 2023 at 01:37:33PM +0200, Miguel Ojeda wrote:
> >
> > Instead, the documentation should answer the question "What is this?".
> > And the answer should be something like "The state of the PHY ......"
>
> Its the state of the state machine, not the state of the PHY. It is

I didn't say it was the state of the PHY -- please note the dots above.

> already documented in kernel doc, so we don't really want to duplicate
> it. So maybe just cross reference to the kdoc:
>
> https://docs.kernel.org/networking/kapi.html#c.phy_state

Yes, that can be worth it for simple 1:1 cases like the `enum`
(assuming they are properly documented in the C side), but we still
want a suitable short description (e.g. "PHY state machine states"),
like Tomonori did in the version he just sent (v6).

I wondered about the docs of each variant, too, but those should be OK
too, because `rustdoc` does not create an individual page for them, so
one can always see the link to the C docs at the top from the `enum`
description.

> > Yes, documenting that something wraps/relies on/maps a particular C
> > functionality is something we do for clarity and practicality (we also
> > link the related C headers). This is, I assume, the kind of clarity
> > Andrew was asking for, i.e. to be practical and let the user know what
> > they are dealing with on the C side, especially early on.
>
> I don't think 'early on' is relevant. In the kernel, you pretty much
> always need the bigger picture, how a pieces of the puzzle fits in
> with what is above it and what is below it. Sometimes you need to
> extend what is above and below. Or a reviewer will tell you to move
> code into the core, so others can share it, etc.

"Early on" in my reply is referring to what you said earlier, i.e.
that initially abstractions are minimal.

In any case, yes, using complex systems typically requires knowing a
bit of what is going on in different parts, but that does not mean we
cannot make self-contained documentation as much as reasonably
possible. We want that using a particular Rust abstraction does not
require reading its source code or the code in the C side.

In the example that you mention, if the reviewer wants to share code,
then it should be extracted into a new Rust abstraction (and possibly
the C core depending on what it is, of course) and using it from the
driver, but also documenting the Rust abstraction.

Cheers,
Miguel