Message-ID: <CANiq72mDWJDb9Fhd4CHt8YKapdWaOrqhJMOrQZ9CDRtvNdrGqA@mail.gmail.com>
Date: Sun, 22 Oct 2023 13:37:33 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: andrew@...n.ch, 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 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 ......"
as the short description. Then ideally a longer explanation of why it
is needed, how it is intended to be used, what this maps to in the C
side (if relevant), anything else that the user may need to know about
it, particular subtleties if any, examples if relevant, etc.

> I'm not sure that a comment on the relationship between C and Rust
> structures like "Wraps the kernel's `struct phy_driver`" is API docs
> but the in-tree files like mutex.rs have the similar so I assume it's
> fine.

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.

But if some detail is not needed to use the API, then we should avoid
writing it in the documentation. And if it is needed, but it can be
written in a way that does not depend/reference the C side, then it
should be.

For instance, as you can see from the `mutex.rs` you mention, the
short description does not mention the C side -- it does so
afterwards, and then it goes onto explaining why it is needed, how it
is used (with examples), and so on. The fact that it exposes the C
`struct mutex` is there, because it adds value ("oh, ok, so this is
what I would use if I wanted the C mutex"), but that bit (and the
rest) is not really about explaining how `Mutex` is implemented:

    /// A mutual exclusion primitive.
    ///
    /// Exposes the kernel's [`struct mutex`]. When multiple threads
attempt to lock the same mutex,
    /// only one at a time is allowed to progress, the others will
block (sleep) until the mutex is
    /// unlocked, at which point another thread will be allowed to
wake up and make progress.
    ///
    /// Since it may block, [`Mutex`] needs to be used with care in
atomic contexts.
    ///
    /// Instances of [`Mutex`] need a lock class and to be pinned. The
recommended way to create such
    /// instances is with the [`pin_init`](crate::pin_init) and
[`new_mutex`] macros.
    ///
    /// # Examples
    ///
    /// The following example shows how to declare, allocate and
initialise a struct (`Example`) that
    /// contains an inner struct (`Inner`) that is protected by a mutex.

    ...

    /// The following example shows how to use interior mutability to
modify the contents of a struct
    /// protected by a mutex despite only having a shared reference:

    ...

`Mutex`'s docs are, in fact, a good a good example of how to write docs!

> Where the implementation comments are supposed to be placed?
> Documentation/networking?

No, they would be normal `//` comments and they should be as close to
the relevant code as possible -- please see
https://docs.kernel.org/rust/coding-guidelines.html#comments. They can
still be read in the generated docs via the "source" buttons, so they
will be there for those reading the actual implementation in the
browser.

Instead, `Documentation/` is detached from the actual code. For Rust
code, we hope to use only those for out-of-line information that is
not related to any particular API.

For instance, the "coding guidelines" document I just linked. Or
end-user / distributor documentation. Or, as another example, Wedson
at some point considered adding some high-level design documents, and
if those would not fit any particular API or if they are not intended
for users of the API, they could perhaps go into `Doc/`. Or perhaps
Boqun's idea of having a reviewers guide, etc.

Cheers,
Miguel

Powered by blists - more mailing lists