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]

Message-Id: <20231024.103732.1242900920044504585.fujita.tomonori@gmail.com>
Date: Tue, 24 Oct 2023 10:37:32 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: miguel.ojeda.sandonis@...il.com, 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, 22 Oct 2023 17:34:04 +0200
Andrew Lunn <andrew@...n.ch> wrote:

> 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

I added links to the kdoc like:

/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state).

But the first line needs to a short description so I copy the C
description:

/// PHY state machine states.

I revised all the comments.