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

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

On Thu, Sep 30, 2021 at 03:42:38PM +0200, Rafał Miłecki wrote:
> On 30.09.2021 15:07, Russell King (Oracle) wrote:
> > On Thu, Sep 30, 2021 at 02:51:40PM +0200, Rafał Miłecki wrote:
> > > On 30.09.2021 14:30, Russell King (Oracle) wrote:
> > > > > It's actually OpenWrt's downstream swconfig-based b53 driver that
> > > > > matches this device.
> > > > > 
> > > > > I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
> > > > > does it match MDIO device then? I thought MDIO devices should be
> > > > > matches only with drivers using mdio_driver_register().
> > > > 
> > > > Note that I've no idea what he swconfig-based b53 driver looks like,
> > > > I don't have the source for that to hand.
> > > > 
> > > > If it calls phy_driver_register(), then it is registering a driver for
> > > > a MDIO device wrapped in a struct phy_device. If this driver has a
> > > > .of_match_table member set, then this is wrong - the basic rule is
> > > > 
> > > > 	PHY drivers must never match using DT compatibles.
> > > > 
> > > > because this is exactly what will occur - it bypasses the check that
> > > > the mdio_device being matched is in fact wrapped by a struct phy_device,
> > > > and we will access members of the non-existent phy_device, including
> > > > the "uninitialised" mutex.
> > > > 
> > > > If the swconfig-based b53 driver does want to bind to a phy_device based
> > > > DT node, then it needs to match using either a custom .match_phy_device
> > > > method in the PHY driver, or it needs to match using the PHY IDs.
> > > 
> > > Sorry, I should have linked downstream b53_mdio.c . It's:
> > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c;h=98cdbffe73c7354f4401389dfcc96014bff62588;hb=HEAD
> > 
> > Yes, I just found a version of the driver
> > 
> > > You can see that is *uses* of_match_table.
> > > 
> > > What about refusing bugged drivers like above b53 with something like:
> > 
> > That will break all the MDIO based DSA and other non-PHY drivers,
> > sorry.
> > 
> > I suppose we could detect if the driver has the MDIO_DEVICE_IS_PHY flag
> > set, and reject any device that does not have MDIO_DEVICE_IS_PHY set:
> > 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 53f034fc2ef7..7bc06126ce10 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -939,6 +939,12 @@ EXPORT_SYMBOL_GPL(mdiobus_modify);
> >   static int mdio_bus_match(struct device *dev, struct device_driver *drv)
> >   {
> >   	struct mdio_device *mdio = to_mdio_device(dev);
> > +	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
> > +
> > +	/* Both the driver and device must type-match */
> > +	if (!(mdiodrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY) ==
> > +	    !(mdio->flags & MDIO_DEVICE_FLAG_PHY))
> > +		return 0;
> >   	if (of_driver_match_device(dev, drv))
> >   		return 1;
> > 
> 
> In OpenWrt & bugged b53 case we have:
> 1. Device without MDIO_DEVICE_FLAG_PHY
> 2. Driver with MDIO_DEVICE_IS_PHY
> 
> I think the logic should be to return 0 on mismatch (reverted).

I assume you mean the test should've been != not == - then yes, you
are absolutely correct. Sorry, I'm still not over a cold.

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