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 10:46:31 -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 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?

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