netdev - Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

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: <ZOoRiVZiAyf7pArp@shell.armlinux.org.uk>
Date: Sat, 26 Aug 2023 15:51:53 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Feiyang Chen <chenfeiyang@...ngson.cn>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink
 caps to dwxgmac2 core

On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote:
> On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> > Move the xgmac specific phylink capabilities to the dwxgmac2 support
> > core.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 34e1b0c3f346..f352be269deb 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> >  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> >  }
> >  
> > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> > +{
> > +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> > +						 MAC_10000FD | MAC_25000FD |
> > +						 MAC_40000FD | MAC_50000FD |
> > +						 MAC_100000FD;
> > +}
> > +
> >  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
> >  {
> >  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
> >  
> >  const struct stmmac_ops dwxgmac210_ops = {
> >  	.core_init = dwxgmac2_core_init,
> 
> > +	.phylink_get_caps = xgmac_phylink_get_caps,
> 
> This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps
> speeds.

So the reason this got added is to keep the code compatible with how
things work today.

When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup()
would enable speeds from 2.5G up to 100G, limiting them if
priv->plat->max_speed is set non-zero.

The table in hwif.c matches when:
	entry->gmac == priv->plat->has_gmac,
	entry->gmac4 == priv->plat->has_gmac4 and
	entry->xgmac == priv->plat->has_xgmac

The entries in the table which patch on has_xgmac = true contain the
following:

                .mac = &dwxgmac210_ops,
                .mac = &dwxlgmac2_ops,

Therefore, to keep things compatible, I've effectively moved this
initialisation into the new .phylink_get_caps method that is part of
those two ops, and since they have has_xgmac true, this means that
all these speeds need to be set.

We do this without regard to max_speed, which we apply separately,
after the .phylink_get_caps method has returned.

So, the code is functionally identical to what happens in the driver,
even if it is the case that xgmac210 doesn't actually support the
speeds. If those extra speeds that the hardware doesn't support were
present before, they're present after. If those extra speeds are
limited by the max_speed, then they will be similarly limited.

While it may look odd, since the specifications for Synopsys are all
behind closed doors, all I can do is transform the code - I can't
know that such-and-such a core doesn't actually support stuff. So
my only option is to keep the code bug-compatible.

I think all I've done here is make it glaringly obvious what the old
code is doing and you've spotted "but that isn't right!" - which is
actually a good thing!

Feel free to submit patches to correct the functionality as bugs in
the driver become more obvious!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!