Received-SPF: pass (google.com: domain of linux-kernel+bounces-48801-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240201-rockchip-rust-phy_depend-v2-0-c5fa4faab924@christina-quast.de>
 <20240201-rockchip-rust-phy_depend-v2-2-c5fa4faab924@christina-quast.de>
In-Reply-To: <20240201-rockchip-rust-phy_depend-v2-2-c5fa4faab924@christina-quast.de>
From: Trevor Gross <tmgross@umich.edu>
Date: Thu, 1 Feb 2024 14:02:03 -0600
Message-ID: <CALNs47tWNNi2GXHAwwT=A1LP=xWwXvrPy4xVapqMQOeyeN0+9Q@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] rust: phy: add some phy_driver and genphy_ functions
To: Christina Quast <contact@christina-quast.de>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, 
	Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, 
	=?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Benno Lossin <benno.lossin@proton.me>, Andreas Hindborg <a.hindborg@samsung.com>, 
	Alice Ryhl <aliceryhl@google.com>, FUJITA Tomonori <fujita.tomonori@gmail.com>, 
	Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, 
	Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Heiko Stuebner <heiko@sntech.de>, rust-for-linux@vger.kernel.org, 
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 1, 2024 at 12:07=E2=80=AFPM Christina Quast
<contact@christina-quast.de> wrote:
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index e457b3c7cb2f..373a4d358e9f 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -95,6 +95,22 @@ pub fn phy_id(&self) -> u32 {
>          unsafe { (*phydev).phy_id }
>      }
>
> +    /// Gets the current crossover of the PHY.
> +    pub fn mdix(&self) -> u8 {

Are possible values for mdix always ETH_TP_MDI{,_INVALID,_X,_AUTO}? If
so, this would be better as an enum.

> +        let phydev =3D self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        unsafe { (*phydev).mdix }
> +    }

> +
>      /// Gets the state of PHY state machine states.
>      pub fn state(&self) -> DeviceState {
>          let phydev =3D self.0.get();
> @@ -300,6 +316,15 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>          // So it's just an FFI call.
>          to_result(unsafe { bindings::genphy_read_abilities(phydev) })
>      }
> +
> +    /// Writes BMCR
> +    pub fn genphy_config_aneg(&mut self) -> Result {

The docs need an update here

> +        let phydev =3D self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type in=
variant of `Self`.
> +        // So it's just an FFI call.
> +        // second param =3D false =3D> autoneg not requested
> +        to_result(unsafe { bindings::__genphy_config_aneg(phydev, false)=
 })

I assume you did this since the non-dunder `genphy_config_aneg` is
inline. I think that is ok since the implementation is so minimal, but
you could also add a binding helper and call that (rust/helpers.c).

> +    }
>  }
>
>  /// Defines certain other features this PHY supports (like interrupts).
> @@ -583,6 +608,12 @@ fn soft_reset(_dev: &mut Device) -> Result {
>          Err(code::ENOTSUPP)
>      }
>
> +    /// Called to initialize the PHY,
> +    /// including after a reset

Docs wrapping

> +    fn config_init(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }

These have been changed to raise a build error rather than ENOTSUPP in
recent , see [1]. That patch is in net-next so you should see it next
time you rebase.

Also - these functions are meant for the vtable and don't do anything
if they are not wired up. See the create_phy_driver function, you will
need to add the field.

>      /// Probes the hardware to determine what abilities it has.
>      fn get_features(_dev: &mut Device) -> Result {
>          Err(code::ENOTSUPP)
>
> --
> 2.43.0
>

[1]: https://lore.kernel.org/rust-for-linux/20240125014502.3527275-2-fujita=
tomonori@gmail.com/