[PATCH v3 0/2] net::phy add unified API for C22 and C45

[PATCH v3 0/2] net::phy add unified API for C22 and C45

[FUJITA Tomonori]

add unified API for C22 and C45, reading/writing registers and
genphy_read_status().

We could add methods specifically C45 like read/write_c45 and
genphy_c45_read_status. Instead, this implements the unified API for
C22 and C45 with a new reg module.

v3:
- move reg module to a new file (with MAINTAINERS updated)
- rewrite commit log and subjects
- rename Access trait to Register
- add build error message for build_assert!
- define the complete MMD list
- drop new func for Mmd
- remove trait GenPhyOps
v2: https://lore.kernel.org/all/20240601043535.53545-2-fujita.tomonori@gmail.com/T/
- add genphy helper support and split the patch
- create a 'reg' module
- define C22 according to the specs
- handle C22 vendor specific registers
- add const for C45 MMD
v1: https://lore.kernel.org/all/20240530.145258.456915814420167538.fujita.tomonori@gmail.com/T/


FUJITA Tomonori (2):
  rust: net::phy unified read/write API for C22 and C45 registers
  rust: net::phy unified genphy_read_status function for C22 and C45
    registers

 MAINTAINERS                      |   1 +
 drivers/net/phy/ax88796b_rust.rs |   7 +-
 rust/kernel/net/phy.rs           |  44 ++-----
 rust/kernel/net/phy/reg.rs       | 201 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 5 files changed, 216 insertions(+), 38 deletions(-)
 create mode 100644 rust/kernel/net/phy/reg.rs


base-commit: e19de2064fdf6f1f2488e36cf3927c3f1d01803c
-- 
2.34.1

[PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[FUJITA Tomonori]

Add the unified read/write API for C22 and C45 register. The
abstractions support access to only C22 registers now. Instead of
adding read/write_c45 methods specifically for C45, a new reg module
supports the unified API to access C22 and C45 registers with trait,
by calling an appropriate phylib functions.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS                      |   1 +
 drivers/net/phy/ax88796b_rust.rs |   7 +-
 rust/kernel/net/phy.rs           |  32 ++----
 rust/kernel/net/phy/reg.rs       | 174 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 5 files changed, 187 insertions(+), 28 deletions(-)
 create mode 100644 rust/kernel/net/phy/reg.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index ba231d2f3a4e..b2872658edb4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8210,6 +8210,7 @@ L:	netdev@vger.kernel.org
 L:	rust-for-linux@vger.kernel.org
 S:	Maintained
 F:	rust/kernel/net/phy.rs
+F:	rust/kernel/net/phy/reg.rs
 
 EXEC & BINFMT API, ELF
 R:	Eric Biederman <ebiederm@xmission.com>
diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index 5c92572962dc..8c7eb009d9fc 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -6,7 +6,7 @@
 //! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
 use kernel::{
     c_str,
-    net::phy::{self, DeviceId, Driver},
+    net::phy::{self, reg::C22, DeviceId, Driver},
     prelude::*,
     uapi,
 };
@@ -24,7 +24,6 @@
     license: "GPL",
 }
 
-const MII_BMCR: u16 = uapi::MII_BMCR as u16;
 const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;
 const BMCR_FULLDPLX: u16 = uapi::BMCR_FULLDPLX as u16;
 
@@ -33,7 +32,7 @@
 // Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation
 // such as used on the Individual Computers' X-Surf 100 Zorro card.
 fn asix_soft_reset(dev: &mut phy::Device) -> Result {
-    dev.write(uapi::MII_BMCR as u16, 0)?;
+    dev.write(C22::BMCR, 0)?;
     dev.genphy_soft_reset()
 }
 
@@ -55,7 +54,7 @@ fn read_status(dev: &mut phy::Device) -> Result<u16> {
         }
         // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
         // linkmode so use MII_BMCR as default values.
-        let ret = dev.read(MII_BMCR)?;
+        let ret = dev.read(C22::BMCR)?;
 
         if ret & BMCR_SPEED100 != 0 {
             dev.set_speed(uapi::SPEED_100);
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index fd40b703d224..9b4c4d24b231 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -7,9 +7,10 @@
 //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
 use crate::{error::*, prelude::*, types::Opaque};
-
 use core::marker::PhantomData;
 
+pub mod reg;
+
 /// PHY state machine states.
 ///
 /// Corresponds to the kernel's [`enum phy_state`].
@@ -175,32 +176,15 @@ pub fn set_duplex(&mut self, mode: DuplexMode) {
         unsafe { (*phydev).duplex = v };
     }
 
-    /// Reads a given C22 PHY register.
+    /// Reads a PHY register.
     // This function reads a hardware register and updates the stats so takes `&mut self`.
-    pub fn read(&mut self, regnum: u16) -> Result<u16> {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
-        // `phydev`.
-        let ret = unsafe {
-            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
-        };
-        if ret < 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(ret as u16)
-        }
+    pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
+        reg.read(self)
     }
 
-    /// Writes a given C22 PHY register.
-    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
-        // `phydev`.
-        to_result(unsafe {
-            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
-        })
+    /// Writes a PHY register.
+    pub fn write<R: reg::Register>(&mut self, reg: R, val: u16) -> Result {
+        reg.write(self, val)
     }
 
     /// Reads a paged register.
diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
new file mode 100644
index 000000000000..53a83e69b200
--- /dev/null
+++ b/rust/kernel/net/phy/reg.rs
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! PHY registers.
+//!
+//! This module provides an interface for PHY registers defined in IEEE 802.3
+//! Clause 22 and 45.
+
+use super::Device;
+use crate::build_assert;
+use crate::error::*;
+use crate::uapi;
+
+/// Accesses PHY registers.
+///
+/// This trait is used to implement the unified interface to access
+/// C22 and C45 PHY registers.
+pub trait Register {
+    /// Reads a PHY register.
+    fn read(&self, dev: &mut Device) -> Result<u16>;
+
+    /// Writes a PHY register.
+    fn write(&self, dev: &mut Device, val: u16) -> Result;
+}
+
+/// C22 registers.
+pub struct C22(u8);
+
+impl C22 {
+    /// Basic mode control.
+    pub const BMCR: Self = C22(0x00);
+    /// Basic mode status.
+    pub const BMSR: Self = C22(0x01);
+    /// PHY identifier 1.
+    pub const PHYSID1: Self = C22(0x02);
+    /// PHY identifier 2.
+    pub const PHYSID2: Self = C22(0x03);
+    /// Auto-negotiation advertisement.
+    pub const ADVERTISE: Self = C22(0x04);
+    /// Auto-negotiation link partner base page ability.
+    pub const LPA: Self = C22(0x05);
+    /// Auto-negotiation expansion.
+    pub const EXPANSION: Self = C22(0x06);
+    /// Auto-negotiation next page transmit.
+    pub const NEXT_PAGE_TRANSMIT: Self = C22(0x07);
+    /// Auto-negotiation link partner received next page.
+    pub const LP_RECEIVED_NEXT_PAGE: Self = C22(0x08);
+    /// Master-slave control.
+    pub const MASTER_SLAVE_CONTROL: Self = C22(0x09);
+    /// Master-slave status.
+    pub const MASTER_SLAVE_STATUS: Self = C22(0x0a);
+    /// PSE Control.
+    pub const PSE_CONTROL: Self = C22(0x0b);
+    /// PSE Status.
+    pub const PSE_STATUS: Self = C22(0x0c);
+    /// MMD Register control.
+    pub const MMD_CONTROL: Self = C22(0x0d);
+    /// MMD Register address data.
+    pub const MMD_DATA: Self = C22(0x0e);
+    /// Extended status.
+    pub const EXTENDED_STATUS: Self = C22(0x0f);
+
+    /// Creates a new instance of `C22` with a vendor specific register.
+    pub const fn vendor_specific<const N: u8>() -> Self {
+        build_assert!(
+            N > 0x0f && N < 0x20,
+            "Vendor-specific register address must be between 16 and 31"
+        );
+        C22(N)
+    }
+}
+
+impl Register for C22 {
+    fn read(&self, dev: &mut Device) -> Result<u16> {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+        // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
+        // `phydev`.
+        let ret = unsafe {
+            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into())
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    fn write(&self, dev: &mut Device, val: u16) -> Result {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
+        // `phydev`.
+        to_result(unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
+        })
+    }
+}
+
+/// MDIO manageable devices.
+pub struct Mmd(u8);
+
+impl Mmd {
+    /// Physical Medium Attachment/Dependent.
+    pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
+    /// WAN interface sublayer.
+    pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
+    /// Physical coding sublayer.
+    pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
+    /// PHY Extender sublayer.
+    pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
+    /// DTE Extender sublayer.
+    pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
+    /// Transmission convergence.
+    pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
+    /// Auto negotiation.
+    pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
+    /// Separated PMA (1).
+    pub const SEPARATED_PMA1: Self = Mmd(8);
+    /// Separated PMA (2).
+    pub const SEPARATED_PMA2: Self = Mmd(9);
+    /// Separated PMA (3).
+    pub const SEPARATED_PMA3: Self = Mmd(10);
+    /// Separated PMA (4).
+    pub const SEPARATED_PMA4: Self = Mmd(11);
+    /// OFDM PMA/PMD.
+    pub const OFDM_PMAPMD: Self = Mmd(12);
+    /// Power unit.
+    pub const POWER_UNIT: Self = Mmd(13);
+    /// Clause 22 extension.
+    pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
+    /// Vendor specific 1.
+    pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
+    /// Vendor specific 2.
+    pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
+}
+
+/// C45 registers.
+pub struct C45 {
+    devad: Mmd,
+    regnum: u16,
+}
+
+impl C45 {
+    /// Creates a new instance of `C45`.
+    pub fn new(devad: Mmd, regnum: u16) -> Self {
+        Self { devad, regnum }
+    }
+}
+
+impl Register for C45 {
+    fn read(&self, dev: &mut Device) -> Result<u16> {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+        // So it's just an FFI call.
+        let ret =
+            unsafe { bindings::phy_read_mmd(phydev, self.devad.0.into(), self.regnum.into()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    fn write(&self, dev: &mut Device, val: u16) -> Result {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+        // So it's just an FFI call.
+        to_result(unsafe {
+            bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
+        })
+    }
+}
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 08f5e9334c9e..76d3f103e764 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,5 +7,6 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mdio.h>
 #include <uapi/linux/mii.h>
 #include <uapi/linux/ethtool.h>
-- 
2.34.1

[PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[FUJITA Tomonori]

Add unified genphy_read_status function for C22 and C45
registers. Instead of having genphy_c22 and genphy_c45 methods, this
unifies genphy_read_status functions for C22 and C45.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs     | 12 ++----------
 rust/kernel/net/phy/reg.rs | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 9b4c4d24b231..ff16d44c7bb0 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -249,16 +249,8 @@ pub fn genphy_suspend(&mut self) -> Result {
     }
 
     /// Checks the link status and updates current link state.
-    pub fn genphy_read_status(&mut self) -> Result<u16> {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call.
-        let ret = unsafe { bindings::genphy_read_status(phydev) };
-        if ret < 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(ret as u16)
-        }
+    pub fn genphy_read_status<R: reg::Register>(&mut self) -> Result<u16> {
+        R::read_status(self)
     }
 
     /// Updates the link status.
diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
index 53a83e69b200..7628eb09e902 100644
--- a/rust/kernel/net/phy/reg.rs
+++ b/rust/kernel/net/phy/reg.rs
@@ -22,6 +22,9 @@ pub trait Register {
 
     /// Writes a PHY register.
     fn write(&self, dev: &mut Device, val: u16) -> Result;
+
+    /// Checks the link status and updates current link state.
+    fn read_status(dev: &mut Device) -> Result<u16>;
 }
 
 /// C22 registers.
@@ -96,6 +99,18 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
             bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
         })
     }
+
+    fn read_status(dev: &mut Device) -> Result<u16> {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So it's just an FFI call.
+        let ret = unsafe { bindings::genphy_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
 }
 
 /// MDIO manageable devices.
@@ -171,4 +186,16 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
             bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
         })
     }
+
+    fn read_status(dev: &mut Device) -> Result<u16> {
+        let phydev = dev.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So it's just an FFI call.
+        let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
 }
-- 
2.34.1

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Andrew Lunn]

On Mon, Jun 03, 2024 at 08:17:48AM +0900, FUJITA Tomonori wrote:
> Add the unified read/write API for C22 and C45 register. The
> abstractions support access to only C22 registers now. Instead of
> adding read/write_c45 methods specifically for C45, a new reg module
> supports the unified API to access C22 and C45 registers with trait,
> by calling an appropriate phylib functions.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  MAINTAINERS                      |   1 +
>  drivers/net/phy/ax88796b_rust.rs |   7 +-
>  rust/kernel/net/phy.rs           |  32 ++----
>  rust/kernel/net/phy/reg.rs       | 174 +++++++++++++++++++++++++++++++
>  rust/uapi/uapi_helper.h          |   1 +

I've not been monitoring kernel rust too much. What is the state of
the build system changes? Can we put Rust code in the subsystems yet?

	Andrew

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Miguel Ojeda]

On Mon, Jun 3, 2024 at 4:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I've not been monitoring kernel rust too much. What is the state of
> the build system changes? Can we put Rust code in the subsystems yet?

In progress.

Cheers,
Miguel

Re: [PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[Andrew Lunn]

On Mon, Jun 03, 2024 at 08:17:49AM +0900, FUJITA Tomonori wrote:
> Add unified genphy_read_status function for C22 and C45
> registers. Instead of having genphy_c22 and genphy_c45 methods, this
> unifies genphy_read_status functions for C22 and C45.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs     | 12 ++----------
>  rust/kernel/net/phy/reg.rs | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 10 deletions(-)

When this hits the netdev list, it needs a user. Having a user would
probably help me understand how you actually use this. How do it
control if it does a C22 or a C45 version?

	 Andrew

Re: [PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[FUJITA Tomonori]

On Mon, 3 Jun 2024 16:09:44 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Jun 03, 2024 at 08:17:49AM +0900, FUJITA Tomonori wrote:
>> Add unified genphy_read_status function for C22 and C45
>> registers. Instead of having genphy_c22 and genphy_c45 methods, this
>> unifies genphy_read_status functions for C22 and C45.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  rust/kernel/net/phy.rs     | 12 ++----------
>>  rust/kernel/net/phy/reg.rs | 27 +++++++++++++++++++++++++++
>>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> When this hits the netdev list, it needs a user. Having a user would
> probably help me understand how you actually use this. How do it

I suppose that this needs to be sent along with QT2025 PHY driver.

> control if it does a C22 or a C45 version?

Currently, a driver can execute C22 version like the following:

dev.genphy_read_status();

With this patch, a driver can execute C22 version like:

dev.genphy_read_status::<C22>();

And a driver can execute C45 version like:

dev.genphy_read_status::<C45>();

Re: [PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[Andrew Lunn]

> Currently, a driver can execute C22 version like the following:
> 
> dev.genphy_read_status();
> 
> With this patch, a driver can execute C22 version like:
> 
> dev.genphy_read_status::<C22>();
> 
> And a driver can execute C45 version like:
> 
> dev.genphy_read_status::<C45>();

O.K. That is nice and clear.

Thanks
	Andrew

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Trevor Gross]

On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add the unified read/write API for C22 and C45 register. The

s/register/registers

> abstractions support access to only C22 registers now. Instead of
> adding read/write_c45 methods specifically for C45, a new reg module
> supports the unified API to access C22 and C45 registers with trait,
> by calling an appropriate phylib functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sorry for the delays in reviews, this looks pretty good to me so far.
I like the cleanup of moving register logic to its own module.

> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
> new file mode 100644
> index 000000000000..53a83e69b200
> --- /dev/null
> +++ b/rust/kernel/net/phy/reg.rs
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! PHY registers.
> +//!
> +//! This module provides an interface for PHY registers defined in IEEE 802.3
> +//! Clause 22 and 45.

Docs suggestion:

    PHY register interfaces.

    This module provides support for accessing PHY registers via Ethernet
    management interface clause 22 and 45, as defined in IEEE 802.3.

> +use super::Device;
> +use crate::build_assert;
> +use crate::error::*;
> +use crate::uapi;
> +
> +/// Accesses PHY registers.
> +///
> +/// This trait is used to implement the unified interface to access
> +/// C22 and C45 PHY registers.
> +pub trait Register {

Since we don't use this anywhere could it be sealed?

This might not even need to be public since it should probably only be
used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
than importing the trait and using `C22::BCMR::read(my_device)`).

> +    /// Reads a PHY register.
> +    fn read(&self, dev: &mut Device) -> Result<u16>;
> +
> +    /// Writes a PHY register.
> +    fn write(&self, dev: &mut Device, val: u16) -> Result;
> +}
> +

> +/// C22 registers.
> +pub struct C22(u8);

Docs suggestion:

    A single MDIO clause 22 register address (5 bits).

> +/// MDIO manageable devices.
> +pub struct Mmd(u8);

> +/// C45 registers.

Docs suggestion

    A single MDIO clause 45 register device and address.

    Clause 45 uses a 5-bit device address to access a specific MMD within a
    port, then a 16-bit register address to access a location within
that device.
    `C45` represents this by storing a [`Mmd`] and a register number.

> +pub struct C45 {
> +    devad: Mmd,
> +    regnum: u16,
> +}

- Trevor

Re: [PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[Trevor Gross]

On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add unified genphy_read_status function for C22 and C45
> registers. Instead of having genphy_c22 and genphy_c45 methods, this
> unifies genphy_read_status functions for C22 and C45.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> [...]
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 9b4c4d24b231..ff16d44c7bb0 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -249,16 +249,8 @@ pub fn genphy_suspend(&mut self) -> Result {
>      }
>
>      /// Checks the link status and updates current link state.

Can you add an example with the usage you showed at [1]?

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

> +    pub fn genphy_read_status<R: reg::Register>(&mut self) -> Result<u16> {
> +        R::read_status(self)
>      }
>
>      /// Updates the link status.
> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
> index 53a83e69b200..7628eb09e902 100644
> --- a/rust/kernel/net/phy/reg.rs
> +++ b/rust/kernel/net/phy/reg.rs
> [...]
>  /// C22 registers.
> @@ -96,6 +99,18 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>              bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
>          })
>      }
> +
> +    fn read_status(dev: &mut Device) -> Result<u16> {
> [...]
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }

to_result can do this check and conversion a bit more succinctly, up
to you if you would prefer to use it

    error::to_result(ret)?;
    Ok(ret as u16)

There are some instances in patch 1/2 as well if you choose to change it.

> +    }
>  }
>
>  /// MDIO manageable devices.
> @@ -171,4 +186,16 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>              bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
>          })
>      }
> +
> +    fn read_status(dev: &mut Device) -> Result<u16> {
> +        let phydev = dev.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So it's just an FFI call.
> +        let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }

Ditto the above

> +    }
>  }
> --
> 2.34.1
>

- Trevor

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[FUJITA Tomonori]

On Thu, 6 Jun 2024 03:41:18 -0500
Trevor Gross <tmgross@umich.edu> wrote:

> On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Add the unified read/write API for C22 and C45 register. The
> 
> s/register/registers

Oops, I'll fix.

>> abstractions support access to only C22 registers now. Instead of
>> adding read/write_c45 methods specifically for C45, a new reg module
>> supports the unified API to access C22 and C45 registers with trait,
>> by calling an appropriate phylib functions.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Sorry for the delays in reviews, this looks pretty good to me so far.
> I like the cleanup of moving register logic to its own module.
> 
>> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
>> new file mode 100644
>> index 000000000000..53a83e69b200
>> --- /dev/null
>> +++ b/rust/kernel/net/phy/reg.rs
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
>> +
>> +//! PHY registers.
>> +//!
>> +//! This module provides an interface for PHY registers defined in IEEE 802.3
>> +//! Clause 22 and 45.
> 
> Docs suggestion:
> 
>     PHY register interfaces.
> 
>     This module provides support for accessing PHY registers via Ethernet
>     management interface clause 22 and 45, as defined in IEEE 802.3.

Much better than mine :) I'll update the comment.

>> +use super::Device;
>> +use crate::build_assert;
>> +use crate::error::*;
>> +use crate::uapi;
>> +
>> +/// Accesses PHY registers.
>> +///
>> +/// This trait is used to implement the unified interface to access
>> +/// C22 and C45 PHY registers.
>> +pub trait Register {
> 
> Since we don't use this anywhere could it be sealed?

Yeah. Only reg module implements it. How it can be done?

diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
index 7628eb09e902..0dfae4677574 100644
--- a/rust/kernel/net/phy/reg.rs
+++ b/rust/kernel/net/phy/reg.rs
@@ -12,11 +12,18 @@
 use crate::error::*;
 use crate::uapi;
 
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::C22 {}
+    impl Sealed for super::C45 {}
+}
+
 /// Accesses PHY registers.
 ///
 /// This trait is used to implement the unified interface to access
 /// C22 and C45 PHY registers.
-pub trait Register {
+pub trait Register: private::Sealed {
     /// Reads a PHY register.
     fn read(&self, dev: &mut Device) -> Result<u16>;


> This might not even need to be public since it should probably only be
> used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
> than importing the trait and using `C22::BCMR::read(my_device)`).

If I drop public, I got the following error:

error[E0603]: trait `Register` is private
   --> rust/kernel/net/phy.rs:181:25
    |
181 |     pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
    |                         ^^^^^^^^ private trait
    |
note: the trait `Register` is defined here
   --> rust/kernel/net/phy/reg.rs:19:1
    |
19  | trait Register {
    | ^^^^^^^^^^^^^^:

(snip)


>> +    /// Reads a PHY register.
>> +    fn read(&self, dev: &mut Device) -> Result<u16>;
>> +
>> +    /// Writes a PHY register.
>> +    fn write(&self, dev: &mut Device, val: u16) -> Result;
>> +}
>> +
> 
>> +/// C22 registers.
>> +pub struct C22(u8);
> 
> Docs suggestion:
> 
>     A single MDIO clause 22 register address (5 bits).

Thanks, I'll.


>> +/// MDIO manageable devices.
>> +pub struct Mmd(u8);
> 
>> +/// C45 registers.
> 
> Docs suggestion
> 
>     A single MDIO clause 45 register device and address.
> 
>     Clause 45 uses a 5-bit device address to access a specific MMD within a
>     port, then a 16-bit register address to access a location within
> that device.
>     `C45` represents this by storing a [`Mmd`] and a register number.
> 

Looks good. Thanks a lot!

Re: [PATCH v3 2/2] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[FUJITA Tomonori]

On Thu, 6 Jun 2024 03:52:12 -0500
Trevor Gross <tmgross@umich.edu> wrote:

> On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Add unified genphy_read_status function for C22 and C45
>> registers. Instead of having genphy_c22 and genphy_c45 methods, this
>> unifies genphy_read_status functions for C22 and C45.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> [...]
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 9b4c4d24b231..ff16d44c7bb0 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -249,16 +249,8 @@ pub fn genphy_suspend(&mut self) -> Result {
>>      }
>>
>>      /// Checks the link status and updates current link state.
> 
> Can you add an example with the usage you showed at [1]?
> 
> [1]: https://lore.kernel.org/rust-for-linux/20240603.233954.1644803151268035787.fujita.tomonori@gmail.com/

Indeed, I'll.


>> +    pub fn genphy_read_status<R: reg::Register>(&mut self) -> Result<u16> {
>> +        R::read_status(self)
>>      }
>>
>>      /// Updates the link status.
>> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
>> index 53a83e69b200..7628eb09e902 100644
>> --- a/rust/kernel/net/phy/reg.rs
>> +++ b/rust/kernel/net/phy/reg.rs
>> [...]
>>  /// C22 registers.
>> @@ -96,6 +99,18 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>>              bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
>>          })
>>      }
>> +
>> +    fn read_status(dev: &mut Device) -> Result<u16> {
>> [...]
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
> 
> to_result can do this check and conversion a bit more succinctly, up
> to you if you would prefer to use it
> 
>     error::to_result(ret)?;
>     Ok(ret as u16)
> 
> There are some instances in patch 1/2 as well if you choose to change it.

Ah, cleaver. I'll use this.


>> +    }
>>  }
>>
>>  /// MDIO manageable devices.
>> @@ -171,4 +186,16 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>>              bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
>>          })
>>      }
>> +
>> +    fn read_status(dev: &mut Device) -> Result<u16> {
>> +        let phydev = dev.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        // So it's just an FFI call.
>> +        let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
> 
> Ditto the above

I'll change the above too.


Thanks a lot!

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Trevor Gross]

On Thu, Jun 6, 2024 at 7:03 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 6 Jun 2024 03:41:18 -0500
> Trevor Gross <tmgross@umich.edu> wrote:
>
> > On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> [..]
> >> +use super::Device;
> >> +use crate::build_assert;
> >> +use crate::error::*;
> >> +use crate::uapi;
> >> +
> >> +/// Accesses PHY registers.
> >> +///
> >> +/// This trait is used to implement the unified interface to access
> >> +/// C22 and C45 PHY registers.
> >> +pub trait Register {
> >
> > Since we don't use this anywhere could it be sealed?
>
> Yeah. Only reg module implements it. How it can be done?
>
> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
> index 7628eb09e902..0dfae4677574 100644
> --- a/rust/kernel/net/phy/reg.rs
> +++ b/rust/kernel/net/phy/reg.rs
> @@ -12,11 +12,18 @@
>  use crate::error::*;
>  use crate::uapi;
>
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::C22 {}
> +    impl Sealed for super::C45 {}
> +}
> +
>  /// Accesses PHY registers.
>  ///
>  /// This trait is used to implement the unified interface to access
>  /// C22 and C45 PHY registers.
> -pub trait Register {
> +pub trait Register: private::Sealed {
>      /// Reads a PHY register.
>      fn read(&self, dev: &mut Device) -> Result<u16>;
>
>
> > This might not even need to be public since it should probably only be
> > used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
> > than importing the trait and using `C22::BCMR::read(my_device)`).
>
> If I drop public, I got the following error:
>
> error[E0603]: trait `Register` is private
>    --> rust/kernel/net/phy.rs:181:25
>     |
> 181 |     pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
>     |                         ^^^^^^^^ private trait
>     |
> note: the trait `Register` is defined here
>    --> rust/kernel/net/phy/reg.rs:19:1
>     |
> 19  | trait Register {
>     | ^^^^^^^^^^^^^^:
>
> (snip)

Oh, I did mean `pub(crate)`.

If that doesn't work, usually the sealed pattern looks like this:

    /* lib.rs */
    mod private {
        /// Marker that a trait cannot be implemented outside of this crate
        trait Sealed {}
    }

    /* reg.rs or other modules */
    use crate::private::Sealed;

    // Traits that aren't meant to be implemented outside of the crate
    // get `Sealed` as a bound
    pub trait Register: Sealed { /* ... */ }

    // Anything with a `Register` impl now also needs `Sealed`
    impl Sealed for C22 {}
    impl Register for C22 { /* ... */ }

    // And then nobody from a different crate can implement `Register` on
    // some random type, because they have no way to access/implement
    // private `Sealed`

I also asked about this because I'm a bit surprised it hasn't come up yet [1].

But if `pub(crate)` works then that is easier, of course.

- Trevor

[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Sealed.20traits/near/443180412

Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[FUJITA Tomonori]

On Thu, 6 Jun 2024 19:43:55 -0400
Trevor Gross <tmgross@umich.edu> wrote:

>> > This might not even need to be public since it should probably only be
>> > used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
>> > than importing the trait and using `C22::BCMR::read(my_device)`).
>>
>> If I drop public, I got the following error:
>>
>> error[E0603]: trait `Register` is private
>>    --> rust/kernel/net/phy.rs:181:25
>>     |
>> 181 |     pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
>>     |                         ^^^^^^^^ private trait
>>     |
>> note: the trait `Register` is defined here
>>    --> rust/kernel/net/phy/reg.rs:19:1
>>     |
>> 19  | trait Register {
>>     | ^^^^^^^^^^^^^^:
>>
>> (snip)
> 
> Oh, I did mean `pub(crate)`.

Oops. Indeed it works but I got the following warnings:

warning: trait `Register` is more private than the item `Device::read`
   --> rust/kernel/net/phy.rs:181:5
    |
181 |     pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ method `Device::read` is reachable at visibility `pub`
    |
note: but trait `Register` is only usable at visibility `pub(crate)`
   --> rust/kernel/net/phy/reg.rs:26:1
    |
26  | pub(crate) trait Register {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: `#[warn(private_bounds)]` on by default

(snip)