Message-ID: <aBnKKafHHjkL5iP-@shell.armlinux.org.uk>
Date: Tue, 6 May 2025 09:36:57 +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 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.

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