Messages in this thread |  | | Date | Sun, 12 Apr 2026 13:55:26 +0100 | | From | "Russell King (Oracle)" <> | | Subject | Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path |
| |
On Sun, Apr 12, 2026 at 12:05:06PM +0000, Biju Das wrote: > Hi Russell King, > > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: 11 April 2026 17:47 > > Subject: Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path > > > > On Sat, Apr 11, 2026 at 03:50:13PM +0200, Andrew Lunn wrote: > > > > 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. > > > > > > Agreed. This patchset should cleanup these locks. > > > > > > We also need to look at lan937x_dsp_workaround(). I also don't see > > > what that mutex lock/unlock is protecting. Accessing bank registers > > > need to be protected, so doing one additional access within that > > > should not need additional protection. > > > > Looking at access_ereg(), shouldn't it be taking the MDIO bus lock and using the __phy_* accessors > > anyway because it's writing various registers which determine what is being read via the > > LAN87XX_EXT_REG_RD_DATA register or the value written via the LAN87XX_EXT_REG_WR_DATA register. > > > > Also, as it has access_ereg_modify_changed(), that entire sequence needs to take the MDIO bus lock to > > safely do the read-modify-write. > > > > Then there's lan87xx_config_rgmii_delay() which is a large open coded read-modify-write for the > > PHYACC_ATTR_BANK_MISC, LAN87XX_CTRL_1 register. > > > > To me, this looks like a racy driver, and it also looks like it's using the wrong lock to try and > > protect hardware accesses. > > OK, will replace it with MDIO bus lock.
Remember that the phy_* accessors will take the MDIO bus lock, so will need to be changed to their __phy_* counterparts.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
|  |