Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path

[Russell King (Oracle)]

On Fri, Apr 10, 2026 at 04:41:08PM +0000, Biju Das wrote:
> Hi Andrew,
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: 10 April 2026 16:15
> > Subject: Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path
> > 
> > > Apart from that, looks fine to me - it seems some paths call
> > > phy_init_hw() can be called with or without phydev->lock held, and
> > > this one will call it with the lock held which seems to be okay.
> > 
> > Haven't we had deadlocks in this area before?
> > 
> > Please test with CONFIG_PROVE_LOCKING enabled.
> 
> I have n't faced any issue with micrel phy. But my collegue
> got the below issue with Microsemi phy. It doesn't finish the boot.
> 
> drivers/net/phy/mscc/mscc_main.c 

Looking at this driver, I'm wondering why it's taking phydev->lock
in vsc85xx_edge_rate_cntl_set()... phy_modify_paged() is already
a fully locked atomic operation (it takes the bus lock) and taking
phydev->lock gains nothing.

vsc85xx_mac_if_set() is a different matter, and this _should_ be
using phy_modify() to atomically change MSCC_PHY_EXT_PHY_CNTL_1.
phydev->lock doesn't guarantee that e.g. userspace won't access
the register behind this code's back.

vsc8531_pre_init_seq_set() is a repeat of vsc85xx_edge_rate_cntl_set()
except with phy_select_page()..phy_restore_page() which does the
necessary bus locking to ensure the entire sequence is done atomically.
Ditto vsc85xx_eee_init_seq_set().

So, I question whether any of the functions in this driver actually
have a valid reason to take phydev->lock - looks to me like a not
very well written driver.

In cases like this, I don't think we should make things more
difficult in the core just because we have a lockdep splat when that
can be avoided by killing off unnecessary locking.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!