[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

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 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

[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 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
>