From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0BA5AC3ABAA for ; Fri, 2 May 2025 17:58:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gXNNDPdZb2Mvvv9l4U0qnaw+JsmJmG680iJnfO61RQ0=; b=fkYL7Xv1MIhiK9QIJQhv31ZI0h AxH/YqoZIeMtcwymutjB68SWL5w4i8G04hVU7iVtRWurVhn7/7NiXvh2zFlPC028fCvy0vOnnSZz4 IOH9lzDBSf1kFYHBZ3io4vUJ70DGMSHatesIow6801DuuiaNIYm1CfyYdqEdxGhV02DRXre8H6lNR tRgd81uw4H182ArXLxOIOaYqm1kMGIslRQ2MhSux2goA7kPaQfntck9+jf2VGawlXjMT83DZIj5tJ hNTB+E6DBFoBITVUrkW2My6yEQAtqH1ztH5t+UE9ivuZIyTqttSrEnysnU+4Kv02hUU0sblHxEry5 kJ2wp8Gg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uAuej-00000002j8l-0123; Fri, 02 May 2025 17:58:37 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uAubO-00000002ijJ-3Q81 for linux-arm-kernel@lists.infradead.org; Fri, 02 May 2025 17:55:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=gXNNDPdZb2Mvvv9l4U0qnaw+JsmJmG680iJnfO61RQ0=; b=drWIecNg8gz9iVU7R4SbsnNC4G OT+gaz2JAollchQiDnxPvMENCLtRfCnY/KiSO97PdVcgLiMk7OCUoMLZbwQairLtNUWRytnnYcFrd 9/v7gYhCHD/48M8bSMa275bj70kY+oJe5NLjmfS1yrf9YAroP0dm7wuZa6zJLyJX2bX5TJFiYjYxQ rm95uOLHeXAc7+LDiUYhyo8m1BWM53IZ5flM/iAtkFVBo6zEpNcvn5C/08Q+cMfcuvVVSFfTly0AZ HvmokMsKGp0KPgpwV6MoB6FY+sewh4jj8ga1Y4dn8f+jDdulRkjDj1b7R0ZIpOinKe5dHIn/tWTGD qOdbDESA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:43400) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1uAub8-0001h2-1h; Fri, 02 May 2025 18:54:54 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.96) (envelope-from ) id 1uAub4-0000s1-0M; Fri, 02 May 2025 18:54:50 +0100 Date: Fri, 2 May 2025 18:54:49 +0100 From: "Russell King (Oracle)" To: Andrew Lunn Cc: Heiner Kallweit , Alexandre Torgue , Alexei Starovoitov , Andrew Lunn , bpf@vger.kernel.org, Daniel Borkmann , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Jesper Dangaard Brouer , John Fastabend , Jon Hunter , linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Maxime Coquelin , netdev@vger.kernel.org, Paolo Abeni , Thierry Reding Subject: Re: [PATCH net-next v2 2/4] net: stmmac: call phylink_carrier_*() in XDP functions Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250502_105510_996523_8F6147A0 X-CRM114-Status: GOOD ( 36.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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) > > --- > > .../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!