Message-ID: <Z77hVrC5Lcbxrlx8@shell.armlinux.org.uk>
Date: Wed, 26 Feb 2025 09:39:34 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Furong Xu <0x1207@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Emil Renner Berthing <kernel@...il.dk>,
	Eric Dumazet <edumazet@...gle.com>,
	Fabio Estevam <festevam@...il.com>, imx@...ts.linux.dev,
	Inochi Amaoto <inochiama@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jan Petrous <jan.petrous@....nxp.com>,
	Jon Hunter <jonathanh@...dia.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Minda Chen <minda.chen@...rfivetech.com>, netdev@...r.kernel.org,
	NXP S32 Linux Team <s32@....com>, Paolo Abeni <pabeni@...hat.com>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Thierry Reding <treding@...dia.com>, hailong.fan@...ngine.com,
	2694439648@...com
Subject: Re: net: stmmac: weirdness in stmmac_hw_setup()

On Wed, Feb 26, 2025 at 10:43:26AM +0800, Furong Xu wrote:
> On Thu, 20 Feb 2025 16:17:43 +0000, "Russell King (Oracle)" wrote:
> 
> > While looking through the stmmac driver, I've come across some
> > weirdness in stmmac_hw_setup() which looks completely barmy to me.
> > 
> > It seems that it follows the initialisation suggested by Synopsys
> > (as best I can determine from the iMX8MP documentation), even going
> > as far as to *enable* transmit and receive *before* the network
> > device has been administratively brought up. stmmac_hw_setup() does
> > this:
> > 
> >         /* Enable the MAC Rx/Tx */
> >         stmmac_mac_set(priv, priv->ioaddr, true);
> > 
> > which sets the TE and RE bits in the MAC configuration register.
> > 
> > This means that if the network link is active, packets will start
> > to be received and will be placed into the receive descriptors.
> > 
> > We won't transmit anything because we won't be placing packets in
> > the transmit descriptors to be transmitted.
> > 
> > However, this in stmmac_hw_setup() is just wrong. Can it be deleted
> > as per the below?
> > Tested-by: Thierry Reding <treding@...dia.com>
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index c2913f003fe6..d6e492f523f5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -3493,9 +3493,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
> >  		priv->hw->rx_csum = 0;
> >  	}
> >  
> > -	/* Enable the MAC Rx/Tx */
> > -	stmmac_mac_set(priv, priv->ioaddr, true);
> > -
> >  	/* Set the HW DMA mode and the COE */
> >  	stmmac_dma_operation_mode(priv);
> >  
> 
> A better fix here:
> https://lore.kernel.org/all/tencent_CCC29C4F562F2DEFE48289DB52F4D91BDE05@qq.com/

I don't think that addresses the issue I highlighted above, since it's
still calling stmmac_mac_set(, true) in stmmac_hw_setup(), which I
believe to be wrong (as per my explanation above.)

However, the removal of setting GMAC_CONFIG_TE in the start_tx method
looks correct to me, because:

- the start_tx method is called via stmmac_start_tx(), which is only
  called from stmmac_start_tx_dma().

- stmmac_start_tx_dma() is called from:
  * stmmac_start_all_dma()
  * stmmac_tx_err()
  * stmmac_enable_tx_queue()

* stmmac_start_all_dma() is called from the end of stmmac_hw_setup(),
  but we've already called stmmac_mac_set(, true) both before and
  after the patch in the above URL, so is redundant.

  Incidentally, this brings the same set of questions I've stated in
  my initial email, and to me this is wrong.

* stmmac_tx_err() can only happen when we are already active (so
  transmission was already enabled).

* stmmac_enable_tx_queue() is called from stmmac_xdp_enable_pool(),
  which will only call this if netif_running() returns true,
  implying that the netdev is already adminstratively brought up
  and thus stmmac_hw_setup() will have been called.

  Again, this brings the same set of questions I've stated in my
  initial email, and to me this is wrong.


While looking at Simon's comment, he talks about stmmac_xdp_open(), so
I just looked at that, and found:

        netif_carrier_on(dev);

Then there's:

        netif_carrier_off(dev);

in stmmac_xdp_release().

These were introduced in commit ac746c8520d9 ("net: stmmac: enhance XDP
ZC driver level switching performance"), well after stmmac had been
converted to phylink. Phylink documents this:

16. Verify that the driver does not call::

        netif_carrier_on()
        netif_carrier_off()

    as these will interfere with phylink's tracking of the link state,
    and cause phylink to omit calls via the :c:func:`mac_link_up` and
    :c:func:`mac_link_down` methods.

So, the presence of these calls will mess up phylink, resulting in
mac_link_up() and/or mac_link_down() *NOT* being called at appropriate
times.

However, stmmac_xdp_(open|release)() doesn't seem to do anything to
bring the PHY or PCS up/down. This makes me wonder whether XDP support
in the stmmac driver is basically broken, or maybe the netdev needs to
already be administratively up (ip li set dev ... up). I don't know
XDP as I've never used it. If that is the case, then those
netif_carrier_*() calls need removing. Or - I say again - stmmac needs
to *stop* using phylink. It's one or the other. A network driver
either needs to use phylink properly or not use phylink *at* *all*.

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

Powered by blists - more mailing lists