[PATCH net-next v4 0/6] net: phy: add Applied Micro QT2025 PHY driver

[PATCH net-next v4 0/6] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

This patchset adds a PHY driver for Applied Micro Circuits Corporation
QT2025.

The first patch adds Rust equivalent to include/linux/sizes.h, makes
code more readable. The 2-5th patches update the PHYLIB Rust bindings.

QT2025 PHY support was implemented as a part of an Ethernet driver for
Tehuti Networks TN40xx chips. Multiple vendors (DLink, Asus, Edimax,
QNAP, etc) developed adapters based on TN40xx chips. Tehuti Networks
went out of business and the driver wasn't merged into mainline. But
it's still distributed with some of the hardware (and also available
on some vendor sites).

The original driver handles multiple PHY hardware (AMCC QT2025, TI
TLK10232, Aqrate AQR105, and Marvell MV88X3120, MV88X3310, and
MV88E2010). I divided the original driver into MAC and PHY drivers and
implemented a QT2025 PHY driver in Rust.

The MAC driver for Tehuti Networks TN40xx chips was already merged in
6.11-rc1. The MAC and this PHY drivers have been tested with Edimax
EN-9320SFP+ 10G network adapter.

v4:
- fix the comments
- add Andrew's Reviewed-by
- fix the order of tags
v3: https://lore.kernel.org/netdev/20240804233835.223460-1-fujita.tomonori@gmail.com/
- use addr_of_mut!` to avoid intermediate mutable reference
- update probe callback's Safety comment
- add MODULE_FIRMWARE equivalent
- add Alice's Reviewed-by
v2: https://lore.kernel.org/netdev/20240731042136.201327-1-fujita.tomonori@gmail.com/
- add comments in accordance with the hw datasheet
- unify C22 and C45 APIs
- load firmware in probe callback instead of config_init
- use firmware API
- handle firmware endian
- check firmware size
- use SZ_*K constants
- avoid confusing phy_id variable
v1: https://lore.kernel.org/netdev/20240415104701.4772-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (6):
  rust: sizes: add commonly used constants
  rust: net::phy support probe callback
  rust: net::phy implement AsRef<kernel::device::Device> trait
  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
  net: phy: add Applied Micro QT2025 PHY driver

 MAINTAINERS                      |   8 ++
 drivers/net/phy/Kconfig          |   6 +
 drivers/net/phy/Makefile         |   1 +
 drivers/net/phy/ax88796b_rust.rs |   7 +-
 drivers/net/phy/qt2025.rs        |  90 +++++++++++++
 rust/kernel/lib.rs               |   1 +
 rust/kernel/net/phy.rs           |  78 ++++++-----
 rust/kernel/net/phy/reg.rs       | 222 +++++++++++++++++++++++++++++++
 rust/kernel/sizes.rs             |  26 ++++
 rust/uapi/uapi_helper.h          |   1 +
 10 files changed, 402 insertions(+), 38 deletions(-)
 create mode 100644 drivers/net/phy/qt2025.rs
 create mode 100644 rust/kernel/net/phy/reg.rs
 create mode 100644 rust/kernel/sizes.rs


base-commit: 399117317001d0f5bf4194feccaafa62b12e744f
-- 
2.34.1

[PATCH net-next v4 1/6] rust: sizes: add commonly used constants

[FUJITA Tomonori]

Add rust equivalent to include/linux/sizes.h, makes code more
readable.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/lib.rs   |  1 +
 rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 rust/kernel/sizes.rs

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 274bdc1b0a82..58ed400198bf 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -43,6 +43,7 @@
 pub mod page;
 pub mod prelude;
 pub mod print;
+pub mod sizes;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
new file mode 100644
index 000000000000..834c343e4170
--- /dev/null
+++ b/rust/kernel/sizes.rs
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Commonly used sizes.
+//!
+//! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
+
+/// 0x00000400
+pub const SZ_1K: usize = bindings::SZ_1K as usize;
+/// 0x00000800
+pub const SZ_2K: usize = bindings::SZ_2K as usize;
+/// 0x00001000
+pub const SZ_4K: usize = bindings::SZ_4K as usize;
+/// 0x00002000
+pub const SZ_8K: usize = bindings::SZ_8K as usize;
+/// 0x00004000
+pub const SZ_16K: usize = bindings::SZ_16K as usize;
+/// 0x00008000
+pub const SZ_32K: usize = bindings::SZ_32K as usize;
+/// 0x00010000
+pub const SZ_64K: usize = bindings::SZ_64K as usize;
+/// 0x00020000
+pub const SZ_128K: usize = bindings::SZ_128K as usize;
+/// 0x00040000
+pub const SZ_256K: usize = bindings::SZ_256K as usize;
+/// 0x00080000
+pub const SZ_512K: usize = bindings::SZ_512K as usize;
-- 
2.34.1

[PATCH net-next v4 2/6] rust: net::phy support probe callback

[FUJITA Tomonori]

Support phy_driver probe callback, used to set up device-specific
structures.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index fd40b703d224..5e8137a1972f 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -338,6 +338,21 @@ impl<T: Driver> Adapter<T> {
         })
     }
 
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we can exclusively access `phy_device` because
+            // it's not published yet, so the accessors on `Device` are okay
+            // to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::probe(dev)?;
+            Ok(0)
+        })
+    }
+
     /// # Safety
     ///
     /// `phydev` must be passed by the corresponding callback in `phy_driver`.
@@ -511,6 +526,11 @@ pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
         } else {
             None
         },
+        probe: if T::HAS_PROBE {
+            Some(Adapter::<T>::probe_callback)
+        } else {
+            None
+        },
         get_features: if T::HAS_GET_FEATURES {
             Some(Adapter::<T>::get_features_callback)
         } else {
@@ -583,6 +603,11 @@ fn soft_reset(_dev: &mut Device) -> Result {
         kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
+    /// Sets up device-specific structures during discovery.
+    fn probe(_dev: &mut Device) -> Result {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
         kernel::build_error(VTABLE_DEFAULT_ERROR)
-- 
2.34.1

[PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[FUJITA Tomonori]

Implement AsRef<kernel::device::Device> trait for Device. A PHY driver
needs a reference to device::Device to call the firmware API.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 5e8137a1972f..569dd3beef9c 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -7,8 +7,7 @@
 //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
 use crate::{error::*, prelude::*, types::Opaque};
-
-use core::marker::PhantomData;
+use core::{marker::PhantomData, ptr::addr_of_mut};
 
 /// PHY state machine states.
 ///
@@ -302,6 +301,15 @@ pub fn genphy_read_abilities(&mut self) -> Result {
     }
 }
 
+impl AsRef<kernel::device::Device> for Device {
+    fn as_ref(&self) -> &kernel::device::Device {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
+    }
+}
+
 /// Defines certain other features this PHY supports (like interrupts).
 ///
 /// These flag values are used in [`Driver::FLAGS`].
-- 
2.34.1

[PATCH net-next v4 4/6] rust: net::phy unified read/write API for C22 and C45 registers

[FUJITA Tomonori]

Add the unified read/write API for C22 and C45 registers. 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.

Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS                      |   1 +
 drivers/net/phy/ax88796b_rust.rs |   7 +-
 rust/kernel/net/phy.rs           |  31 ++---
 rust/kernel/net/phy/reg.rs       | 194 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   1 +
 5 files changed, 207 insertions(+), 27 deletions(-)
 create mode 100644 rust/kernel/net/phy/reg.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 5dbf23cf11c8..9dbfcf77acb2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8354,6 +8354,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 569dd3beef9c..f2cdb617d44b 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -9,6 +9,8 @@
 use crate::{error::*, prelude::*, types::Opaque};
 use core::{marker::PhantomData, ptr::addr_of_mut};
 
+pub mod reg;
+
 /// PHY state machine states.
 ///
 /// Corresponds to the kernel's [`enum phy_state`].
@@ -174,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..5fb05b79a956
--- /dev/null
+++ b/rust/kernel/net/phy/reg.rs
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! PHY register interfaces.
+//!
+//! This module provides support for accessing PHY registers in the
+//! Ethernet management interface clauses 22 and 45 register namespaces, as
+//! defined in IEEE 802.3.
+
+use super::Device;
+use crate::build_assert;
+use crate::error::*;
+use crate::uapi;
+
+mod private {
+    /// Marker that a trait cannot be implemented outside of this crate
+    pub trait Sealed {}
+}
+
+/// Accesses PHY registers.
+///
+/// This trait is used to implement the unified interface to access
+/// C22 and C45 PHY registers.
+///
+/// # Examples
+///
+/// ```ignore
+/// fn link_change_notify(dev: &mut Device) {
+///     // read C22 BMCR register
+///     dev.read(C22::BMCR);
+///     // read C45 PMA/PMD control 1 register
+///     dev.read(C45::new(Mmd::PMAPMD, 0));
+/// }
+/// ```
+pub trait Register: private::Sealed {
+    /// 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;
+}
+
+/// A single MDIO clause 22 register address (5 bits).
+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 private::Sealed for C22 {}
+
+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())
+        };
+        to_result(ret)?;
+        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)
+        })
+    }
+}
+
+/// A single MDIO clause 45 register device and address.
+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);
+}
+
+/// 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,
+}
+
+impl C45 {
+    /// Creates a new instance of `C45`.
+    pub fn new(devad: Mmd, regnum: u16) -> Self {
+        Self { devad, regnum }
+    }
+}
+
+impl private::Sealed for C45 {}
+
+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()) };
+        to_result(ret)?;
+        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 net-next v4 5/6] 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.

Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs     | 12 ++----------
 rust/kernel/net/phy/reg.rs | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index f2cdb617d44b..deccffde31f8 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 5fb05b79a956..7edb5d494e0a 100644
--- a/rust/kernel/net/phy/reg.rs
+++ b/rust/kernel/net/phy/reg.rs
@@ -31,6 +31,13 @@ pub trait Sealed {}
 ///     dev.read(C22::BMCR);
 ///     // read C45 PMA/PMD control 1 register
 ///     dev.read(C45::new(Mmd::PMAPMD, 0));
+///
+///     // Checks the link status as reported by registers in the C22 namespace
+///     // and updates current link state.
+///     dev.genphy_read_status::<phy::C22>();
+///     // Checks the link status as reported by registers in the C45 namespace
+///     // and updates current link state.
+///     dev.genphy_read_status::<phy::C45>();
 /// }
 /// ```
 pub trait Register: private::Sealed {
@@ -39,6 +46,9 @@ pub trait Register: private::Sealed {
 
     /// 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>;
 }
 
 /// A single MDIO clause 22 register address (5 bits).
@@ -112,6 +122,15 @@ 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) };
+        to_result(ret)?;
+        Ok(ret as u16)
+    }
 }
 
 /// A single MDIO clause 45 register device and address.
@@ -191,4 +210,13 @@ 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) };
+        to_result(ret)?;
+        Ok(ret as u16)
+    }
 }
-- 
2.34.1

[PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

This driver supports Applied Micro Circuits Corporation QT2025 PHY,
based on a driver for Tehuti Networks TN40xx chips.

The original driver for TN40xx chips supports multiple PHY hardware
(AMCC QT2025, TI TLK10232, Aqrate AQR105, and Marvell 88X3120,
88X3310, and MV88E2010). This driver is extracted from the original
driver and modified to a PHY driver in Rust.

This has been tested with Edimax EN-9320SFP+ 10G network adapter.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS               |  7 +++
 drivers/net/phy/Kconfig   |  6 +++
 drivers/net/phy/Makefile  |  1 +
 drivers/net/phy/qt2025.rs | 90 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+)
 create mode 100644 drivers/net/phy/qt2025.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 9dbfcf77acb2..d4464e59dfea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1609,6 +1609,13 @@ F:	Documentation/admin-guide/perf/xgene-pmu.rst
 F:	Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
 F:	drivers/perf/xgene_pmu.c
 
+APPLIED MICRO QT2025 PHY DRIVER
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+L:	netdev@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/qt2025.rs
+
 APTINA CAMERA SENSOR PLL
 M:	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 7fddc8306d82..e0ff386d90cd 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -109,6 +109,12 @@ config ADIN1100_PHY
 	  Currently supports the:
 	  - ADIN1100 - Robust,Industrial, Low Power 10BASE-T1L Ethernet PHY
 
+config AMCC_QT2025_PHY
+	tristate "AMCC QT2025 PHY"
+	depends on RUST_PHYLIB_ABSTRACTIONS
+	help
+	  Adds support for the Applied Micro Circuits Corporation QT2025 PHY.
+
 source "drivers/net/phy/aquantia/Kconfig"
 
 config AX88796B_PHY
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 202ed7f450da..461d6a3b9997 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_ADIN_PHY)		+= adin.o
 obj-$(CONFIG_ADIN1100_PHY)	+= adin1100.o
 obj-$(CONFIG_AIR_EN8811H_PHY)   += air_en8811h.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
+obj-$(CONFIG_AMCC_QT2025_PHY)	+= qt2025.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
 ifdef CONFIG_AX88796B_RUST_PHY
   obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
new file mode 100644
index 000000000000..45864a7e1120
--- /dev/null
+++ b/drivers/net/phy/qt2025.rs
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) Tehuti Networks Ltd.
+// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Applied Micro Circuits Corporation QT2025 PHY driver
+use kernel::c_str;
+use kernel::error::code;
+use kernel::firmware::Firmware;
+use kernel::net::phy::{
+    self,
+    reg::{Mmd, C45},
+    DeviceId, Driver,
+};
+use kernel::prelude::*;
+use kernel::sizes::{SZ_16K, SZ_32K, SZ_8K};
+
+kernel::module_phy_driver! {
+    drivers: [PhyQT2025],
+    device_table: [
+        DeviceId::new_with_driver::<PhyQT2025>(),
+    ],
+    name: "qt2025_phy",
+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    description: "AMCC QT2025 PHY driver",
+    license: "GPL",
+    firmware: ["qt2025-2.0.3.3.fw"],
+}
+
+struct PhyQT2025;
+
+#[vtable]
+impl Driver for PhyQT2025 {
+    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
+
+    fn probe(dev: &mut phy::Device) -> Result<()> {
+        // The vendor driver does the following checking but we have no idea why.
+        let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?;
+        if (hw_id >> 8) & 0xff != 0xb3 {
+            return Err(code::ENODEV);
+        }
+
+        // The 8051 will remain in the reset state.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?;
+        // Configure the 8051 clock frequency.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?;
+        // Non loopback mode.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC319), 0x0038)?;
+        // Global control bit to select between LAN and WAN (WIS) mode.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC31A), 0x0098)?;
+        dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?;
+        dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?;
+        dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?;
+        dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?;
+        // Configure transmit and recovered clock.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?;
+        // The 8051 will finish the reset state.
+        dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?;
+        // The 8051 will start running from the boot ROM.
+        dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?;
+
+        let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?;
+        if fw.data().len() > SZ_16K + SZ_8K {
+            return Err(code::EFBIG);
+        }
+
+        // The 24kB of program memory space is accessible by MDIO.
+        // The first 16kB of memory is located in the address range 3.8000h - 3.BFFFh.
+        // The next 8kB of memory is located at 4.8000h - 4.9FFFh.
+        let mut j = SZ_32K;
+        for (i, val) in fw.data().iter().enumerate() {
+            if i == SZ_16K {
+                j = SZ_32K;
+            }
+
+            let mmd = if i < SZ_16K { Mmd::PCS } else { Mmd::PHYXS };
+            dev.write(C45::new(mmd, j as u16), (*val).into())?;
+
+            j += 1;
+        }
+        // The 8051 will start running from SRAM.
+        dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?;
+
+        Ok(())
+    }
+
+    fn read_status(dev: &mut phy::Device) -> Result<u16> {
+        dev.genphy_read_status::<C45>()
+    }
+}
-- 
2.34.1

Re: [PATCH net-next v4 1/6] rust: sizes: add commonly used constants

[Benno Lossin]

On 17.08.24 07:19, FUJITA Tomonori wrote:
> Add rust equivalent to include/linux/sizes.h, makes code more
> readable.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/lib.rs   |  1 +
>  rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>  create mode 100644 rust/kernel/sizes.rs

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH net-next v4 2/6] rust: net::phy support probe callback

[Benno Lossin]

On 17.08.24 07:19, FUJITA Tomonori wrote:
> Support phy_driver probe callback, used to set up device-specific
> structures.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 17.08.24 07:19, FUJITA Tomonori wrote:
> Implement AsRef<kernel::device::Device> trait for Device. A PHY driver
> needs a reference to device::Device to call the firmware API.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 5e8137a1972f..569dd3beef9c 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -7,8 +7,7 @@
>  //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
> 
>  use crate::{error::*, prelude::*, types::Opaque};
> -
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, ptr::addr_of_mut};
> 
>  /// PHY state machine states.
>  ///
> @@ -302,6 +301,15 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
> 
> +impl AsRef<kernel::device::Device> for Device {
> +    fn as_ref(&self) -> &kernel::device::Device {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.

I don't see this invariant on `phy::Device`.

---
Cheers,
Benno

> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
> +    }
> +}
> +
>  /// Defines certain other features this PHY supports (like interrupts).
>  ///
>  /// These flag values are used in [`Driver::FLAGS`].
> --
> 2.34.1
> 

Re: [PATCH net-next v4 4/6] rust: net::phy unified read/write API for C22 and C45 registers

[Andrew Lunn]

On Sat, Aug 17, 2024 at 05:19:37AM +0000, FUJITA Tomonori wrote:
> Add the unified read/write API for C22 and C45 registers. 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.
> 
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v4 5/6] rust: net::phy unified genphy_read_status function for C22 and C45 registers

[Andrew Lunn]

On Sat, Aug 17, 2024 at 05:19:38AM +0000, 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.
> 
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> +        dev.genphy_read_status::<C45>()
> +    }

Probably a dumb Rust question. Shouldn't this have a ? at the end? It
can return a negative error code.

	Andrew

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Benno Lossin]

On 17.08.24 20:51, Andrew Lunn wrote:
>> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
>> +        dev.genphy_read_status::<C45>()
>> +    }
> 
> Probably a dumb Rust question. Shouldn't this have a ? at the end? It
> can return a negative error code.

`read_status` returns a `Result<u16>` and `Device::genphy_read_status`
also returns a `Result<u16>`. In the function body we just delegate to
the latter, so no `?` is needed. We just return the entire result.

Here is the equivalent pseudo-C code:

    int genphy_read_status(struct phy_device *dev);
    
    int read_status(struct phy_device *dev)
    {
        return genphy_read_status(dev);
    }

There you also don't need an if for the negative error code, since it's
just propagated.

Hope this helps!

---
Cheers,
Benno

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[FUJITA Tomonori]

On Sat, 17 Aug 2024 13:30:15 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> +impl AsRef<kernel::device::Device> for Device {
>> +    fn as_ref(&self) -> &kernel::device::Device {
>> +        let phydev = self.0.get();
>> +        // SAFETY: The struct invariant ensures that we may access
>> +        // this field without additional synchronization.
> 
> I don't see this invariant on `phy::Device`.

You meant that `phy::Device` Invariants says that all methods defined
on this struct are safe to call; not about accessing a field so the
above SAFETY comment isn't correct, right?

> ---
> Cheers,
> Benno
> 
>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>> +    }
>> +}

SAFETY: A valid `phy_device` always have a valid `mdio.dev`.

Better?

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 04:13, FUJITA Tomonori wrote:
> On Sat, 17 Aug 2024 13:30:15 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>> +impl AsRef<kernel::device::Device> for Device {
>>> +    fn as_ref(&self) -> &kernel::device::Device {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: The struct invariant ensures that we may access
>>> +        // this field without additional synchronization.
>>
>> I don't see this invariant on `phy::Device`.
> 
> You meant that `phy::Device` Invariants says that all methods defined
> on this struct are safe to call; not about accessing a field so the
> above SAFETY comment isn't correct, right?

Correct.

>> ---
>> Cheers,
>> Benno
>>
>>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>>> +    }
>>> +}
> 
> SAFETY: A valid `phy_device` always have a valid `mdio.dev`.
> 
> Better?

It would be nice if you could add this on the invariants on
`phy::Device` (you will also have to extend the INVAIRANTS comment that
creates a `&'a mut Device`)

---
Cheers,
Benno

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[FUJITA Tomonori]

On Sun, 18 Aug 2024 06:01:27 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>>>> +impl AsRef<kernel::device::Device> for Device {
>>>> +    fn as_ref(&self) -> &kernel::device::Device {
>>>> +        let phydev = self.0.get();
>>>> +        // SAFETY: The struct invariant ensures that we may access
>>>> +        // this field without additional synchronization.
>>>
>>> I don't see this invariant on `phy::Device`.
>> 
>> You meant that `phy::Device` Invariants says that all methods defined
>> on this struct are safe to call; not about accessing a field so the
>> above SAFETY comment isn't correct, right?
> 
> Correct.

Understood.

>>>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>>>> +    }
>>>> +}
>> 
>> SAFETY: A valid `phy_device` always have a valid `mdio.dev`.
>> 
>> Better?
> 
> It would be nice if you could add this on the invariants on
> `phy::Device` (you will also have to extend the INVAIRANTS comment that
> creates a `&'a mut Device`)

How about the followings?

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 5e8137a1972f..3e1d6c43ca33 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -7,8 +7,7 @@
 //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
 use crate::{error::*, prelude::*, types::Opaque};
-
-use core::marker::PhantomData;
+use core::{marker::PhantomData, ptr::addr_of_mut};
 
 /// PHY state machine states.
 ///
@@ -60,6 +59,7 @@ pub enum DuplexMode {
 ///
 /// Referencing a `phy_device` using this struct asserts that you are in
 /// a context where all methods defined on this struct are safe to call.
+/// This struct always has a valid `mdio.dev`.
 ///
 /// [`struct phy_device`]: srctree/include/linux/phy.h
 // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
@@ -76,9 +76,9 @@ impl Device {
     ///
     /// # Safety
     ///
-    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
-    /// and the caller must be in a context where all methods defined on this struct
-    /// are safe to call.
+    /// For the duration of 'a, the pointer must point at a valid `phy_device` with
+    /// a valid `mdio.dev`, and the caller must be in a context where all methods
+    /// defined on this struct are safe to call.
     unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
         // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
         let ptr = ptr.cast::<Self>();
@@ -302,6 +302,14 @@ pub fn genphy_read_abilities(&mut self) -> Result {
     }
 }
 
+impl AsRef<kernel::device::Device> for Device {
+    fn as_ref(&self) -> &kernel::device::Device {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
+        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
+    }
+}
+
 /// Defines certain other features this PHY supports (like interrupts).
 ///
 /// These flag values are used in [`Driver::FLAGS`].

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 09:36, FUJITA Tomonori wrote:
> On Sun, 18 Aug 2024 06:01:27 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>>>>> +    }
>>>>> +}
>>>
>>> SAFETY: A valid `phy_device` always have a valid `mdio.dev`.
>>>
>>> Better?
>>
>> It would be nice if you could add this on the invariants on
>> `phy::Device` (you will also have to extend the INVAIRANTS comment that
>> creates a `&'a mut Device`)
> 
> How about the followings?
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 5e8137a1972f..3e1d6c43ca33 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -7,8 +7,7 @@
>  //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
> 
>  use crate::{error::*, prelude::*, types::Opaque};
> -
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, ptr::addr_of_mut};
> 
>  /// PHY state machine states.
>  ///
> @@ -60,6 +59,7 @@ pub enum DuplexMode {
>  ///
>  /// Referencing a `phy_device` using this struct asserts that you are in
>  /// a context where all methods defined on this struct are safe to call.
> +/// This struct always has a valid `mdio.dev`.

Please turn this into a bullet point list.

>  ///
>  /// [`struct phy_device`]: srctree/include/linux/phy.h
>  // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> @@ -76,9 +76,9 @@ impl Device {
>      ///
>      /// # Safety
>      ///
> -    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
> -    /// and the caller must be in a context where all methods defined on this struct
> -    /// are safe to call.
> +    /// For the duration of 'a, the pointer must point at a valid `phy_device` with
> +    /// a valid `mdio.dev`, and the caller must be in a context where all methods
> +    /// defined on this struct are safe to call.

Also here.

>      unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>          // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>          let ptr = ptr.cast::<Self>();
> @@ -302,6 +302,14 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
> 
> +impl AsRef<kernel::device::Device> for Device {
> +    fn as_ref(&self) -> &kernel::device::Device {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
> +    }

Just to be sure: the `phydev.mdio.dev` struct is refcounted and
incrementing the refcount is fine, right?

---
Cheers,
Benno

> +}
> +
>  /// Defines certain other features this PHY supports (like interrupts).
>  ///
>  /// These flag values are used in [`Driver::FLAGS`].

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[FUJITA Tomonori]

On Sun, 18 Aug 2024 09:03:01 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>>  /// PHY state machine states.
>>  ///
>> @@ -60,6 +59,7 @@ pub enum DuplexMode {
>>  ///
>>  /// Referencing a `phy_device` using this struct asserts that you are in
>>  /// a context where all methods defined on this struct are safe to call.
>> +/// This struct always has a valid `mdio.dev`.
> 
> Please turn this into a bullet point list.

/// - Referencing a `phy_device` using this struct asserts that you are in
///   a context where all methods defined on this struct are safe to call.
/// - This struct always has a valid `mdio.dev`.

Looks fine?

>>  ///
>>  /// [`struct phy_device`]: srctree/include/linux/phy.h
>>  // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
>> @@ -76,9 +76,9 @@ impl Device {
>>      ///
>>      /// # Safety
>>      ///
>> -    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
>> -    /// and the caller must be in a context where all methods defined on this struct
>> -    /// are safe to call.
>> +    /// For the duration of 'a, the pointer must point at a valid `phy_device` with
>> +    /// a valid `mdio.dev`, and the caller must be in a context where all methods
>> +    /// defined on this struct are safe to call.
> 
> Also here.

/// # Safety
///
/// For the duration of 'a,
/// - the pointer must point at a valid `phy_device`, and the caller
///   must be in a context where all methods defined on this struct
///   are safe to call.
/// - 'mdio.dev' must be a valid.

Better?

>> +impl AsRef<kernel::device::Device> for Device {
>> +    fn as_ref(&self) -> &kernel::device::Device {
>> +        let phydev = self.0.get();
>> +        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>> +    }
> 
> Just to be sure: the `phydev.mdio.dev` struct is refcounted and
> incrementing the refcount is fine, right?

phydev.mdio.dev is valid after phydev is initialized.

struct phy_device {
	struct mdio_device mdio;
	...

struct mdio_device {
	struct device dev;
	...

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 11:15, FUJITA Tomonori wrote:
> On Sun, 18 Aug 2024 09:03:01 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>>  /// PHY state machine states.
>>>  ///
>>> @@ -60,6 +59,7 @@ pub enum DuplexMode {
>>>  ///
>>>  /// Referencing a `phy_device` using this struct asserts that you are in
>>>  /// a context where all methods defined on this struct are safe to call.
>>> +/// This struct always has a valid `mdio.dev`.
>>
>> Please turn this into a bullet point list.
> 
> /// - Referencing a `phy_device` using this struct asserts that you are in
> ///   a context where all methods defined on this struct are safe to call.
> /// - This struct always has a valid `mdio.dev`.

Hmm, I think `self.0.mdio.dev` would be clearer.

> 
> Looks fine?
> 
>>>  ///
>>>  /// [`struct phy_device`]: srctree/include/linux/phy.h
>>>  // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
>>> @@ -76,9 +76,9 @@ impl Device {
>>>      ///
>>>      /// # Safety
>>>      ///
>>> -    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
>>> -    /// and the caller must be in a context where all methods defined on this struct
>>> -    /// are safe to call.
>>> +    /// For the duration of 'a, the pointer must point at a valid `phy_device` with
>>> +    /// a valid `mdio.dev`, and the caller must be in a context where all methods
>>> +    /// defined on this struct are safe to call.
>>
>> Also here.
> 
> /// # Safety
> ///
> /// For the duration of 'a,
> /// - the pointer must point at a valid `phy_device`, and the caller
> ///   must be in a context where all methods defined on this struct
> ///   are safe to call.
> /// - 'mdio.dev' must be a valid.

Also here: `(*ptr).mdio.dev`.

---
Cheers,
Benno

> 
> Better?
> 
>>> +impl AsRef<kernel::device::Device> for Device {
>>> +    fn as_ref(&self) -> &kernel::device::Device {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
>>> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
>>> +    }
>>
>> Just to be sure: the `phydev.mdio.dev` struct is refcounted and
>> incrementing the refcount is fine, right?
> 
> phydev.mdio.dev is valid after phydev is initialized.
> 
> struct phy_device {
> 	struct mdio_device mdio;
> 	...
> 
> struct mdio_device {
> 	struct device dev;
> 	...
> 

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[FUJITA Tomonori]

On Sun, 18 Aug 2024 10:42:32 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> /// - Referencing a `phy_device` using this struct asserts that you are in
>> ///   a context where all methods defined on this struct are safe to call.
>> /// - This struct always has a valid `mdio.dev`.
> 
> Hmm, I think `self.0.mdio.dev` would be clearer.

Sure, fixed.

>> /// # Safety
>> ///
>> /// For the duration of 'a,
>> /// - the pointer must point at a valid `phy_device`, and the caller
>> ///   must be in a context where all methods defined on this struct
>> ///   are safe to call.
>> /// - 'mdio.dev' must be a valid.
> 
> Also here: `(*ptr).mdio.dev`.

Thanks, looks better.

Here's an updated patch.

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 5e8137a1972f..ec337cbd391b 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -7,8 +7,7 @@
 //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
 use crate::{error::*, prelude::*, types::Opaque};
-
-use core::marker::PhantomData;
+use core::{marker::PhantomData, ptr::addr_of_mut};
 
 /// PHY state machine states.
 ///
@@ -58,8 +57,9 @@ pub enum DuplexMode {
 ///
 /// # Invariants
 ///
-/// Referencing a `phy_device` using this struct asserts that you are in
-/// a context where all methods defined on this struct are safe to call.
+/// - Referencing a `phy_device` using this struct asserts that you are in
+///   a context where all methods defined on this struct are safe to call.
+/// - This struct always has a valid `self.0.mdio.dev`.
 ///
 /// [`struct phy_device`]: srctree/include/linux/phy.h
 // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
@@ -76,9 +76,11 @@ impl Device {
     ///
     /// # Safety
     ///
-    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
-    /// and the caller must be in a context where all methods defined on this struct
-    /// are safe to call.
+    /// For the duration of 'a,
+    /// - the pointer must point at a valid `phy_device`, and the caller
+    ///   must be in a context where all methods defined on this struct
+    ///   are safe to call.
+    /// - `(*ptr).mdio.dev` must be a valid.
     unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
         // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
         let ptr = ptr.cast::<Self>();
@@ -302,6 +304,14 @@ pub fn genphy_read_abilities(&mut self) -> Result {
     }
 }
 
+impl AsRef<kernel::device::Device> for Device {
+    fn as_ref(&self) -> &kernel::device::Device {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
+        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
+    }
+}
+
 /// Defines certain other features this PHY supports (like interrupts).
 ///
 /// These flag values are used in [`Driver::FLAGS`].
-- 
2.34.1

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Andrew Lunn]

> Just to be sure: the `phydev.mdio.dev` struct is refcounted and
> incrementing the refcount is fine, right?

There is nothing special here. PHY devices are just normal Linux
devices. The device core is responsible for calling .probe() and
.remove() on a device, and these should be the first and last
operation on a device. The device core provides refcounting of the
struct device, so it should always be valid.

So i have to wounder if you are solving this at the correct
level. This should be generic to any device, so the Rust concept of a
device should be stating these guarantees, not each specific type of
device.

	Andrew

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

On Sat, Aug 17, 2024 at 09:34:13PM +0000, Benno Lossin wrote:
> On 17.08.24 20:51, Andrew Lunn wrote:
> >> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> >> +        dev.genphy_read_status::<C45>()
> >> +    }
> > 
> > Probably a dumb Rust question. Shouldn't this have a ? at the end? It
> > can return a negative error code.
> 
> `read_status` returns a `Result<u16>` and `Device::genphy_read_status`
> also returns a `Result<u16>`. In the function body we just delegate to
> the latter, so no `?` is needed. We just return the entire result.
> 
> Here is the equivalent pseudo-C code:
> 
>     int genphy_read_status(struct phy_device *dev);
>     
>     int read_status(struct phy_device *dev)
>     {
>         return genphy_read_status(dev);
>     }
> 
> There you also don't need an if for the negative error code, since it's
> just propagated.

O.K, it seems to work. But one of the things we try to think about in
the kernel is avoiding future bugs. Say sometime in the future i
extend it:

    fn read_status(dev: &mut phy::Device) -> Result<u16> {
        dev.genphy_read_status::<C45>()

        dev.genphy_read_foo()
    }

By forgetting to add the ? to dev.genphy_read_status, have i just
introduced a bug? Could i have avoided that by always having the ?
even when it is not needed?

	Andrew

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Greg KH]

On Sun, Aug 18, 2024 at 05:38:01PM +0200, Andrew Lunn wrote:
> > Just to be sure: the `phydev.mdio.dev` struct is refcounted and
> > incrementing the refcount is fine, right?
> 
> There is nothing special here. PHY devices are just normal Linux
> devices. The device core is responsible for calling .probe() and
> .remove() on a device, and these should be the first and last
> operation on a device. The device core provides refcounting of the
> struct device, so it should always be valid.
> 
> So i have to wounder if you are solving this at the correct
> level. This should be generic to any device, so the Rust concept of a
> device should be stating these guarantees, not each specific type of
> device.

It should, why isn't it using the rust binding to Device that we already
have:
	https://rust.docs.kernel.org/kernel/device/struct.Device.html
or is it?  I'm confused now...

thanks,

greg k-h

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 17:45, Greg KH wrote:
> On Sun, Aug 18, 2024 at 05:38:01PM +0200, Andrew Lunn wrote:
>>> Just to be sure: the `phydev.mdio.dev` struct is refcounted and
>>> incrementing the refcount is fine, right?
>>
>> There is nothing special here. PHY devices are just normal Linux
>> devices. The device core is responsible for calling .probe() and
>> .remove() on a device, and these should be the first and last
>> operation on a device. The device core provides refcounting of the
>> struct device, so it should always be valid.

Thanks that's good to know.

>> So i have to wounder if you are solving this at the correct
>> level. This should be generic to any device, so the Rust concept of a
>> device should be stating these guarantees, not each specific type of
>> device.
> 
> It should, why isn't it using the rust binding to Device that we already
> have:
> 	https://rust.docs.kernel.org/kernel/device/struct.Device.html
> or is it?  I'm confused now...

It is using that one.
I wanted to verify that we can use that one, since using this
implementation users can freely increment the refcount of the device
(without decrementing it before control is given back to PHYLIB). Since
that seems to be the case, all is fine.

---
Cheers,
Benno

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 13:25, FUJITA Tomonori wrote:
> On Sun, 18 Aug 2024 10:42:32 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>> /// - Referencing a `phy_device` using this struct asserts that you are in
>>> ///   a context where all methods defined on this struct are safe to call.
>>> /// - This struct always has a valid `mdio.dev`.
>>
>> Hmm, I think `self.0.mdio.dev` would be clearer.
> 
> Sure, fixed.
> 
>>> /// # Safety
>>> ///
>>> /// For the duration of 'a,
>>> /// - the pointer must point at a valid `phy_device`, and the caller
>>> ///   must be in a context where all methods defined on this struct
>>> ///   are safe to call.
>>> /// - 'mdio.dev' must be a valid.
>>
>> Also here: `(*ptr).mdio.dev`.
> 
> Thanks, looks better.
> 
> Here's an updated patch.
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 5e8137a1972f..ec337cbd391b 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -7,8 +7,7 @@
>  //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
> 
>  use crate::{error::*, prelude::*, types::Opaque};
> -
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, ptr::addr_of_mut};
> 
>  /// PHY state machine states.
>  ///
> @@ -58,8 +57,9 @@ pub enum DuplexMode {
>  ///
>  /// # Invariants
>  ///
> -/// Referencing a `phy_device` using this struct asserts that you are in
> -/// a context where all methods defined on this struct are safe to call.
> +/// - Referencing a `phy_device` using this struct asserts that you are in
> +///   a context where all methods defined on this struct are safe to call.
> +/// - This struct always has a valid `self.0.mdio.dev`.
>  ///
>  /// [`struct phy_device`]: srctree/include/linux/phy.h
>  // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> @@ -76,9 +76,11 @@ impl Device {
>      ///
>      /// # Safety
>      ///
> -    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
> -    /// and the caller must be in a context where all methods defined on this struct
> -    /// are safe to call.
> +    /// For the duration of 'a,
> +    /// - the pointer must point at a valid `phy_device`, and the caller
> +    ///   must be in a context where all methods defined on this struct
> +    ///   are safe to call.
> +    /// - `(*ptr).mdio.dev` must be a valid.
>      unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>          // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>          let ptr = ptr.cast::<Self>();
> @@ -302,6 +304,14 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
> 
> +impl AsRef<kernel::device::Device> for Device {
> +    fn as_ref(&self) -> &kernel::device::Device {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
> +        unsafe { kernel::device::Device::as_ref(addr_of_mut!((*phydev).mdio.dev)) }
> +    }
> +}
> +
>  /// Defines certain other features this PHY supports (like interrupts).
>  ///
>  /// These flag values are used in [`Driver::FLAGS`].
> --
> 2.34.1
> 

This looks, good, if you want, you can add my reviewed-by on this patch.

---
Cheers,
Benno

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Benno Lossin]

On 17.08.24 07:19, FUJITA Tomonori wrote:
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 7fddc8306d82..e0ff386d90cd 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -109,6 +109,12 @@ config ADIN1100_PHY
>  	  Currently supports the:
>  	  - ADIN1100 - Robust,Industrial, Low Power 10BASE-T1L Ethernet PHY
> 
> +config AMCC_QT2025_PHY
> +	tristate "AMCC QT2025 PHY"
> +	depends on RUST_PHYLIB_ABSTRACTIONS

This is missing a depends on RUST_FW_LOADER_ABSTRACTIONS.

---
Cheers,
Benno

> +	help
> +	  Adds support for the Applied Micro Circuits Corporation QT2025 PHY.
> +
>  source "drivers/net/phy/aquantia/Kconfig"

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Andrew Lunn]

> Thanks that's good to know.
> 
> >> So i have to wounder if you are solving this at the correct
> >> level. This should be generic to any device, so the Rust concept of a
> >> device should be stating these guarantees, not each specific type of
> >> device.
> > 
> > It should, why isn't it using the rust binding to Device that we already
> > have:
> > 	https://rust.docs.kernel.org/kernel/device/struct.Device.html
> > or is it?  I'm confused now...
> 
> It is using that one.
> I wanted to verify that we can use that one, since using this
> implementation users can freely increment the refcount of the device
> (without decrementing it before control is given back to PHYLIB). Since
> that seems to be the case, all is fine.

Any driver which is not using the device core is broken, and no amount
of SAFETY is going to fix it.

	Andrew

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Benno Lossin]

On 18.08.24 17:44, Andrew Lunn wrote:
> On Sat, Aug 17, 2024 at 09:34:13PM +0000, Benno Lossin wrote:
>> On 17.08.24 20:51, Andrew Lunn wrote:
>>>> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
>>>> +        dev.genphy_read_status::<C45>()
>>>> +    }
>>>
>>> Probably a dumb Rust question. Shouldn't this have a ? at the end? It
>>> can return a negative error code.
>>
>> `read_status` returns a `Result<u16>` and `Device::genphy_read_status`
>> also returns a `Result<u16>`. In the function body we just delegate to
>> the latter, so no `?` is needed. We just return the entire result.
>>
>> Here is the equivalent pseudo-C code:
>>
>>     int genphy_read_status(struct phy_device *dev);
>>
>>     int read_status(struct phy_device *dev)
>>     {
>>         return genphy_read_status(dev);
>>     }
>>
>> There you also don't need an if for the negative error code, since it's
>> just propagated.
> 
> O.K, it seems to work. But one of the things we try to think about in
> the kernel is avoiding future bugs. Say sometime in the future i
> extend it:
> 
>     fn read_status(dev: &mut phy::Device) -> Result<u16> {
>         dev.genphy_read_status::<C45>()
> 
>         dev.genphy_read_foo()
>     }
> 
> By forgetting to add the ? to dev.genphy_read_status, have i just
> introduced a bug? Could i have avoided that by always having the ?
> even when it is not needed?

The above code will not compile, since there is a missing `;` in the
second line. If you try to do it with the semicolon:

    fn read_status(dev: &mut phy::Device) -> Result<u16> {
        dev.genphy_read_status::<C45>();
 
        dev.genphy_read_foo()
    }

Then you get this error:

    error: unused `core::result::Result` that must be used
      --> drivers/net/phy/qt2025.rs:88:9
       |
    88 |         dev.genphy_read_status::<C45>();
       |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: this `Result` may be an `Err` variant, which should be handled
       = note: `-D unused-must-use` implied by `-D warnings`
       = help: to override `-D warnings` add `#[allow(unused_must_use)]`
    help: use `let _ = ...` to ignore the resulting value
       |
    88 |         let _ = dev.genphy_read_status::<C45>();
       |         +++++++

If you want to use `?` regardless, you will have to do this:
     
     fn read_status(dev: &mut phy::Device) -> Result<u16> {
         Ok(dev.genphy_read_status::<C45>()?)
     }

In my opinion this does not add significant protection for the scenario
that you outlined and is a lot more verbose. But if you're not used to
Rust, this might be different, since the code below looks more wrong:

    fn read_status(dev: &mut phy::Device) -> Result<u16> {
        Ok(dev.genphy_read_status::<C45>()?);
        
        dev.genphy_read_foo()
    }

But I would keep it the way it currently is.

---
Cheers,
Benno

Re: [PATCH net-next v4 3/6] rust: net::phy implement AsRef<kernel::device::Device> trait

[Benno Lossin]

On 18.08.24 18:16, Andrew Lunn wrote:
>> Thanks that's good to know.
>>
>>>> So i have to wounder if you are solving this at the correct
>>>> level. This should be generic to any device, so the Rust concept of a
>>>> device should be stating these guarantees, not each specific type of
>>>> device.
>>>
>>> It should, why isn't it using the rust binding to Device that we already
>>> have:
>>> 	https://rust.docs.kernel.org/kernel/device/struct.Device.html
>>> or is it?  I'm confused now...
>>
>> It is using that one.
>> I wanted to verify that we can use that one, since using this
>> implementation users can freely increment the refcount of the device
>> (without decrementing it before control is given back to PHYLIB). Since
>> that seems to be the case, all is fine.
> 
> Any driver which is not using the device core is broken, and no amount
> of SAFETY is going to fix it.

This is what I did not know, and I asked to ensure that we don't
introduce miscommunication with the C side. (i.e. can we rely
in our SAFETY comments that devices are always used in this way)

---
Cheers,
Benno

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> Then you get this error:
> 
>     error: unused `core::result::Result` that must be used
>       --> drivers/net/phy/qt2025.rs:88:9
>        |
>     88 |         dev.genphy_read_status::<C45>();
>        |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        |
>        = note: this `Result` may be an `Err` variant, which should be handled
>        = note: `-D unused-must-use` implied by `-D warnings`
>        = help: to override `-D warnings` add `#[allow(unused_must_use)]`
>     help: use `let _ = ...` to ignore the resulting value
>        |
>     88 |         let _ = dev.genphy_read_status::<C45>();
>        |         +++++++

O.K, so the compiler tells you, which is great. The C compiler would
not, which is why i tend to think of these things.

	Andrew

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

On Sun, 18 Aug 2024 16:10:25 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 17.08.24 07:19, FUJITA Tomonori wrote:
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 7fddc8306d82..e0ff386d90cd 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -109,6 +109,12 @@ config ADIN1100_PHY
>>  	  Currently supports the:
>>  	  - ADIN1100 - Robust,Industrial, Low Power 10BASE-T1L Ethernet PHY
>> 
>> +config AMCC_QT2025_PHY
>> +	tristate "AMCC QT2025 PHY"
>> +	depends on RUST_PHYLIB_ABSTRACTIONS
> 
> This is missing a depends on RUST_FW_LOADER_ABSTRACTIONS.

Oops, added. Thanks!

Re: [PATCH net-next v4 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Miguel Ojeda]

On Sun, Aug 18, 2024 at 7:39 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> O.K, so the compiler tells you, which is great. The C compiler would
> not, which is why i tend to think of these things.

For the unused result, there is `__must_check` of course, though it
does not seem to apply to types in GCC like it is applied to `Result`
in Rust (Clang is OK with it).

C23 brings `[[nodiscard]]`, which can be. GCC >= 11 and Clang >= 17
support it, so one can do things like:

    typedef struct [[nodiscard]] Result {
        ...
    } Result;

    Result g(void);

    Result f(void)
    {
        g(); // will warn.
        return g();
    }

https://godbolt.org/z/PqK36rjWY

And have a `Result`-like type (or, rather, like our alias in the
kernel, given the generics).

So if one is happy using new types, that is great.

It would be nice to be able to do something like:

    #define Result __must_check int

...but that faces trouble when using it for e.g. local variables.

Cheers,
Miguel