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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Sep 2021 11:10:39 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Rafał Miłecki <zajec5@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: Race between "Generic PHY" and "bcm53xx" drivers after
 -EPROBE_DEFER

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.
-- 
Florian