[PATCH net-next 0/2] rust: net: phy: soft_reset and match_phy_device may run without phy_device->lock

[PATCH net-next 0/2] rust: net: phy: soft_reset and match_phy_device may run without phy_device->lock

[SeungJong Ha]

While auditing the SAFETY comments of Rust abstractions against their C
callers, and confirming the result with Clang's context analysis [1], it
turned out that PHYLIB calls `soft_reset` and `match_phy_device` both
with and without `phy_device->lock`, while the Rust side assumes it is
held.

No driver is affected, but it seems better for the abstraction to tell
the two cases apart. Patch 1 makes `Device` generic over a lock context,
as `device::Device` is, and patch 2 gives these two callbacks the
`MaybeLocked` one.

The audit, the patches and their changelogs were done with an AI coding
assistant (Claude Code, Claude Opus 5.5); the lock-less paths were then
checked with Clang 23 context analysis as described in patch 2.

[1] https://docs.kernel.org/dev-tools/context-analysis.html

---
SeungJong Ha (2):
      rust: net: phy: make Device generic over a lock context
      rust: net: phy: pass a MaybeLocked device to soft_reset and match_phy_device

 drivers/net/phy/ax88796b_rust.rs |   8 +-
 rust/kernel/net/phy.rs           | 183 ++++++++++++++++++++++++++++-----------
 rust/kernel/net/phy/reg.rs       |  17 ++--
 3 files changed, 145 insertions(+), 63 deletions(-)
---
base-commit: 2d916a8c307c31b433318509bf0311d52d509f09
change-id: 20261110-exp-phy-rs-maybelocked-ddc59993efba

Best regards,
--  
SeungJong Ha <engineer.jjhama@gmail.com>

[PATCH net-next 1/2] rust: net: phy: make Device generic over a lock context

[SeungJong Ha]

Make `Device` generic over a lock context, as `device::Device` is over
its device context: `Locked`, the default, for callbacks that run under
`phy_device->lock`, and `MaybeLocked` for callbacks that may run without
it. `Device<Locked>` derefs to `Device<MaybeLocked>`. The next patch
uses `MaybeLocked`.

Methods that do not need the lock (`phy_id`, register access,
`read_paged` and `genphy_soft_reset`) move to both contexts and take
`&self` instead of `&mut self`, since a `MaybeLocked` callback may run
concurrently with a `Locked` one on the same device. No functional
change.

Built with AX88796B_RUST_PHY=y and CLIPPY=1.

Signed-off-by: SeungJong Ha <engineer.jjhama@gmail.com>
---
 rust/kernel/net/phy.rs     | 145 ++++++++++++++++++++++++++++++++-------------
 rust/kernel/net/phy/reg.rs |  17 +++---
 2 files changed, 113 insertions(+), 49 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index c4e7b1d6c6f4..60c58bbc12d6 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -50,6 +50,48 @@ pub enum DuplexMode {
     Unknown,
 }
 
+/// A trait implemented by all contexts a [`Device`] can be used in.
+///
+/// - [`Locked`]: Serialized against every access made under `phy_device->lock`. The default
+///   context of [`Device`].
+/// - [`MaybeLocked`]: `phy_device->lock` may or may not be held.
+///
+/// [`Device<Locked>`] dereferences to [`Device<MaybeLocked>`].
+pub trait PhyContext: private::Sealed {}
+
+/// The [`Device`] context of every [`Driver`] callback that does not take a
+/// [`Device<MaybeLocked>`].
+///
+/// PHYLIB holds `phy_device->lock` for these callbacks, except for [`Driver::probe`], which runs
+/// before the device is published, and for [`Driver::suspend`] and [`Driver::resume`], which run
+/// without concurrent access to the instance.
+///
+/// # Invariants
+///
+/// For the lifetime of a [`Device<Locked>`] reference, the holder is serialized against every
+/// other access to the `phy_device` made under `phy_device->lock`.
+pub struct Locked;
+
+/// The [`Device`] context of callbacks that PHYLIB calls both with and without
+/// `phy_device->lock` held.
+///
+/// A callback in this context must not take `phy_device->lock`.
+///
+/// # Invariants
+///
+/// None beyond those of [`Device`].
+pub struct MaybeLocked;
+
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Locked {}
+    impl Sealed for super::MaybeLocked {}
+}
+
+impl PhyContext for Locked {}
+impl PhyContext for MaybeLocked {}
+
 /// An instance of a PHY device.
 ///
 /// Wraps the kernel's [`struct phy_device`].
@@ -57,11 +99,13 @@ pub enum DuplexMode {
 /// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
 /// executes [`Driver`]'s methods during the callback.
 ///
+/// `Ctx` is the [`PhyContext`] of the callback. Methods that rely on `phy_device->lock` are
+/// implemented for [`Device<Locked>`] only.
+///
 /// # 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.
-/// - This struct always has a valid `self.0.mdio.dev`.
+/// - `self.0` is a valid `phy_device`, and `self.0.mdio.dev` is a valid `struct device`.
+/// - The invariants of `Ctx` hold for the lifetime of a reference to [`Device<Ctx>`].
 ///
 /// [`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
@@ -71,7 +115,7 @@ pub enum DuplexMode {
 // [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
 // to the instance.
 #[repr(transparent)]
-pub struct Device(Opaque<bindings::phy_device>);
+pub struct Device<Ctx: PhyContext = Locked>(Opaque<bindings::phy_device>, PhantomData<Ctx>);
 
 impl Device {
     /// Creates a new [`Device`] instance from a raw pointer.
@@ -91,14 +135,6 @@ unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
         unsafe { &mut *ptr }
     }
 
-    /// Gets the id of the PHY.
-    pub fn phy_id(&self) -> u32 {
-        let phydev = self.0.get();
-        // SAFETY: The struct invariant ensures that we may access
-        // this field without additional synchronization.
-        unsafe { (*phydev).phy_id }
-    }
-
     /// Gets the state of PHY state machine states.
     pub fn state(&self) -> DeviceState {
         let phydev = self.0.get();
@@ -177,27 +213,6 @@ pub fn set_duplex(&mut self, mode: DuplexMode) {
         unsafe { (*phydev).duplex = v as c_int };
     }
 
-    /// Reads a PHY register.
-    // This function reads a hardware register and updates the stats so takes `&mut self`.
-    pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
-        reg.read(self)
-    }
-
-    /// 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.
-    pub fn read_paged(&mut self, page: u16, 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.
-        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
-
-        to_result(ret).map(|()| ret as u16)
-    }
-
     /// Resolves the advertisements into PHY settings.
     pub fn resolve_aneg_linkmode(&mut self) {
         let phydev = self.0.get();
@@ -206,14 +221,6 @@ pub fn resolve_aneg_linkmode(&mut self) {
         unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
     }
 
-    /// Executes software reset the PHY via `BMCR_RESET` bit.
-    pub fn genphy_soft_reset(&mut self) -> 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.
-        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
-    }
-
     /// Initializes the PHY.
     pub fn init_hw(&mut self) -> Result {
         let phydev = self.0.get();
@@ -276,7 +283,61 @@ pub fn genphy_read_abilities(&mut self) -> Result {
     }
 }
 
-impl AsRef<kernel::device::Device> for Device {
+impl<Ctx: PhyContext> Device<Ctx> {
+    /// Gets the id of the PHY.
+    pub fn phy_id(&self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is valid by the type invariant of `Self`. `phy_id` is set by
+        // `phy_device_create()` and never written afterwards.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Reads a PHY register.
+    // This function reads a hardware register and updates the stats.
+    pub fn read<R: reg::Register>(&self, reg: R) -> Result<u16> {
+        reg.read(self)
+    }
+
+    /// Writes a PHY register.
+    pub fn write<R: reg::Register>(&self, reg: R, val: u16) -> Result {
+        reg.write(self, val)
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&self, page: u16, 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.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
+
+        to_result(ret).map(|()| ret as u16)
+    }
+
+    /// Executes software reset the PHY via `BMCR_RESET` bit.
+    pub fn genphy_soft_reset(&self) -> 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.
+        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
+    }
+}
+
+impl core::ops::Deref for Device<Locked> {
+    type Target = Device<MaybeLocked>;
+
+    fn deref(&self) -> &Self::Target {
+        let ptr: *const Self = self;
+
+        // CAST: `Device<Locked>` and `Device<MaybeLocked>` transparently wrap the same type.
+        let ptr = ptr.cast::<Self::Target>();
+
+        // SAFETY: `ptr` was derived from `&self`, and the invariants of `MaybeLocked` are
+        // implied by those of `Locked`.
+        unsafe { &*ptr }
+    }
+}
+
+impl<Ctx: PhyContext> AsRef<kernel::device::Device> for Device<Ctx> {
     fn as_ref(&self) -> &kernel::device::Device {
         let phydev = self.0.get();
         // SAFETY: The struct invariant ensures that `mdio.dev` is valid.
diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
index 80e22c264ea8..6d27a70d6180 100644
--- a/rust/kernel/net/phy/reg.rs
+++ b/rust/kernel/net/phy/reg.rs
@@ -8,7 +8,10 @@
 //! Ethernet management interface clauses 22 and 45 register namespaces, as
 //! defined in IEEE 802.3.
 
-use super::Device;
+use super::{
+    Device,
+    PhyContext, //
+};
 use crate::{
     build_assert::build_assert,
     error::*,
@@ -44,10 +47,10 @@ pub trait Sealed {}
 /// ```
 pub trait Register: private::Sealed {
     /// Reads a PHY register.
-    fn read(&self, dev: &mut Device) -> Result<u16>;
+    fn read<C: PhyContext>(&self, dev: &Device<C>) -> Result<u16>;
 
     /// Writes a PHY register.
-    fn write(&self, dev: &mut Device, val: u16) -> Result;
+    fn write<C: PhyContext>(&self, dev: &Device<C>, val: u16) -> Result;
 
     /// Checks the link status and updates current link state.
     fn read_status(dev: &mut Device) -> Result<u16>;
@@ -104,7 +107,7 @@ pub const fn vendor_specific<const N: u8>() -> Self {
 impl private::Sealed for C22 {}
 
 impl Register for C22 {
-    fn read(&self, dev: &mut Device) -> Result<u16> {
+    fn read<C: PhyContext>(&self, dev: &Device<C>) -> 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
@@ -116,7 +119,7 @@ fn read(&self, dev: &mut Device) -> Result<u16> {
         Ok(ret as u16)
     }
 
-    fn write(&self, dev: &mut Device, val: u16) -> Result {
+    fn write<C: PhyContext>(&self, dev: &Device<C>, 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
@@ -196,7 +199,7 @@ pub fn new(devad: Mmd, regnum: u16) -> Self {
 impl private::Sealed for C45 {}
 
 impl Register for C45 {
-    fn read(&self, dev: &mut Device) -> Result<u16> {
+    fn read<C: PhyContext>(&self, dev: &Device<C>) -> 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.
@@ -206,7 +209,7 @@ fn read(&self, dev: &mut Device) -> Result<u16> {
         Ok(ret as u16)
     }
 
-    fn write(&self, dev: &mut Device, val: u16) -> Result {
+    fn write<C: PhyContext>(&self, dev: &Device<C>, 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.

-- 
2.54.0

[PATCH net-next 2/2] rust: net: phy: pass a MaybeLocked device to soft_reset and match_phy_device

[SeungJong Ha]

The SAFETY comments of `soft_reset_callback` and
`match_phy_device_callback` state that PHYLIB holds `phy_device->lock`
when it calls them. It does on some paths but not on others:
phy_init_hw() calls `soft_reset` both with and without the lock (e.g.
from phy_attach_direct() and phy_mii_ioctl()), and phy_bus_match() calls
`match_phy_device` without it.

Hand both callbacks a `&Device<MaybeLocked>` so that the type matches
what PHYLIB provides, and correct the SAFETY comments. Update
ax88796b_rust, the only Rust implementation, to the new signature; it
only does register writes and genphy_soft_reset(), so its behaviour does
not change.

This was found while auditing SAFETY comments of Rust abstractions
against their C callers, and confirmed with Clang's context analysis:
annotating the two `phy_driver` slots with `__must_hold(&phydev->lock)`
flags exactly the lock-less paths above. It was not triggered at runtime.

Built with AX88796B_RUST_PHY=y and CLIPPY=1; a `soft_reset` that calls
`start_aneg()` or `set_speed()` no longer compiles. Not tested on
hardware.

Signed-off-by: SeungJong Ha <engineer.jjhama@gmail.com>
---
 drivers/net/phy/ax88796b_rust.rs |  8 ++++----
 rust/kernel/net/phy.rs           | 44 ++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index 2d24628a4e58..bde801df305c 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -30,7 +30,7 @@
 // BMCR_RESET bit and poll for the reset bit to be cleared.
 // 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 {
+fn asix_soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
     dev.write(C22::BMCR, 0)?;
     dev.genphy_soft_reset()
 }
@@ -85,7 +85,7 @@ fn resume(dev: &mut phy::Device) -> Result {
         dev.genphy_resume()
     }
 
-    fn soft_reset(dev: &mut phy::Device) -> Result {
+    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
         asix_soft_reset(dev)
     }
 
@@ -115,7 +115,7 @@ fn resume(dev: &mut phy::Device) -> Result {
         dev.genphy_resume()
     }
 
-    fn soft_reset(dev: &mut phy::Device) -> Result {
+    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
         asix_soft_reset(dev)
     }
 }
@@ -127,7 +127,7 @@ impl Driver for PhyAX88796B {
     const NAME: &'static CStr = c"Asix Electronics AX88796B";
     const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_model_mask(0x003b1841);
 
-    fn soft_reset(dev: &mut phy::Device) -> Result {
+    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
         asix_soft_reset(dev)
     }
 }
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 60c58bbc12d6..c2dfc58dbe55 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -72,10 +72,10 @@ pub trait PhyContext: private::Sealed {}
 /// other access to the `phy_device` made under `phy_device->lock`.
 pub struct Locked;
 
-/// The [`Device`] context of callbacks that PHYLIB calls both with and without
-/// `phy_device->lock` held.
+/// The [`Device`] context of [`Driver::soft_reset`] and [`Driver::match_phy_device`].
 ///
-/// A callback in this context must not take `phy_device->lock`.
+/// PHYLIB calls these callbacks both with and without `phy_device->lock` held. A callback in this
+/// context must not take `phy_device->lock`.
 ///
 /// # Invariants
 ///
@@ -322,6 +322,22 @@ pub fn genphy_soft_reset(&self) -> Result {
     }
 }
 
+impl Device<MaybeLocked> {
+    /// Creates a new [`Device`] reference in the [`MaybeLocked`] context from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of `'a`, the pointer must point at a valid `phy_device` whose
+    /// `mdio.dev` is valid.
+    unsafe fn from_raw_maybe_locked<'a>(ptr: *mut bindings::phy_device) -> &'a Self {
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
+        let ptr = ptr.cast::<Self>();
+        // SAFETY: The pointer is valid for `'a`. A shared reference does not assert exclusive
+        // access, and `Opaque` permits concurrent mutation by the C side.
+        unsafe { &*ptr }
+    }
+}
+
 impl core::ops::Deref for Device<Locked> {
     type Target = Device<MaybeLocked>;
 
@@ -370,10 +386,9 @@ impl<T: Driver> Adapter<T> {
     /// `phydev` must be passed by the corresponding callback in `phy_driver`.
     unsafe extern "C" fn soft_reset_callback(phydev: *mut bindings::phy_device) -> c_int {
         from_result(|| {
-            // SAFETY: This callback is called only in contexts
-            // where we hold `phy_device->lock`, so the accessors on
-            // `Device` are okay to call.
-            let dev = unsafe { Device::from_raw(phydev) };
+            // SAFETY: `phydev` is valid for the callback. `phy_init_hw()` calls `soft_reset` both
+            // with and without `phy_device->lock`, which is the `MaybeLocked` context.
+            let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
             T::soft_reset(dev)?;
             Ok(0)
         })
@@ -471,10 +486,9 @@ impl<T: Driver> Adapter<T> {
         phydev: *mut bindings::phy_device,
         _phydrv: *const bindings::phy_driver,
     ) -> c_int {
-        // SAFETY: This callback is called only in contexts
-        // where we hold `phy_device->lock`, so the accessors on
-        // `Device` are okay to call.
-        let dev = unsafe { Device::from_raw(phydev) };
+        // SAFETY: `phydev` is valid for the callback. `phy_bus_match()` holds no PHY lock, which
+        // is the `MaybeLocked` context.
+        let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
         T::match_phy_device(dev).into()
     }
 
@@ -635,7 +649,9 @@ pub trait Driver {
     const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
 
     /// Issues a PHY software reset.
-    fn soft_reset(_dev: &mut Device) -> Result {
+    ///
+    /// Called in the [`MaybeLocked`] context.
+    fn soft_reset(_dev: &Device<MaybeLocked>) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -651,7 +667,9 @@ fn get_features(_dev: &mut Device) -> Result {
 
     /// Returns true if this is a suitable driver for the given phydev.
     /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
-    fn match_phy_device(_dev: &Device) -> bool {
+    ///
+    /// Called in the [`MaybeLocked`] context.
+    fn match_phy_device(_dev: &Device<MaybeLocked>) -> bool {
         false
     }
 

-- 
2.54.0

Re: [PATCH net-next 1/2] rust: net: phy: make Device generic over a lock context

[하승종]

Sashiko's review of this patch was sent off-list [1], so I am replying
here.

> Can calling genphy_soft_reset() on a Device<MaybeLocked> cause a data race
> on struct phy_device fields and bitfields when a Device<Locked> callback
> runs concurrently?

The race comes from the caller, not from this series. PHYLIB calls
soft_reset without `phydev->lock` on some paths, and on those paths
`genphy_soft_reset()` and `phy_init_hw()` itself touch these fields the same
way for every C driver. This series does not change what runs there; it
makes the Rust interface match that contract, so a soft_reset
implementation no longer gets a `&mut Device<Locked>` and cannot call the
methods that rely on the lock. Before it, the callback could do strictly
more on the same path.

> Should the methods in impl<Ctx: PhyContext> Device<Ctx>, Deref::deref(),
> AsRef::as_ref(), and the Register::read() and Register::write()
> implementations [...] have #[inline] annotations?

The moved methods did not have `#[inline]` before. This patch only moves them.
I can add it in v2 if that is preferred.

[1] https://sashiko.dev/#/patchset/20261110-exp-phy-rs-maybelocked-v1-0-3023cc9055e9@gmail.com?part=1

Re: [PATCH net-next 2/2] rust: net: phy: pass a MaybeLocked device to soft_reset and match_phy_device

[하승종]

Sashiko's review of this patch was also sent off-list [1].

> Can Adapter::read_mmd_callback() and Adapter::write_mmd_callback() also be
> entered without phy_device->lock held, while still constructing a &mut
> Device<Locked> via Device::from_raw()?

Yes, and that predates this series. read_mmd and write_mmd run under the
MDIO bus lock rather than phydev->lock, e.g. rtl822x_hwmon_read() ->
phy_read_mmd().

They need their own context for the MDIO bus lock. This series only
separates callbacks by `phydev->lock` and no in-tree Rust driver implements
`read_mmd` or `write_mmd`, so I will handle them in a follow-up.

[1] https://sashiko.dev/#/patchset/20261110-exp-phy-rs-maybelocked-v1-0-3023cc9055e9@gmail.com?part=2

2026년 11월 10일 (화) 오후 8:00, SeungJong Ha <engineer.jjhama@gmail.com>님이 작성:
>
> The SAFETY comments of `soft_reset_callback` and
> `match_phy_device_callback` state that PHYLIB holds `phy_device->lock`
> when it calls them. It does on some paths but not on others:
> phy_init_hw() calls `soft_reset` both with and without the lock (e.g.
> from phy_attach_direct() and phy_mii_ioctl()), and phy_bus_match() calls
> `match_phy_device` without it.
>
> Hand both callbacks a `&Device<MaybeLocked>` so that the type matches
> what PHYLIB provides, and correct the SAFETY comments. Update
> ax88796b_rust, the only Rust implementation, to the new signature; it
> only does register writes and genphy_soft_reset(), so its behaviour does
> not change.
>
> This was found while auditing SAFETY comments of Rust abstractions
> against their C callers, and confirmed with Clang's context analysis:
> annotating the two `phy_driver` slots with `__must_hold(&phydev->lock)`
> flags exactly the lock-less paths above. It was not triggered at runtime.
>
> Built with AX88796B_RUST_PHY=y and CLIPPY=1; a `soft_reset` that calls
> `start_aneg()` or `set_speed()` no longer compiles. Not tested on
> hardware.
>
> Signed-off-by: SeungJong Ha <engineer.jjhama@gmail.com>
> ---
>  drivers/net/phy/ax88796b_rust.rs |  8 ++++----
>  rust/kernel/net/phy.rs           | 44 ++++++++++++++++++++++++++++------------
>  2 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
> index 2d24628a4e58..bde801df305c 100644
> --- a/drivers/net/phy/ax88796b_rust.rs
> +++ b/drivers/net/phy/ax88796b_rust.rs
> @@ -30,7 +30,7 @@
>  // BMCR_RESET bit and poll for the reset bit to be cleared.
>  // 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 {
> +fn asix_soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
>      dev.write(C22::BMCR, 0)?;
>      dev.genphy_soft_reset()
>  }
> @@ -85,7 +85,7 @@ fn resume(dev: &mut phy::Device) -> Result {
>          dev.genphy_resume()
>      }
>
> -    fn soft_reset(dev: &mut phy::Device) -> Result {
> +    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
>          asix_soft_reset(dev)
>      }
>
> @@ -115,7 +115,7 @@ fn resume(dev: &mut phy::Device) -> Result {
>          dev.genphy_resume()
>      }
>
> -    fn soft_reset(dev: &mut phy::Device) -> Result {
> +    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
>          asix_soft_reset(dev)
>      }
>  }
> @@ -127,7 +127,7 @@ impl Driver for PhyAX88796B {
>      const NAME: &'static CStr = c"Asix Electronics AX88796B";
>      const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_model_mask(0x003b1841);
>
> -    fn soft_reset(dev: &mut phy::Device) -> Result {
> +    fn soft_reset(dev: &phy::Device<phy::MaybeLocked>) -> Result {
>          asix_soft_reset(dev)
>      }
>  }
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 60c58bbc12d6..c2dfc58dbe55 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -72,10 +72,10 @@ pub trait PhyContext: private::Sealed {}
>  /// other access to the `phy_device` made under `phy_device->lock`.
>  pub struct Locked;
>
> -/// The [`Device`] context of callbacks that PHYLIB calls both with and without
> -/// `phy_device->lock` held.
> +/// The [`Device`] context of [`Driver::soft_reset`] and [`Driver::match_phy_device`].
>  ///
> -/// A callback in this context must not take `phy_device->lock`.
> +/// PHYLIB calls these callbacks both with and without `phy_device->lock` held. A callback in this
> +/// context must not take `phy_device->lock`.
>  ///
>  /// # Invariants
>  ///
> @@ -322,6 +322,22 @@ pub fn genphy_soft_reset(&self) -> Result {
>      }
>  }
>
> +impl Device<MaybeLocked> {
> +    /// Creates a new [`Device`] reference in the [`MaybeLocked`] context from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of `'a`, the pointer must point at a valid `phy_device` whose
> +    /// `mdio.dev` is valid.
> +    unsafe fn from_raw_maybe_locked<'a>(ptr: *mut bindings::phy_device) -> &'a Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: The pointer is valid for `'a`. A shared reference does not assert exclusive
> +        // access, and `Opaque` permits concurrent mutation by the C side.
> +        unsafe { &*ptr }
> +    }
> +}
> +
>  impl core::ops::Deref for Device<Locked> {
>      type Target = Device<MaybeLocked>;
>
> @@ -370,10 +386,9 @@ impl<T: Driver> Adapter<T> {
>      /// `phydev` must be passed by the corresponding callback in `phy_driver`.
>      unsafe extern "C" fn soft_reset_callback(phydev: *mut bindings::phy_device) -> c_int {
>          from_result(|| {
> -            // SAFETY: This callback is called only in contexts
> -            // where we hold `phy_device->lock`, so the accessors on
> -            // `Device` are okay to call.
> -            let dev = unsafe { Device::from_raw(phydev) };
> +            // SAFETY: `phydev` is valid for the callback. `phy_init_hw()` calls `soft_reset` both
> +            // with and without `phy_device->lock`, which is the `MaybeLocked` context.
> +            let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
>              T::soft_reset(dev)?;
>              Ok(0)
>          })
> @@ -471,10 +486,9 @@ impl<T: Driver> Adapter<T> {
>          phydev: *mut bindings::phy_device,
>          _phydrv: *const bindings::phy_driver,
>      ) -> c_int {
> -        // SAFETY: This callback is called only in contexts
> -        // where we hold `phy_device->lock`, so the accessors on
> -        // `Device` are okay to call.
> -        let dev = unsafe { Device::from_raw(phydev) };
> +        // SAFETY: `phydev` is valid for the callback. `phy_bus_match()` holds no PHY lock, which
> +        // is the `MaybeLocked` context.
> +        let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
>          T::match_phy_device(dev).into()
>      }
>
> @@ -635,7 +649,9 @@ pub trait Driver {
>      const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
>
>      /// Issues a PHY software reset.
> -    fn soft_reset(_dev: &mut Device) -> Result {
> +    ///
> +    /// Called in the [`MaybeLocked`] context.
> +    fn soft_reset(_dev: &Device<MaybeLocked>) -> Result {
>          build_error!(VTABLE_DEFAULT_ERROR)
>      }
>
> @@ -651,7 +667,9 @@ fn get_features(_dev: &mut Device) -> Result {
>
>      /// Returns true if this is a suitable driver for the given phydev.
>      /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
> -    fn match_phy_device(_dev: &Device) -> bool {
> +    ///
> +    /// Called in the [`MaybeLocked`] context.
> +    fn match_phy_device(_dev: &Device<MaybeLocked>) -> bool {
>          false
>      }
>
>
> --
> 2.54.0
>

Re: [PATCH net-next 1/2] rust: net: phy: make Device generic over a lock context

[Andrew Lunn]

On Wed, Nov 11, 2026 at 10:02:04PM +0900, 하승종 wrote:
> Sashiko's review of this patch was sent off-list [1], so I am replying
> here.

The netdev's own LLM reviews are better than Sashiko, so we tend to
ignore Sashiko. The netdev one also does Cc: netdev.

> 
> > Can calling genphy_soft_reset() on a Device<MaybeLocked> cause a data race
> > on struct phy_device fields and bitfields when a Device<Locked> callback
> > runs concurrently?
> 
> The race comes from the caller, not from this series.

You might find the netdev LLM reports this as an existing issue. Lets
wait and see.

     Andrew

Re: [PATCH net-next 1/2] rust: net: phy: make Device generic over a lock context

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch moves `genphy_soft_reset()` into `impl<Ctx:
  PhyContext> Device<Ctx>` (rust/kernel/net/phy.rs:317) and makes it a…
- [Medium] The new `Locked` documentation and `# Invariants`
  (rust/kernel/net/phy.rs:62-73) say that PHYLIB holds…
- [Medium] Before this patch, every Rust MDIO access needed `&mut
  Device`, which only Locked or exclusive callbacks hold.
- [Low] `Device::from_raw()` (rust/kernel/net/phy.rs:130) still returns
  `&'a mut Device<Locked>`.
- [Low] rust: net: phy: missing #[inline] on small Device and Register
  wrappers

--

rust: net: phy: make Device generic over a lock context

This commit makes the Rust phy Device generic over a lock context. Locked
is the default, and MaybeLocked is for callbacks that may run without
phy_device->lock. Device<Locked> derefs to Device<MaybeLocked>, and
phy_id(), register access, read_paged() and genphy_soft_reset() move to
both contexts and take &self instead of &mut self.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index c4e7b1d6c6f4a..60c58bbc12d6d 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -50,6 +50,48 @@ pub enum DuplexMode {
>      Unknown,
>  }
>  

[ ... ]

> +/// The [`Device`] context of every [`Driver`] callback that does not take a
> +/// [`Device<MaybeLocked>`].
> +///
> +/// PHYLIB holds `phy_device->lock` for these callbacks, except for [`Driver::probe`], which runs
> +/// before the device is published, and for [`Driver::suspend`] and [`Driver::resume`], which run
> +/// without concurrent access to the instance.
> +///
> +/// # Invariants
> +///
> +/// For the lifetime of a [`Device<Locked>`] reference, the holder is serialized against every
> +/// other access to the `phy_device` made under `phy_device->lock`.
> +pub struct Locked;

[Severity: Medium]
Is this invariant accurate for Driver::read_mmd and Driver::write_mmd?

read_mmd_callback() and write_mmd_callback() still build a
&mut Device<Locked> through Device::from_raw(). The C side calls them with
only the MDIO bus lock held:

drivers/net/phy/phy-core.c:phy_read_mmd() {
	phy_lock_mdio_bus(phydev);
	ret = __phy_read_mmd(phydev, devad, regnum);
	phy_unlock_mdio_bus(phydev);
}

drivers/net/phy/phy-core.c:__phy_read_mmd() {
	...
	if (phydev->drv && phydev->drv->read_mmd)
		return phydev->drv->read_mmd(phydev, devad, regnum);
	...
}

phy_probe() also calls get_features without phydev->lock, and
get_features is not in the exception list here either.

This patch also makes C45::read() and C45::write() in reg.rs generic over
PhyContext. A &Device<MaybeLocked> holder can now reach a bound Rust
driver's read_mmd_callback(). With the follow-up patch, the path would be:

match_phy_device (MaybeLocked, unlocked)
  dev.read(C45::new(..))
    phy_read_mmd()
      read_mmd_callback()
        Device::from_raw() -> &mut Device<Locked>

Could safe code in that read_mmd then call Locked-only mutators such as
set_speed() while the bound driver's read_status runs under
phydev->lock?

The follow-up patch "rust: net: phy: pass a MaybeLocked device to
soft_reset and match_phy_device" fixes how this commit classifies
match_phy_device and soft_reset. The read_mmd and write_mmd case is still
there after it.

> @@ -71,7 +115,7 @@ pub enum DuplexMode {
>  // [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
>  // to the instance.
>  #[repr(transparent)]
> -pub struct Device(Opaque<bindings::phy_device>);
> +pub struct Device<Ctx: PhyContext = Locked>(Opaque<bindings::phy_device>, PhantomData<Ctx>);
>  
>  impl Device {
>      /// Creates a new [`Device`] instance from a raw pointer.
> @@ -91,14 +135,6 @@ unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>          unsafe { &mut *ptr }
>      }

[Severity: Low]
Does the safety reasoning in from_raw() still hold? It still returns
&'a mut Device<Locked>, and its SAFETY comment still claims "we have
unique access for the duration of 'a".

The commit message says a MaybeLocked callback "may run concurrently with
a Locked one on the same device". With the follow-up patch, a
&Device<MaybeLocked> and a &mut Device<Locked> for the same phy_device can
be live at the same time on different threads.

For example, a newly registered driver's match_phy_device can run through
__driver_attach() while the bound driver's read_status holds the &mut.

Opaque prevents noalias here, so this may have no runtime effect. Should
the from_raw() safety comment be updated to explain why &mut is still
acceptable? The SAFETY comments in set_speed() and set_duplex() also still
say the field is accessed "without additional synchronization".

[ ... ]

> @@ -276,7 +283,61 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
>  
> -impl AsRef<kernel::device::Device> for Device {
> +impl<Ctx: PhyContext> Device<Ctx> {

[ ... ]

> +    /// Reads a PHY register.
> +    // This function reads a hardware register and updates the stats.
> +    pub fn read<R: reg::Register>(&self, reg: R) -> Result<u16> {
> +        reg.read(self)
> +    }
> +
> +    /// Writes a PHY register.
> +    pub fn write<R: reg::Register>(&self, reg: R, val: u16) -> Result {
> +        reg.write(self, val)
> +    }

[Severity: Medium]
Can MaybeLocked register writes interleave with a multi-access sequence in
a Locked callback on the same PHY?

Before this patch, every Rust MDIO access needed &mut Device. For Rust
drivers, phydev->lock therefore serialized multi-register sequences.
C22::write() and C45::write() hold bus->mdio_lock for only one
transaction.

The Rust Driver vtable has no read_page or write_page. Rust drivers have
to open-code page-select sequences (for example through
C22::vendor_specific::<0x1f>()) or indirect MMD sequences with write() and
read().

With the follow-up patch, match_phy_device is MaybeLocked. It runs from
__driver_attach()->phy_bus_match() on devices that are already bound.

Suppose it writes a page-select or indirect-address register while the
bound driver's read_status or config_aneg is partway through a paged
access. Would the Locked callback then access a register on the wrong
page?

A concurrent BMCR write could also break ordered sequences such as
asix_soft_reset(), which writes BMCR = 0 and then BMCR_RESET. Should the
MaybeLocked documentation at least say that register accesses in that
context must not change persistent paging or indirect state?

[ ... ]

> +    /// Executes software reset the PHY via `BMCR_RESET` bit.
> +    pub fn genphy_soft_reset(&self) -> 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.
> +        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
> +    }

[Severity: Medium]
The commit message lists genphy_soft_reset among the "Methods that do not
need the lock". Does the C function really do only MDIO accesses?

drivers/net/phy/phy_device.c:genphy_soft_reset() {
	...
	if (phydev->autoneg == AUTONEG_ENABLE)
		res |= BMCR_ANRESTART;
	...
	phydev->suspended = 0;
	...
	if (phydev->autoneg == AUTONEG_DISABLE)
		ret = genphy_setup_forced(phydev);
	...
}

genphy_setup_forced() also writes phydev->pause and phydev->asym_pause. It
builds BMCR from phydev->speed and phydev->duplex.

In struct phy_device, suspended:1 is in the same run of bitfields as
autoneg, link, autoneg_complete, interrupts, irq_suspended and irq_rerun.
Setting suspended is a non-atomic read-modify-write of that shared storage.

With the follow-up patch, match_phy_device gets a &Device<MaybeLocked> and
can call dev.genphy_soft_reset() from safe code.

phy_bus_match() calls match_phy_device without any PHY lock.
__driver_attach() calls driver_match_device() for every device on the bus.
That includes devices already bound, where phy_state_machine() runs under
phydev->lock.

Could the suspended = 0 store then race with genphy_update_link() or
genphy_read_status() updating link, autoneg_complete, speed and duplex?
It could also race with the IRQ path updating irq_suspended and
irq_rerun.

Before this patch, the reset needed &mut Device and match_phy_device could
not call it. Nothing later in the series seems to change this.

The Rust abstraction already treats these fields as lock-protected:
set_speed() and set_duplex() are Locked-only. Should genphy_soft_reset()
also stay in the Locked-only impl?

[ ... ]

> @@ -91,14 +135,6 @@ unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>          unsafe { &mut *ptr }
>      }
>  
> -    /// Gets the id of the PHY.
> -    pub fn phy_id(&self) -> u32 {
> -        let phydev = self.0.get();
> -        // SAFETY: The struct invariant ensures that we may access
> -        // this field without additional synchronization.
> -        unsafe { (*phydev).phy_id }
> -    }
> -
>      /// Gets the state of PHY state machine states.
>      pub fn state(&self) -> DeviceState {
>          let phydev = self.0.get();

[Severity: Low]
This is a nit. These are kernel crate abstractions, and the Rust
guidelines ask for #[inline] on small functions and on thin wrappers
that only forward to a binding call. None of the functions this patch
touches have it:

- phy_id(), read(), write(), read_paged() and genphy_soft_reset() in
  the new impl<Ctx: PhyContext> Device<Ctx> block
- deref() in the new Deref impl for Device<Locked>
- as_ref() in the AsRef<kernel::device::Device> impl
- the C22 and C45 Register::read() and Register::write()
  implementations in reg.rs

Most of these are now generic over Ctx (or C). They get instantiated
in the driver crate anyway, so the practical effect is small.

deref() is the one that matters most. It is a non-generic pointer cast
that runs whenever a &Device<Locked> is coerced to
&Device<MaybeLocked>, and it should compile away entirely.

This patch already moves or rewrites all of these functions. Could
#[inline] be added to them while they are being touched?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-exp-phy-rs-maybelocked-v1-0-3023cc9055e9%40gmail.com

Re: [PATCH net-next 2/2] rust: net: phy: pass a MaybeLocked device to soft_reset and match_phy_device

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [High] Safe Rust code in `Driver::match_phy_device` can now oops the
  kernel by calling `read_paged()`.
- [Low] The rewritten `MaybeLocked` doc says "PHYLIB calls these
  callbacks [Driver::soft_reset and Driver::match_phy_device] both with…

--

rust: net: phy: pass a MaybeLocked device to soft_reset and match_phy_device

This patch changes the Rust PHY abstraction so that the soft_reset and
match_phy_device callbacks receive a &Device<MaybeLocked> instead of a
Locked device. It also corrects the SAFETY comments that said PHYLIB holds
phy_device->lock for these callbacks. ax88796b_rust is updated to the new
signature.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 60c58bbc12d6d..c2dfc58dbe55f 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -72,10 +72,10 @@ pub trait PhyContext: private::Sealed {}
>  /// other access to the `phy_device` made under `phy_device->lock`.
>  pub struct Locked;
>  
> -/// The [`Device`] context of callbacks that PHYLIB calls both with and without
> -/// `phy_device->lock` held.
> +/// The [`Device`] context of [`Driver::soft_reset`] and [`Driver::match_phy_device`].
>  ///
> -/// A callback in this context must not take `phy_device->lock`.
> +/// PHYLIB calls these callbacks both with and without `phy_device->lock` held. A callback in this
> +/// context must not take `phy_device->lock`.

[Severity: Low]
Is this accurate for match_phy_device? Its only caller is phy_bus_match(),
and that never holds phydev->lock:

drivers/net/phy/phy_device.c:phy_bus_match() {
    ...
	if (phydrv->match_phy_device)
		return phydrv->match_phy_device(phydev, phydrv);
    ...
}

The new SAFETY comment in match_phy_device_callback() further down in this
patch says "phy_bus_match() holds no PHY lock". So the type doc and the
callback's SAFETY comment describe the same callback differently.

Could the doc say that soft_reset is called both with and without the lock,
and match_phy_device always without it?

[ ... ]

> @@ -370,10 +386,9 @@ impl<T: Driver> Adapter<T> {
>      /// `phydev` must be passed by the corresponding callback in `phy_driver`.
>      unsafe extern "C" fn soft_reset_callback(phydev: *mut bindings::phy_device) -> c_int {
>          from_result(|| {
> -            // SAFETY: This callback is called only in contexts
> -            // where we hold `phy_device->lock`, so the accessors on
> -            // `Device` are okay to call.
> -            let dev = unsafe { Device::from_raw(phydev) };
> +            // SAFETY: `phydev` is valid for the callback. `phy_init_hw()` calls `soft_reset` both
> +            // with and without `phy_device->lock`, which is the `MaybeLocked` context.
> +            let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
>              T::soft_reset(dev)?;
>              Ok(0)
>          })

[Severity: High]
Is this conversion complete? read_mmd_callback() and write_mmd_callback()
in rust/kernel/net/phy.rs still say "This callback is called only in
contexts where we hold phy_device->lock", and they still build a Locked
device:

rust/kernel/net/phy.rs:Adapter::read_mmd_callback() {
    ...
            let dev = unsafe { Device::from_raw(phydev) };
            // CAST: the C side verifies devnum < 32.
            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
    ...
}

phy_read_mmd() and phy_write_mmd() take only the MDIO bus lock. Then
__phy_read_mmd() and __phy_write_mmd() dispatch to the driver:

drivers/net/phy/phy-core.c:__phy_read_mmd() {
    ...
	if (phydev->drv && phydev->drv->read_mmd)
		return phydev->drv->read_mmd(phydev, devad, regnum);
    ...
}

<C45 as Register>::read() and write() in rust/kernel/net/phy/reg.rs are
now generic over PhyContext. That means the new MaybeLocked soft_reset can
get back to a Locked device:

phy_attach_direct() or phy_mii_ioctl()   (no phydev->lock)
  phy_init_hw()
    drv->soft_reset()
      soft_reset_callback()
        T::soft_reset(&Device<MaybeLocked>)
          dev.read(C45::new(...))
            phy_read_mmd()               (MDIO bus lock only)
              __phy_read_mmd()
                read_mmd_callback()
                  T::read_mmd(&mut Device<Locked>)

>From there, T::read_mmd() can call start_aneg(), set_speed(),
set_duplex() and so on without phy_device->lock, racing with
phy_state_machine(). The commit message says a soft_reset can no longer
reach these Locked-only operations. This path also creates a &mut Device
that aliases the still-live &Device<MaybeLocked>.

The series also opens a second lock-less entry. Before the series,
match_phy_device got only &Device and could not call read() or write(),
which took &mut self. Now it can issue C45 reads. On an already-bound
device, those go to the bound Rust driver's read_mmd_callback() with no
lock held.

The Locked type doc says PHYLIB holds phy_device->lock for every callback
except probe, suspend, resume and the MaybeLocked ones. That would also be
wrong for read_mmd and write_mmd.

No in-tree Rust PHY driver implements read_mmd or write_mmd today, so
nothing triggers this yet.

Should read_mmd and write_mmd also move to a MaybeLocked-style context?

> @@ -471,10 +486,9 @@ impl<T: Driver> Adapter<T> {
>          phydev: *mut bindings::phy_device,
>          _phydrv: *const bindings::phy_driver,
>      ) -> c_int {
> -        // SAFETY: This callback is called only in contexts
> -        // where we hold `phy_device->lock`, so the accessors on
> -        // `Device` are okay to call.
> -        let dev = unsafe { Device::from_raw(phydev) };
> +        // SAFETY: `phydev` is valid for the callback. `phy_bus_match()` holds no PHY lock, which
> +        // is the `MaybeLocked` context.
> +        let dev = unsafe { Device::from_raw_maybe_locked(phydev) };
>          T::match_phy_device(dev).into()
>      }

[Severity: High]
Can safe code in a Rust match_phy_device dereference a NULL phydev->drv
here? Device::read_paged() is now a &self method for every context, so a
match_phy_device implementation can call it on &Device<MaybeLocked>:

Device::read_paged()
  phy_read_paged()
    phy_select_page()
      phy_save_page()
        phy_lock_mdio_bus()
        __phy_read_page()

drivers/net/phy/phy-core.c:__phy_read_page() {
	if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not available, PHY driver not loaded?\n"))
    ...
}

match_phy_device runs from phy_bus_match() before phy_probe() sets
phydev->drv = phydrv. phydev is allocated zeroed, and phy_remove() resets
drv to NULL.

So for an unbound PHY, this would oops with the MDIO bus mdio_lock held.
Every other PHY on that bus would then hang.

This wasn't reachable before the series: read_paged() took &mut self, and
match_phy_device only got &Device. The previous patch, "rust: net: phy:
make Device generic over a lock context", made read_paged() a &self method
for all contexts, and this patch makes MaybeLocked the match_phy_device
context.

The read_paged() SAFETY comment ("just an FFI call") also does not cover
the requirement of phy_read_paged() that phydev->drv is non-NULL.

C drivers that need paged access during match open-code the page select
instead, for example rtlgen_supports_2_5gbps() in
drivers/net/phy/realtek/realtek_main.c.

Should read_paged() be limited to contexts where phydev->drv is known to
be set, or should match_phy_device get a narrower context?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-exp-phy-rs-maybelocked-v1-0-3023cc9055e9%40gmail.com