Messages in this thread |  | | From | Biju Das <> | | Subject | RE: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path | | Date | Sun, 12 Apr 2026 12:05:06 +0000 |
| |
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.
Cheers, Biju
|  |