Date: Wed, 25 Oct 2023 07:24:00 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers

On 25.10.23 03:10, FUJITA Tomonori wrote:
> On Tue, 24 Oct 2023 16:23:20 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
> 
>> On 24.10.23 02:58, FUJITA Tomonori wrote:
>>> This patch adds abstractions to implement network PHY drivers; the
>>> driver registration and bindings for some of callback functions in
>>> struct phy_driver and many genphy_ functions.
>>>
>>> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
>>>
>>> This patch enables unstable const_maybe_uninit_zeroed feature for
>>> kernel crate to enable unsafe code to handle a constant value with
>>> uninitialized data. With the feature, the abstractions can initialize
>>> a phy_driver structure with zero easily; instead of initializing all
>>> the members by hand. It's supposed to be stable in the not so distant
>>> future.
>>>
>>> Link: https://github.com/rust-lang/rust/pull/116218
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>>
>> [...]
>>
>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>>> new file mode 100644
>>> index 000000000000..2d821c2475e1
>>> --- /dev/null
>>> +++ b/rust/kernel/net/phy.rs
>>> @@ -0,0 +1,708 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>
>>> +
>>> +//! Network PHY device.
>>> +//!
>>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>>> +
>>> +use crate::{
>>> +    bindings,
>>> +    error::*,
>>> +    prelude::{vtable, Pin},
>>> +    str::CStr,
>>> +    types::Opaque,
>>> +};
>>> +use core::marker::PhantomData;
>>> +
>>> +/// PHY state machine states.
>>> +///
>>> +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state).
>>
>> Please use `rustfmt`, this line is 109 characters long.
> 
> Hmm, `make rustfmt` doesn't warn on my env. `make rustfmtcheck` or
> `make rustdoc` doesn't.

Sorry, my bad, `rustfmt` does not format comments, so you have to do
them manually.

> What's the limit?

The limit is 100 characters.

>> Also it might make sense to use a relative link, since then it also
>> works offline (though you have to build the C docs).
> 
> /// Corresponds to the kernel's [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state).
> 
> 101 characters too long?
> 
> Then we could write:
> 
> /// PHY state machine states.
> ///
> /// Corresponds to the kernel's
> /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state).
> ///
> /// Some of PHY drivers access to the state of PHY's software state machine.

That is one way, another would be to do:

/// PHY state machine states.
///
/// Corresponds to the kernel's [`enum phy_state`].
///
/// Some of PHY drivers access to the state of PHY's software state machine.
///
/// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state

But as I noted before, then people who only build the rustdoc will not
be able to view it. I personally would prefer to have the correct link
offline, but do not know about others.

>>> +    /// Gets the current link state. It returns true if the link is up.

I just noticed this as well, here also please split the line.

>>> +    pub fn get_link(&self) -> bool {
>>> +        const LINK_IS_UP: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>>> +        phydev.link() == LINK_IS_UP
>>> +    }
>>
>> Can we please change this name? I think Tomo is waiting for Andrew to
>> give his OK. All the other getter functions already follow the Rust
>> naming convention, so this one should as well. I think using
>> `is_link_up` would be ideal, since `link()` reads a bit weird in code:
>>
>>       if dev.link() {
>>           // ...
>>       }
>>
>> vs
>>
>>       if dev.is_link_up() {
>>           // ...
>>       }
> 
> I'll go with is_link_up()
> 
> 
>>> +    /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled.
>>
>> Move the second sentence into a new line, it should not be part of the
>> one-line summary.
> 
> Oops, make one-line?
> 
> /// Gets the current auto-negotiation configuration and returns true if auto-negotiation is enabled.
> 
> Or
> 
> /// Gets the current auto-negotiation configuration.
> ///
> /// It returns true if auto-negotiation is enabled.

I would prefer this, since the function name itself already is
pretty self-explanatory. If someone really wants to understand
it, they probably have to read the source code.

-- 
Cheers,
Benno

Powered by blists - more mailing lists