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

Alice Ryhl
2023-11-17T09:39:09Z

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 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@gmail.com>

In this reply, I go through my minor nits:

> +use crate::{
> + prelude::{vtable, Pin},
> +};

Normally, if you're importing specific prelude items by name instead of
using prelude::*, then you would import them using their non-prelude
path.

> +#[derive(PartialEq)]
> +pub enum DeviceState {

If you add PartialEq and you can add Eq, then also add Eq.

> +/// An adapter for the registration of a PHY driver.
> +struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}

You don't need this struct. The methods can be top-level functions.

But I know that others have used the same style of defining a useless
struct, so it's fine with me.

> + /// Defines certain other features this PHY supports.
> + /// It is a combination of the flags in the [`flags`] module.
> + const FLAGS: u32 = 0;

You need an empty line between the two lines if you intend for them to
be separate lines in rendered documentation.

> +#[vtable]
> +pub trait Driver {
> + /// Issues a PHY software reset.
> + fn soft_reset(_dev: &mut Device) -> Result {
> + Err(code::ENOTSUPP)
> + }
> [...]
> +}

I believe that the guidance for what to put in optional vtable-trait
methods was changed in:

https://lore.kernel.org/all/20231026201855.1497680-1-benno.lossin@proton.me/

> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}

I would change this to "it's okay to call phy_drivers_unregister from a
different thread than the one in which phy_drivers_register was called".

> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}

Here, you can say "Registration has no &self methods, so immutable
references to it are useless".

> + // macro use only
> + #[doc(hidden)]
> + pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
> + bindings::mdio_device_id {
> + phy_id: self.id,
> + phy_id_mask: self.mask.as_int(),
> + }
> + }

This is fine, but I probably would just expose it for everyone. It's not
like it hurts if non-macro code can call this method, even if it doesn't
have a reason to do so.

Alice