Re: Lockup in phy_probe() for MDIO device (Broadcom's switch)

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Sep 30, 2021 at 12:30:52PM +0200, Rafał Miłecki wrote:
> On 30.09.2021 12:17, Russell King (Oracle) wrote:
> > On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
> > > This isn't necessarily a PHY / MDIO regression. It could be some core
> > > change that exposed a PHY / MDIO bug.
> > 
> > I think what's going on is that the switch device is somehow being
> > probed by phylib. It looks to me like we don't check that the mdio
> > device being matched in phy_bus_match() is actually a PHY (by
> > checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
> > before proceeding with any matching.)
> > 
> > We do, however, check the driver side. This looks to me like a problem
> > especially when the mdio bus can contain a mixture of PHY devices and
> > non-PHY devices. However, I would expect this to also be blowing up in
> > the mainline kernel as well - but it doesn't seem to.
> > 
> > Maybe Andrew can provide a reason why this doesn't happen - maybe we've
> > just been lucky with out-of-bounds read accesses (to the non-existent
> > phy_device wrapped around the mdio_device?)
> 
> I'll see if I can use buildroot to test unmodified kernel.
> 
> 
> > If my theory is correct, this patch should solve your issue:
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index ba5ad86ec826..dac017174ab1 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -462,7 +462,8 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
> >   	const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
> >   	int i;
> > -	if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY))
> > +	if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY) ||
> > +	    !(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY))
> >   		return 0;
> >   	if (phydrv->match_phy_device)
> > 
> 
> Unfortunately this doesn't seem to help

Hmm.

In phy_probe, can you add:

	WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));

just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.

MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.

Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.

Could it be that openwrt is carrying a patch that is causing this
issue?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!