Re: Race between "Generic PHY" and "bcm53xx" drivers after -EPROBE_DEFER

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Sep 20, 2021 at 11:10:39AM -0700, Florian Fainelli wrote:
> On 9/20/21 11:02 AM, Vladimir Oltean wrote:
> > On Mon, Sep 20, 2021 at 10:46:31AM -0700, Florian Fainelli wrote:
> >> On 9/20/21 10:40 AM, Vladimir Oltean wrote:
> >>> On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
> >>>> The SPROM is a piece of NVRAM that is intended to describe in a set of
> >>>> key/value pairs various platform configuration details. There can be up
> >>>> to 3 GMACs on the SoC which you can connect in a variety of ways towards
> >>>> internal/external PHYs or internal/external Ethernet switches. The SPROM
> >>>> is used to describe whether you connect to a regular PHY (not at PHY
> >>>> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
> >>>> switch pseudo-PHY via MDIO.
> >>>>
> >>>> What appears to be missing here is that we should not be executing this
> >>>> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
> >>>> PHY device proper to begin with and this collides with registering the
> >>>> b53_mdio driver.
> >>>
> >>> Who provisions the SPROM exactly? It still seems pretty broken to me
> >>> that one of the GMACs has a bgmac->phyaddr pointing to a switch.
> >>
> >> The OEMs are typically responsible for that. It is not "broken" per-se,
> >> and you will find additional key/value pairs that e.g.: describe the
> >> initial switch configuration something like:
> >>
> >> vlan0ports="0 1 2 3 5t"
> >> vlan1ports="4 5t"
> >>
> >> So this has been used as a dumping ground of "how I want the device to
> >> be configured eventually". 0x1e/30 is sort of "universally" within
> >> Broadcom's own universe that this designates an Ethernet switch
> >> pseudo-PHY MDIO bus address, and we all know that nobody in their right
> >> mind would design a Wi-Fi router with a discrete Ethernet switch that is
> >> not from Broadcom, right?
> >>
> > 
> > But even so, what's a "pseudo PHY" exactly? I think that's at the bottom
> > of this issue. In the Linux device model, a device has a single driver.
> > In this case, the same MDIO device either has a switch driver, if you
> > accept it's a switch, or a PHY driver, if you accept it's a PHY.
> > I said it's "broken" because the expectation seems to be that it's a switch,
> > but it looks like it's treated otherwise. Simply put, the same device
> > can't be both a switch and a PHY.
> 
> A pseudo-PHY is a device that can snoop and respond to MDIO bus
> requests. I understand it cannot be both, just explaining to you how the
> people at Broadcom have been seeing the world from their perspective.
> Anything that is found at MDIO address 0x1e/30 is considered a MDIO
> attached switch, that's all.
> 
> > 
> > The issue is really in bcma_phy_connect. That is what force-binds the
> > generic PHY driver. Since the bgmac-bcma driver does not support fixed
> > links, it tries to make do the way it can. This will not work with DSA.
> 
> Yes, I understand that.
> 
> > 
> >>> Special-casing the Broadcom switch seems not enough, the same thing
> >>> could happen with a Marvell switch or others. How about looking up the
> >>> device tree whether the bgmac->mii_bus' OF node has any child with a
> >>> "reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
> >>> actually returns true for it?
> >>
> >> We could do that, however I don't know whether this will break the
> >> arch/mips/bcm47xx devices which are still in active use by the OpenWrt
> >> community and for which there is no Device Tree (no technical
> >> limitation, just no motivation since devices are EOL'd), but maybe out
> >> of tree patches can be carried in the OpenWrt tree to revert anything
> >> that upstream came up with.
> > 
> > By OpenWRT do you mean swconfig or actual DSA?
> 
> Yes, swconfig in that case with the b53 swconfig driver trying to
> register as a PHY device.
> 
> > 
> > I think Rafal is using device tree, so the check can be conditionally
> > made based on the presence of an OF node corresponding to the MDIO bus.
> > That would still work, unless the OpenWRT people want to use DSA without
> > device tree too...
> > 
> 
> All I am saying is that there is not really any need to come up with a
> Device Tree-based solution since you can inspect the mdio_device and
> find out whether it is an Ethernet PHY or a MDIO device proper, and that
> ought to cover all cases that I can think of.

Okay, but where's the problem? I guess we're on the same page, and
you're saying that we should not be calling bcma_mdio_mii_register, and
assigning the result to bgmac->mii_bus, because that makes us call
bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?