lkml.org 
[lkml]   [2023]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 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!

\
 
 \ /
  Last update: 2023-03-27 00:30    [W:0.126 / U:1.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site