Message-ID: <aBnK4i52tEHs3jm_@shell.armlinux.org.uk>
Date: Tue, 6 May 2025 09:40:02 +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 Tue, May 06, 2025 at 09:36:57AM +0100, Russell King (Oracle) wrote:
> On Fri, May 02, 2025 at 06:54:49PM +0100, Russell King (Oracle) wrote:
> > 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.
> 
> Well, this series has been discarded from patchwork. Shrug. I won't be
> posting another version, stmmac can remain broken. I don't have a
> suggestion on better names for these functions.

... and in any case, Andrew's comment would be a *separate* change to
the subject of this series, so is not appropriate to be part of this
series.

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