netdev - Re: [PATCH net] net: dsa: mt7530: fix 10M/100M speed on MT7988 switch

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: <ZbL1VEcH3RgHZKsq@makrotopia.org>
Date: Thu, 25 Jan 2024 23:57:08 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: DENG Qingfang <dqfext@...il.com>, Sean Wang <sean.wang@...iatek.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, John Crispin <john@...ozen.org>
Subject: Re: [PATCH net] net: dsa: mt7530: fix 10M/100M speed on MT7988 switch

On Fri, Jan 26, 2024 at 01:44:57AM +0300, Arınç ÜNAL wrote:
> On 25.01.2024 19:18, Daniel Golle wrote:
> > On Thu, Jan 25, 2024 at 12:49:19PM +0300, Arınç ÜNAL wrote:
> > > On 24/01/2024 08:17, Daniel Golle wrote:
> > > > Setup PMCR port register for actual speed and duplex on internally
> > > > connected PHYs of the MT7988 built-in switch. This fixes links with
> > > > speeds other than 1000M.
> > > > 
> > > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch")
> > > > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> > > 
> > > Acked-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> > > 
> > > I'm wondering why we manually set speed and duplex for these interface
> > > modes in the first place. I don't how it works for
> > > PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and
> > > 802.3z interfaces, phylink should already supply proper speed and duplex.
> > 
> > It's true that duplex should always be set to full-duplex already by
> > phylink. However, speed could be 2500MBit/s (2500Base-X) or 2000MBit/s
> > (?, TRGMII) and we yet need to program the PCR like if it was
> > 1000MBit/s.
> > 
> > Regarding the INTERNAL case: it was added by mistake. In case of
> > MT7988, all ports of the switch are connected via INTERNAL links,
> > however, the PHYs still need adjustment of the PCR register just like
> > on all other MT753x switches and the CPU port is setup elsewhere
> > anyway.
> 
> It's not necessarily PHYs needing adjustment of the port MAC control
> register.

It's not the PHYs which need adjustment but the MAC PMCR register which
does.

> After reset, speed, duplex mode, etc. will be determined by polling
> the PHY connected to the switch MAC.

Yes, but as it is a DSA driver we don't use **hardware** PHY polling
and keep that off. Instead, PHY interrupts or software PHY polling is
used to have Linux determine the link properties.
We're then forcing these properties on the MAC port of the switch.

> on the PMCR because we're also configuring switch MACs that are not
> connected to PHYs, meaning the switch cannot determine these properties by
> polling a PHY.

The switch **never** determines these properties itself when using the
DSA driver. It has a facility to do so, and yes, when accessing
Ethernet in U-Boot or when using the 'swconfig'-based driver then this
facility is used. But, I repeat, when using DSA we do not use hardware
PHY polling. We poll (or rather: react to interrupts) in software
instead.

> 
> From what I understand, this code block is for overriding the speed and
> duplex variables to make the operations on the PMCR below work. It seems
> that this is actually only useful for PHY_INTERFACE_MODE_2500BASEX.
> PHY_INTERFACE_MODE_TRGMII is given SPEED_1000 by
> drivers/net/phy/phylink.c:phylink_interface_max_speed().
> PHY_INTERFACE_MODE_2500BASEX is given SPEED_2500. Overriding the duplex
> variable looks unnecessary.
> 
> Your patch here doesn't affect CPU ports because MT7531 and MT7988 PMCRs

This patch does not intend to affect the CPU port. As I've already
said in my previous reply "[...] the CPU port is setup elsewhere
anyway."

Maybe it wasn't clear, but I meant that the CPU port settings are
intentionally unaffected by this patch.

It is intended to affect user ports which with phy-mode = "internal";
set in DTS -- here we **do** need to set PMCR according the external
link speed and duplex.


> are configured with cpu_port_config before mt753x_phylink_mac_link_up(),
> and PHY_INTERFACE_MODE_INTERNAL is not used for MT7530 which, for MT7530,
> PMCRs will be set only on mt753x_phylink_mac_link_up().
> 
> PMCR_FORCE_SPEED_1000 is set on cpu_port_config. If someone were to get rid
> of cpu_port_config because of its utter uselessness, PMCR_FORCE_SPEED_1000
> would not be set, causing the link between port 6 MAC and SoC MAC to break.
> 
> In conclusion, I will add "case SPEED_10000:" to the operations where the
> speed and EEE bits are set on my patch for getting rid of cpu_port_config.

PMCR needs to be set according to actual link speed for built-in TP
PHYs. This is true for all mt7530 variants including MT7988.

Maybe the confusion here is that on MT7988 we use 'internal' phy-mode
for both, the switch CPU port's link to mtk_eth_soc gmac0 as well as
the links to the 4 built-in 1GE switch PHYs.

The latter were affected by wrongly overriding speed and duplex in
case phy-mode is set to "internal", which should not have been put
there (by me) in first place.

Let's just remove it, ok?