Message-ID: <aBUG6Z_Crs31W45x@shell.armlinux.org.uk>
Date: Fri, 2 May 2025 18:54:49 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>, bpf@...r.kernel.org,
	Daniel Borkmann <daniel@...earbox.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Jon Hunter <jonathanh@...dia.com>,
	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>,
	Thierry Reding <treding@...dia.com>
Subject: Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in
 XDP functions

On Fri, May 02, 2025 at 05:29:21PM +0200, Andrew Lunn wrote:
> On Fri, May 02, 2025 at 02:35:36PM +0100, Russell King (Oracle) wrote:
> > Phylink does not permit drivers to mess with the netif carrier, as
> > this will de-synchronise phylink with the MAC driver. Moreover,
> > setting and clearing the TE and RE bits via stmmac_mac_set() in this
> > path is also wrong as the link may not be up.
> > 
> > Replace the netif_carrier_on(), netif_carrier_off() and
> > stmmac_mac_set() calls with the appropriate phylink_carrier_block() and
> > phylink_carrier_unblock() calls, thereby allowing phylink to manage the
> > netif carrier and TE/RE bits through the .mac_link_up() and
> > .mac_link_down() methods.
> > 
> > This change will have the side effect of printing link messages to
> > the kernel log, even though the physical link hasn't changed state.
> > This matches the carrier state that userspace sees, which has always
> > "bounced".
> > 
> > Note that RE should only be set after the DMA is ready to avoid the
> > receive FIFO between the MAC and DMA blocks overflowing, so
> > phylink_start() needs to be placed after DMA has been started.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f59a2363f150..ac27ea679b23 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -6922,6 +6922,11 @@ void stmmac_xdp_release(struct net_device *dev)
> >  	/* Ensure tx function is not running */
> >  	netif_tx_disable(dev);
> >  
> > +	/* Take down the software link. stmmac_xdp_open() must be called after
> > +	 * this function to release this block.
> > +	 */
> > +	phylink_carrier_block(priv->phylink);
> > +
> >  	/* Disable NAPI process */
> >  	stmmac_disable_all_queues(priv);
> >  
> > @@ -6937,14 +6942,10 @@ void stmmac_xdp_release(struct net_device *dev)
> >  	/* Release and free the Rx/Tx resources */
> >  	free_dma_desc_resources(priv, &priv->dma_conf);
> >  
> > -	/* Disable the MAC Rx/Tx */
> > -	stmmac_mac_set(priv, priv->ioaddr, false);
> > -
> >  	/* set trans_start so we don't get spurious
> >  	 * watchdogs during reset
> >  	 */
> >  	netif_trans_update(dev);
> > -	netif_carrier_off(dev);
> >  }
> >  
> 
> >  int stmmac_xdp_open(struct net_device *dev)
> > @@ -7026,25 +7027,28 @@ int stmmac_xdp_open(struct net_device *dev)
> >  		hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >  	}
> >  
> > -	/* Enable the MAC Rx/Tx */
> > -	stmmac_mac_set(priv, priv->ioaddr, true);
> > -
> >  	/* Start Rx & Tx DMA Channels */
> >  	stmmac_start_all_dma(priv);
> >  
> > +	/* Allow phylink to bring the software link back up.
> > +	 * stmmac_xdp_release() must have been called prior to this.
> > +	 */
> 
> This is counter intuitive. Why is release called before open?

Indeed - and that should've been caught in the review where XDP was
being added.

> Looking into stmmac_xdp_set_prog() i think i get it. Even if there is
> not a running XDP prog, stmmac_xdp_release() is called, and then
> stmmac_xdp_open().

If there is a change of "do we have an XDP prog" state, then
stmmac_xdp_release() is called to free all the current contexts to
do with queue/descriptor management, and then stmmac_xdp_open() is
called thereafter. These are doing a subset of .ndo_open/.ndo_release
and I think that's where they're getting their naming from.

The only possible sequence is:

	stmmac_open()
then, on each XDP prog addition or removal, but not replacement:
		stmmac_xdp_release()
		stmmac_xdp_open()
finally,
	stmmac_release()

> Maybe these two functions need better names? prepare and commit?

Yes, it's all counter intuitive, and there are various things about the
XDP code that make it hard to follow.

For example, stmmac_xdp_set_prog() leads you to think, because of the
way the need_update variable is set, that looking for references to
xdp_prog would show one where all the dependents are, but no, there's
stmmac_xdp_is_enabled(), which is nice and readable, but could've
been used in stmmac_xdp_set_prog() to make it more obvious what to
grep for.

Incidentally, if stmmac_xdp_open() fails to re-grab the interrupts,
then it calls phylink_stop(), stmmac_hw_teardown(), and
free_dma_desc_resources().

If one then set the interface administratively down, stmmac_release()
gets called, which again calls phylink_stop(), free_dma_desc_resources()
and stmmac_release_ptp().

stmmac_release_ptp() disables/unprepares clk_ptp_ref, and unregisters
the PTP stuff. stmmac_hw_teardown() also disables/unprepares
clk_ptp_ref, so we probably unbalance the clk API in this case...
and probably much other stuff.

Calling free_dma_desc_resources() twice calls functios such as 
free_dma_tx_desc_resources() twice, and it looks like that's not going
to be healthy, calling dma_free_coherent() with the same arguments,
double-releasing memory. Same for kfree(). Probably same for the RX
stuff.

Basically, if one messes with XDP in this driver, expect things to go
bang and kill the kernel if something goes wrong with the whole
xdp_release+xdp_open dance.

Honestly, this needs a rewrite, but I currently know nowt about XDP.

So, I'd suggest that the names of these functions is the least of the
problems here.

-- 
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