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

[SeungJong Ha]

On Fri, Nov 13, 2026 at 2:02 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:

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

No. This series does not touch read_mmd_callback() and
write_mmd_callback(); their SAFETY claim predates it and is wrong in the
same way, since they run under the MDIO bus lock instead.

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

Since nothing uses them and the abstraction cannot express the MDIO bus
lock yet, I would drop them from the Driver trait in v2.

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

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

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

Neither. match_phy_device should not get the register accessors at all,
as before this series. The patch 1 findings about genphy_soft_reset() and
register writes from match_phy_device have the same root cause. Patch 1 makes
the register accessors &self, which hands them to match_phy_device even though
it can run on an unbound device or one bound to another driver. That was not
possible before, since they took &mut self.

For v2 I plan to keep &mut self on the moved methods, so match_phy_device
stays as it is, and give only soft_reset a &mut Device<MaybeLocked>.


match_phy_device is more about binding than locking. I think it belongs
with moving phy::Device onto device::DeviceContext, as pci::Device does,
rather than in this series.


Fujita, does that split make sense? And is this worth doing now, given
that no in-tree driver is affected, or would you rather wait for a real
user?

pw-bot: cr