netdev - Re: [PATCH net] net: dsa: mt7530: permit port 5 to work without port 6 on MT7621 SoC

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: <ZAh679+uLJN/J5x7@corigine.com>
Date:   Wed, 8 Mar 2023 13:09:19 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Russell King <linux@...linux.org.uk>,
        René van Dorst <opensource@...rst.com>,
        Alexander Couzens <lynxis@...0.eu>,
        Ilya Lipnitskiy <ilya.lipnitskiy@...il.com>,
        Richard van Schagen <richard@...terhints.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        erkin.bozoglu@...ont.com, Greg Ungerer <gerg@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, arinc9.unal@...il.com,
        Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net] net: dsa: mt7530: permit port 5 to work without port
 6 on MT7621 SoC

On Tue, Mar 07, 2023 at 05:54:11PM +0200, Vladimir Oltean wrote:
> The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
> internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
> receives corrupted frames, unless port 6 (attached to GMAC0) has been
> brought up by the driver. This is true regardless of whether port 5 is
> used as a user port or as a CPU port (carrying DSA tags).
> 
> Offline debugging (blind for me) which began in the linked thread showed
> experimentally that the configuration done by the driver for port 6
> contains a step which is needed by port 5 as well - the write to
> CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
> the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
> the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
> RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
> MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
> BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
> values of the register, while on the MT7621 SoC it cannot.
> 
> The call path that triggers the register write is:
> 
> mt753x_phylink_mac_config() for port 6
> -> mt753x_pad_setup()
>    -> mt7530_pad_clk_setup()
> 
> so this fully explains the behavior noticed by Arınç, that bringing port
> 6 up is necessary.
> 
> The simplest fix for the problem is to extract the register writes which
> are needed for both port 5 and 6 into a common mt7530_pll_setup()
> function, which is called at mt7530_setup() time, immediately after
> switch reset. We can argue that this mirrors the code layout introduced
> in mt7531_setup() by commit 42bc4fafe359 ("net: mt7531: only do PLL once
> after the reset"), in that the PLL setup has the exact same positioning,
> and further work to consolidate the separate setup() functions is not
> hindered.
> 
> Testing confirms that:
> 
> - the slight reordering of writes to MT7530_P6ECR and to
>   CORE_GSWPLL_GRP1 / CORE_GSWPLL_GRP2 introduced by this change does not
>   appear to cause problems for the operation of port 6 on MT7621 and on
>   MT7623 (where port 5 also always worked)
> 
> - packets sent through port 5 are not corrupted anymore, regardless of
>   whether port 6 is enabled by phylink or not (or even present in the
>   device tree)
> 
> My algorithm for determining the Fixes: tag is as follows. Testing shows
> that some logic from mt7530_pad_clk_setup() is needed even for port 5.
> Prior to commit ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK
> API"), a call did exist for all phy_is_pseudo_fixed_link() ports - so
> port 5 included. That commit replaced it with a temporary "Port 5 is not
> supported!" comment, and the following commit 38f790a80560 ("net: dsa:
> mt7530: Add support for port 5") replaced that comment with a
> configuration procedure in mt7530_setup_port5() which was insufficient
> for port 5 to work. I'm laying the blame on the patch that claimed
> support for port 5, although one would have also needed the change from
> commit c3b8e07909db ("net: dsa: mt7530: setup core clock even in TRGMII
> mode") for the write to be performed completely independently from port
> 6's configuration.
> 
> Thanks go to Arınç for describing the problem, for debugging and for
> testing.
> 
> Reported-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> Link: https://lore.kernel.org/netdev/f297c2c4-6e7c-57ac-2394-f6025d309b9d@arinc9.com/
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Simon Horman <simon.horman@...igine.com>