linux-kernel - Re: [PATCH net-next v2] net: stmmac: don't create a MDIO bus if unnecessary

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]

Message-ID: <nx2qggr3aget4t57qbosj6ya5ocq47t6w33ve5ycabs5mzvo7c@vctjvc5gip5d>
Date:   Thu, 7 Dec 2023 17:00:15 -0600
From:   Andrew Halaney <ahalaney@...hat.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        Serge Semin <fancer.lancer@...il.com>
Subject: Re: [PATCH net-next v2] net: stmmac: don't create a MDIO bus if
 unnecessary

On Thu, Dec 07, 2023 at 10:30:23PM +0100, Andrew Lunn wrote:
> On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote:
> > The stmmac_dt_phy() function, which parses the devicetree node of the
> > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > fixed-link means in relation to the MAC's MDIO bus. This results in
> > a MDIO bus being created in situations it need not be.
>
> Please extend that with something like....
>
> This is bad, because ....
>
> Most 'clean' driver unconditionally create the MDIO bus. But stmmac is
> not that clean, and has to keep backwards compatibility to some old
> usage. I'm just wondering what this patch actually brings us, and is
> it worth it. Is it fixing a real bug, or just an optimisation?
>
>    Andrew
>

It is an optimization for speeding up time to link up. I already sent
out v3 moments before this arrived, I'll be sure to capture that more
clearly in v4 (and wait a little longer before respinning it).

I'm trying to optimize this device configuration (as shown in the commit):
"""
    Here's[1] an example where there is no MDIO bus or fixed-link for
    the ethernet1 MAC, so no MDIO bus should be created since ethernet0
    is the MDIO master for ethernet1's phy:

        &ethernet0 {
                phy-mode = "sgmii";
                phy-handle = <&sgmii_phy0>;

                mdio {
                        compatible = "snps,dwmac-mdio";
                        sgmii_phy0: phy@8 {
                                compatible = "ethernet-phy-id0141.0dd4";
                                reg = <0x8>;
                                device_type = "ethernet-phy";
                        };

                        sgmii_phy1: phy@a {
                                compatible = "ethernet-phy-id0141.0dd4";
                                reg = <0xa>;
                                device_type = "ethernet-phy";
                        };
                };
        };

        &ethernet1 {
                phy-mode = "sgmii";
                phy-handle = <&sgmii_phy1>;
        };
"""

I'm seeing that ethernet1 scans the whole MDIO bus created for it, and
finds nothing, wasting time in the process. Since there's no mdio bus
described (since it's vacant) you get something like this call order:

    stmmac_mdio_register()
    of_mdiobus_register(new_bus, mdio_node) // mdio_node is NULL in this case
    __of_mdiobus_register(mdio, np, owner)  // this doesn't set phy_mask since np == NULL
    __mdiobus_register(mdio, owner)
    mdiobus_scan_bus_c22(bus)
    mdiobus_scan_c22()                      // Called PHY_MAX_ADDR times, probing an empty bus

Which is causing a good bit of delay in the time to link up, each
attempted probe is taking about 5 ms and that's just benchmarking the
get_phy_c22_id() calls, if you look at the whole loop it's greater (but I
am unsure if that's just scheduling contention or something else going
on, once I realized we were doing this work unnecessarily I decided to
try and remove it)

I know you said the standard is to make the MDIO bus unconditionally, but
to me that feels like a waste, and describing say an empty MDIO bus
(which would set the phy_mask for us eventually and not scan a
bunch of phys, untested but I think that would work) seems like a bad
description in the devicetree (I could definitely see describing an
empty MDIO bus getting NACKed, but that's a guess).

Please let me know if you disagree with that logic and have some
alternative solutions for optimizing. In either case I think this code
needs some cleaning so I'll carry this through. It also seems that
unconditional creation of the MDIO bus is something that's biting some
stmmac variants that lack an MDIO controller based on Serge's latest
comments on v3:

    https://lore.kernel.org/netdev/jz6ot44fjkbmwcezi3fkgqd54nurglblbemrchfgxgq6udlhqz@ntepnnzzelta/

Thanks,
Andrew