Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Mon, 9 Oct 2023 14:59:19 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Hi Tomonori,
> 
> A few nits I noticed. Please note that this is not really a full
> review, and that I recommend that other people like Wedson should take
> a look again and OK these abstractions before this is merged.
> 
> On Mon, Oct 9, 2023 at 3:41 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +config RUST_PHYLIB_BINDINGS
> 
> This should be called ABSTRACTIONS. Please see:

Fixed.

>     https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings
> 
> Also, could this symbol go elsewhere?

This symbol is used by the third patch. Where do you want this? 


>> +        bool "PHYLIB bindings support"
> 
> Ditto.

Updated.


>> +          a wrapper around the C phlib core.
> 
> Typo.

Oops, sorry.


>> +#![feature(const_maybe_uninit_zeroed)]
> 
> The patch message should justify this addition and warn about it.

I added the following to the commit log.

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.


>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> new file mode 100644
>> index 000000000000..f31983bf0460
>> --- /dev/null
>> +++ b/rust/kernel/net/phy.rs
>> @@ -0,0 +1,733 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Newline missing.

Added.


>> +    /// Full-duplex mode
> 
> Please use the style of the rest of the Rust comments.

I'm not sure what the style should be but something like the
following?

/// Represents duplex mode.
pub enum DuplexMode {
    /// PHY is in full-duplex mode.
    Full,

>> +/// An instance of a PHY device.
>> +/// Wraps the kernel's `struct phy_device`.
> 
> That should be separated.

Added.


>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> 
> Missing Markdown around the lifetime.

Fixed.


>> +        // FIXME: enum-cast
> 
> Please explain what needs to be fixed.

Added.


>> +    /// Executes software reset the PHY via BMCR_RESET bit.
> 
> Markdown missing (multiple instances).

Can you elaborate?


>> +    /// Reads Link partner ability.
> 
> Why is "link" capitalized here?

Fixed.


>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> 
> Broken formatting? Does `rustdoc` complain about it?

Yes, sorry about that.


>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
> 
> Perhaps "The `drivers` field"?

I replaced this with the following comment suggested by Benno.

/// All elements of the `drivers` slice are valid and currently registered
/// to the kernel via `phy_drivers_register`.


>> +            // SAFETY: The type invariants guarantee that self.drivers is valid.
> 
> Markdown.

Fixed.


>> +/// Represents the kernel's `struct mdio_device_id`.
>> +pub struct DeviceId {
>> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
>> +    pub id: u32,
>> +    mask: DeviceMask,
>> +}
> 
> It would be nice to explain why the field is `pub`.

Added.


>> +    /// Get a mask as u32.
> 
> Markdown.

Fixed.


> This patch could be split a bit too, but that is up to the maintainers.

Yeah.


>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver` and registers it.
> 
> "kernel's" or similar

Added.


>> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> 
> Markdown.

Fixed.

>> +/// for module loading into the module binary file. Every driver needs an entry in device_table.
> 
> Markdown.

Fixed.


>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +///     device_table: [
>> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
>> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
>> +///         DeviceId::new_with_driver::<PhyAX88796B>()
>> +///     ],
>> +///     name: "rust_asix_phy",
>> +///     author: "Rust for Linux Contributors",
>> +///     description: "Rust Asix PHYs driver",
>> +///     license: "GPL",
>> +/// }
>> +/// ```
> 
> Please add an example above with the expansion of the macro so that it
> is easy to understand at a glance, see e.g. what Benno did in
> `pin-init` (`rust/init*`).

Added.

Thanks a lot!