From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 45C5648CD50; Thu, 8 Oct 2026 09:40:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791452418; cv=none; b=bnVKROrEJstVOp/q0h6/ihi7LmzHt6+gVRitLbbOrGq16UbI5OJ+auA6Nb8VVaaa6y/Aj5LeSYVywCrUHey4p9l8aQCzIy9bTZdkReI2Vfv3m8do2glq6UeTFi5JpsRS+d4yZ5L9a6dajlQfGBfcvCJ9ghSnIVjjmH88F4A0P/Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791452418; c=relaxed/simple; bh=ifHSObE8O9BYkkBoN4HrHeV1JF6y2jVafybfihz3ZJ4=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: Content-Type:MIME-Version; b=VHr7VyXA6E4LbLznpmZpl969JJ15nAEVwhmHfzlx3AUW6gmzxU9aNZs+YxmO/CznkLHG0sa1bftKKOT1M4ZT0HZX8cOXH80ASRnS9Iw8UT22eRPSVrU3OGciTNecNxKSSGGeFmA3tjUkmPrGaLL7kIOOI9Jl8Y1bI0egtDuGEps= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=FKSPB68z; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="FKSPB68z" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1A5341F000FF; Thu, 8 Oct 2026 09:40:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1791452416; bh=QECDrhyO2SRT7dWC4GJ9AXoE3hWRnyFkFCJDPpfrh+g=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=FKSPB68zclMN7RYAV+hPKr/bl9XazhnkHafJCjnGhVNcJma6Z/ZH934v217X3p1OB BDRmaR8S6HlbzsN6nevJpyrdTSjvrSoLz4TwJG7VrjVq83vC0EfyIc7G1OQxy6kYTK c5t9rk64lmwNfcTswBwxYyCT5opgE99uxLyVD0nDGmmFnjFzVGV5FmT+vyW10AaDsd o6EBe5obpvDT8zuR0yo/W582qyGTAUt13qi+lXmAXOATc6+bPLRVwg0JaSKzSaAeHk m2WoG5tayf9wqqgzURGo1UMxfk5LsU/aQhg2p2DeWvPVIK8VXW/J8q4szcqUT3T1Iv CwmGLhtWjuN6g== Subject: Re: [PATCH net-next v3 7/8] net: bcmgenet: allow the MTU to be changed From: netdev-bot+sashiko@kernel.org To: nb@tipi-net.de Cc: opendmb@gmail.com, florian.fainelli@broadcom.com, bcm-kernel-feedback-list@broadcom.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@kernel.org, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, dave.stevenson@raspberrypi.com, justin.chen@broadcom.com, pierremarinleclercq88@gmail.com, f.fainelli@gmail.com Date: Thu, 08 Oct 2026 09:40:15 +0000 Message-ID: <179145241567.434549.11505841771363424901@kernel.org> In-Reply-To: <20261007-nb-genet-mtu-nn-v2-v3-7-74a796c019ce@tipi-net.de> References: <20261007-nb-genet-mtu-nn-v2-v3-7-74a796c019ce@tipi-net.de> X-sashiko-severity: Medium Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider. Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 2 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] bcmgenet_change_mtu() leaves the PHY state machine running, but it needs the MAC disabled while it reprograms the MAC. - [Low] At this commit, ENET_MAX_MTU (bcmgenet.c:90) and bcmgenet_pkt_rdy_thld() (bcmgenet.c:2688) size one RX descriptor for 2… - [Low] bcmgenet_change_mtu() leaves the PHY state machine running, but bcmgenet_netif_stop() -> bcmgenet_intr_disable() (line 3596) writes… Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the new priv->datapath_up flag does not cover it. - [Medium] This problem wasn't introduced by this patch, but bcmgenet_netif_stop() ignores the -ETIMEDOUT result of… -- net: bcmgenet: allow the MTU to be changed On GENET v2 and later, this commit sets dev->max_mtu from the RX packet ready threshold. It also adds an ndo_change_mtu handler that tears down and rebuilds the DMA rings in place while the PHY keeps running. A new datapath_up flag lets bcmgenet_close() skip the teardown when a failed MTU change has already taken the datapath down. Link: https://github.com/raspberrypi/linux/issues/5561 > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index 17c8318c20388..6d21ac2d78d51 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -84,6 +84,12 @@ > ENET_THLD_MAX * ENET_THLD_UNIT, \ > ENET_THLD_PAGE_LEN) > > +/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN > + * interface can use the parent MTU. > + */ > +#define ENET_MAX_MTU (ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \ > + ETH_HLEN - VLAN_HLEN) [Severity: Low] At this commit, ENET_MAX_MTU and bcmgenet_pkt_rdy_thld() both size one RX descriptor as GENET_RBUF_ALIGN + mtu + ETH_HLEN + VLAN_HLEN. Meanwhile, bcmgenet_set_mtu_regs() programs the MAC limit with the larger ENET_FRAME_OVERHEAD: bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN); If the MAC accepts a frame that is bigger than one descriptor, for example QinQ at the parent MTU, that frame spans two descriptors. Would it then be dropped in bcmgenet_desc_rx()? if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) { netif_err(priv, rx_status, dev, "dropping fragmented packet!\n"); The forwarded FCS case doesn't look reachable here. reset_umac() in open clears CMD_CRC_FWD, and bcmgenet_set_features() then reads crc_fwd_en back as 0. So only frames longer than the MTU plus one VLAN tag seem to be affected. The later patch in this series, "net: bcmgenet: reassemble jumbo frames from status block fragments", removes ENET_MAX_MTU and replaces the drop with bcmgenet_add_frag() reassembly. That means this only applies to the intermediate state at this commit. > + > /* Tx/Rx DMA register offset, skip 256 descriptors */ > #define WORDS_PER_BD(p) (p->hw_params->words_per_bd) > #define DMA_DESC_SIZE (WORDS_PER_BD(priv) * sizeof(u32)) [ ... ] > @@ -3605,7 +3612,11 @@ static int bcmgenet_close(struct net_device *dev) > > netif_dbg(priv, ifdown, dev, "bcmgenet_close\n"); > > - bcmgenet_netif_stop(dev, false); > + /* A failed MTU change can have torn the datapath down already */ > + if (priv->datapath_up) { > + bcmgenet_netif_stop(dev, false); > + priv->datapath_up = false; > + } [Severity: High] This isn't a bug introduced by this patch, but does datapath_up cover the suspend/resume path? Only bcmgenet_open() and bcmgenet_change_mtu() update the flag. The non-WoL branch of bcmgenet_suspend() calls bcmgenet_netif_stop(dev, true). That disables the NAPIs and frees rx_cbs/tx_cbs in bcmgenet_fini_dma() without NULLing the pointers, and it leaves datapath_up set. If bcmgenet_resume() then fails in bcmgenet_init_dma(): ret = bcmgenet_init_dma(priv, false); if (ret) { netdev_err(dev, "failed to initialize DMA\n"); goto out_clk_disable; } then the interface is still running, but it has no rings and datapath_up is still 1. A later bcmgenet_close() from ifdown, unregister or shutdown would call bcmgenet_netif_stop() a second time. The NAPIs are already disabled at that point. Can napi_disable() in bcmgenet_disable_tx_napi() then spin forever with RTNL held? If it did return, bcmgenet_fini_dma() would kfree() rx_cbs and tx_cbs a second time. A failed bcmgenet_init_dma() on resume looks realistic. The earlier patch in this series, "net: bcmgenet: pad transmit frames out of the packet ready window", describes TX DMA refusing to halt so that every later bcmgenet_init_dma() fails. The RX buffers also come from page_pool_dev_alloc(), which doesn't sleep. bcmgenet_change_mtu() adds another way to hit this, because it calls bcmgenet_netif_stop() without checking datapath_up. Would clearing datapath_up in the suspend teardown, and setting it again only after a successful resume, close this gap? > > /* Really kill the PHY state machine and disconnect from it */ > phy_disconnect(dev->phydev); > @@ -3853,6 +3864,71 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier) > return 0; > } > > +static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu) > +{ [ ... ] > + /* The watchdog trips on an idle queue once the rings are gone */ > + netif_device_detach(dev); > + > + /* Only the buffers and the MTU registers change, leave the PHY up */ > + bcmgenet_netif_stop(dev, false); [Severity: Medium] This is a pre-existing issue, but bcmgenet_netif_stop() ignores the -ETIMEDOUT result of bcmgenet_dma_teardown(): bcmgenet_dma_teardown(priv); /* Disable MAC transmit. TX DMA disabled must be done before this */ umac_enable_set(priv, CMD_TX_EN, false); If RDMA or TDMA doesn't halt in time, bcmgenet_fini_dma() still returns the RX pages to the page pool and destroys the pools. Hardware descriptors may still point at those pages. Could the device then DMA into freed pages, or read freed skbs on TX? MAC RX is disabled before the teardown, so this needs a real halt timeout to trigger. This new caller lets an admin run the teardown repeatedly with ip link set mtu. > + priv->datapath_up = false; > + > + WRITE_ONCE(dev->mtu, new_mtu); > + priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu); > + bcmgenet_set_mtu_regs(priv, new_mtu); > + > + ret = bcmgenet_init_dma(priv, true); [Severity: Medium] Is the MAC actually disabled at this point if the interface was opened without a link? After an open with no link, reset_umac() leaves CMD_SW_RESET set. umac_enable_set() returns early while that bit is set: if (reg & CMD_SW_RESET) { spin_unlock_bh(&priv->reg_lock); return; } So the CMD_RX_EN/CMD_TX_EN disable in bcmgenet_netif_stop() does nothing. The PHY state machine keeps running and doesn't take RTNL. If the first link-up arrives during the teardown and rebuild, bcmgenet_mii_setup()->bcmgenet_mac_config() does: if (reg & CMD_SW_RESET) { reg &= ~CMD_SW_RESET; bcmgenet_umac_writel(priv, reg, UMAC_CMD); udelay(2); reg |= CMD_TX_EN | CMD_RX_EN; } bcmgenet_set_mtu_regs() is documented as "Call with the MAC disabled". Wouldn't it, along with the RBUF flush in bcmgenet_init_dma(), then run while the MAC is receiving? Nothing seems to serialize the adjust_link callback against this window. reg_lock only covers single read-modify-write sequences. > + if (ret) { [ ... ] > + bcmgenet_hfb_restore(priv); > + bcmgenet_netif_start(dev, false); > + > + /* bcmgenet_netif_start() only restores the link interrupt */ > + if (bcmgenet_has_mdio_intr(priv)) > + bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_MDIO_EVENT, > + INTRL2_CPU_MASK_CLEAR); [Severity: Low] The PHY keeps running during the MTU change, but bcmgenet_netif_stop()->bcmgenet_intr_disable() also masks UMAC_IRQ_MDIO_EVENT. It is only unmasked here, after the whole teardown and rebuild. bcmgenet_mii_wait() depends on that interrupt to wake priv->wq: wait_event_timeout(priv->wq, !(bcmgenet_umac_readl(priv, UMAC_MDIO_CMD) & MDIO_START_BUSY), HZ / 100); Would every phylib MDIO access in this window wait the full 10 ms timeout while holding phydev->lock? bcmgenet_mii_wait() always returns 0, so the effect looks like added latency rather than a failure. [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-nb-genet-mtu-nn-v2-v3-0-74a796c019ce%40tipi-net.de