Messages in this thread | | | Date | Mon, 20 Feb 2023 20:15:59 +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() |
| |
[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().
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |