Messages in this thread Patch in this message | | | Date | Tue, 21 Feb 2023 14:57:44 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: phylib locking (was: Re: [REGRESSION] Re: [patch V3 09/33] genirq/msi: Add range checking) to msi_insert_desc() |
| |
On Mon, Feb 20, 2023 at 08:15:59PM +0000, Russell King (Oracle) wrote: > [dropped most on the Cc as this has probably deviated off topic for > them... and changed the subject] > > On Mon, Feb 20, 2023 at 08:43:44PM +0100, Andrew Lunn wrote: > > On Mon, Feb 20, 2023 at 07:17:11PM +0000, Russell King (Oracle) wrote: > > > On Mon, Feb 20, 2023 at 06:29:33PM +0000, Marc Zyngier wrote: > > > > Lockdep also reports[1] a possible circular locking dependency between > > > > phy_attach_direct() and rtnetlink_rcv_msg(), which looks interesting. > > > > > > > > [1] https://paste.debian.net/1271454/ > > > > > > Adding Andrew, but really this should be in a separate thread, since > > > this has nothing to do with MSI. > > > > > > It looks like the open path takes the RTNL lock followed by the phydev > > > lock, whereas the PHY probe path takes the phydev lock, and then if > > > there's a SFP attached to the PHY, we end up taking the RTNL lock. > > > That's going to be utterly horrid to try and solve, and isn't going > > > to be quick to fix. > > > > What are we actually trying to protect in phy_probe() when we take the > > lock and call phydev->drv->probe(phydev) ? > > > > The main purpose of the lock is to protect members of phydev, such as > > link, speed, duplex, which can be inconsistent when the lock is not > > held. But the PHY is not attached to a MAC yet, so a MAC cannot be > > using it, and those members of phydev are not valid yet anyway. > > > > The lock also prevents parallel operation on the device by phylib, but > > i cannot think of how that could happen at this early stage in the > > life of the PHY. > > > > So maybe we can move the mutex_lock() after the call to > > phydev->drv->probe()? > > That's what I've been thinking too - I dug back in the history, and > it was a spin_lock_bh(), and before that it was a spin_lock(). > > The patch that converted it to a spin_lock_bh() is a brilliant > example of a poor commit message "Lock debugging finds a problem" > but doesn't say _what_ the problem was! Going back further still, the > spin_lock() was there from the very beginnings of PHYLIB. So the > reasoning for having a lock here has been lost in the depths of time. > > The lock certainly doesn't prevent any interaction with > phy_attach_direct(), so it seems to be utterly pointless to take > the lock in the probe() function. > > So yes, I agree, we can move the lock - and I wonder whether we > could just get rid of it completely in phy_probe().
Thinking about this more, I think taking phydev->lock in both phy_probe() and phy_remove() are both entirely pointless, so I think we should remove both and be done with this. As I note above, it does nothing to stop a race between phy_attach_direct() and phy_probe() or even phy_remove(). So, I think this is entirely sensible:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 71becceb8764..b46a074b27e4 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3098,8 +3098,6 @@ static int phy_probe(struct device *dev) if (phydrv->flags & PHY_IS_INTERNAL) phydev->is_internal = true; - mutex_lock(&phydev->lock); - /* Deassert the reset signal */ phy_device_reset(phydev, 0); @@ -3173,8 +3171,6 @@ static int phy_probe(struct device *dev) if (err) phy_device_reset(phydev, 1); - mutex_unlock(&phydev->lock); - return err; } @@ -3184,9 +3180,7 @@ static int phy_remove(struct device *dev) cancel_delayed_work_sync(&phydev->state_queue); - mutex_lock(&phydev->lock); phydev->state = PHY_DOWN; - mutex_unlock(&phydev->lock); sfp_bus_del_upstream(phydev->sfp_bus); phydev->sfp_bus = NULL; -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |