Date:   Mon, 20 Feb 2023 20:15:59 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Marc Zyngier <maz@kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: phylib locking (was: Re: [REGRESSION] Re: [patch V3 09/33]
 genirq/msi: Add range checking) to msi_insert_desc()
Message-ID: <Y/PU/9zTLxIJyWkz@shell.armlinux.org.uk>
References: <20221124230505.073418677@linutronix.de>
 <20221124232325.798556374@linutronix.de>
 <Y/Opu6ETe3ZzZ/8E@shell.armlinux.org.uk>
 <86fsb0xkaa.wl-maz@kernel.org>
 <Y/PHN+0nETV3o1pQ@shell.armlinux.org.uk>
 <Y/PNcLACXSN2X/F/@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y/PNcLACXSN2X/F/@lunn.ch>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

[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!