Re: phylib locking (was: Re: [REGRESSION] Re: [patch V3 09/33] genirq/msi: Add range checking) to msi_insert_desc()

[Russell King (Oracle)]

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!