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: Sun, 22 Oct 2023 17:34:04 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
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 01:37:33PM +0200, Miguel Ojeda wrote:
> On Sun, Oct 22, 2023 at 11:47 AM FUJITA Tomonori
> <fujita.tomonori@...il.com> wrote:
> >
> > Agreed that the first three paragraphs at the top of the file are
> > implementation comments. Are there any other comments in the file,
> > which look implementation comments to you? To me, the rest look the
> > docs for Rust API users.
> 
> I think some should be improved with that in mind, yeah. For instance,
> this one seems good to me:
> 
>     /// An instance of a PHY driver.
> 
> But this one is not:
> 
>     /// Creates the kernel's `phy_driver` instance.
> 
> It is especially bad because the first line of the docs is the "short
> description" used for lists by `rustdoc`.
> 
> For similar reasons, this one is bad (and in this case it is the only line!):
> 
>     /// Corresponds to the kernel's `enum phy_state`.
> 
> That line could be part of the documentation if you think it is
> helpful for a reader as a practical note explaining what it is
> supposed to map in the C side. But it should really not be the very
> first line / short description.
> 
> 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
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, 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.

	Andrew