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 9DDF9C98304 for ; Wed, 23 Sep 2026 19:39:45 +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-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LQz5Qs/TyBSC2iBixGCGQ7+e98TfYZR6lZcaYPuMI3s=; b=3IeN23BT75LZUNjP5E5WAgLXKE 3uUQquVQsbVTy6OO19Ry3q3Yz+OLHoPc+Atdz8hxdCSaxsW79Dvy6dz6j7iACvyPojOoDtqK06zng hNDS8HL6aipnO4bdWWfMrLRRjporZ2lB/c0+yvVGdXeUAu815rdtZm0Qe1sLxDRHWH/B8CMFYlSDg CynLtf/MfQGLcCTHlNJ9Eju8Utejeweo2wrs9lQfBY3Fd9l5iVnBXpKFZDiyRmWEFBwVXgyZA4I6j yWekn2TAbBP3OXXdI+Rqd/vwxSVZEzaa1Dzbmwu5jcM8Rt+LrxJLL36IB1Ul5HYmOp4AGPr/WCtFF I/oZC5TA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1x9Soc-00000009Ljy-2PYf; Wed, 23 Sep 2026 19:39:38 +0000 Received: from sea.source.kernel.org ([172.234.252.31]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1x9SoZ-00000009Lig-3dhN for linux-arm-kernel@lists.infradead.org; Wed, 23 Sep 2026 19:39:35 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 41D24429F8; Wed, 23 Sep 2026 19:39:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2FEA81F00893; Wed, 23 Sep 2026 19:39:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1790192375; bh=LQz5Qs/TyBSC2iBixGCGQ7+e98TfYZR6lZcaYPuMI3s=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=XKyj694ZA7KNrrTWaoenz7yQfsl2C6JxrsUwR1InKVJ6s8yjgW7j86gyVskHjdYJv h1kFfcnUtphet4keeq+mhjFvoFD8DmLD7SXpuWUjid9VeaJ4PYbGs9D7suPXOYKASg PrdvMRADbQ3TqMM6/TNf7JtkIebGA50UXlYp5dKfvSfO0mH2NUrG3ohxBvn1UuKsMg pSLQ/EYAMCQiId98RVUN2VXfWFebFuwWb30yfxKhmNFLzGl9/dOsNR6ArI09n4+kIy aGWHt48QYMVEAnQffQ2p4JBb+puisgBWB+yed+mEpvRkp++jJPwSfbzxNmJgKxlxbX RSMHZPRmZV4NQ== Date: Wed, 23 Sep 2026 20:39:29 +0100 From: Simon Horman To: Daniel Machon Cc: netdev-bot+sashiko@kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horatiu.vultur@microchip.com, steen.hegelund@microchip.com, UNGLinuxDriver@microchip.com, ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org, john.fastabend@gmail.com, sdf@fomichev.me, herve.codina@bootlin.com, arnd@arndb.de, gregkh@linuxfoundation.org, mohsin.bashr@gmail.com, richardcochran@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH net-next v7 11/14] net: lan966x: add PCIe FDMA MTU change support Message-ID: <20260923193929.GW13925@horms.kernel.org> References: <20260918-lan966x-pci-fdma-v7-11-0ecc179c8a2c@microchip.com> <179008205354.2160803.17637640817184992984@kernel.org> <20260923094622.gmxyw46ligz4rsyd@DEN-DL-M70577.microsemi.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260923094622.gmxyw46ligz4rsyd@DEN-DL-M70577.microsemi.net> 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 Wed, Sep 23, 2026 at 11:46:22AM +0200, Daniel Machon wrote: > > Thank you for your contribution! Sashiko AI review found 4 potential > > issue(s) to consider. > > > > Critical: 0 · High: 2 (2) · Medium: 1 (1) · Low: 1 > > > > 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). > > > > - [High] MTU change during or after PCIe FDMA teardown runs the full > > reload on freed/quiesced state: unbounded napi_disable() while holding… > > - [High] The new resize readiness check publishes lan966x->rx.lan966x > > before the PCI FDMA rings exist, so an MTU change can race with ring… > > - [Medium] The new PCIe resize path retires DMA rings and tears down > > their ATU outbound window without ever confirming the hardware is… > > - [Low] FDMA_PCI_DB_SIZE_MAX / FDMA_PCI_MAX_MTU perform unsigned > > arithmetic with no underflow guard and no BUILD_BUG_ON, even though… > > > > Pre-existing issues: > > - [High] The hazard the new PCIe reload path documents and avoids is > > still live in the sibling platform reload path, which this series… > > - [High] The two implementers of lan966x_fdma_ops::fdma_resize now > > disagree about the 'rings not built yet' precondition, and the… > > - [Medium] Window in which DEV_MAC_MAXLEN_CFG exceeds the DCB buffer > > size, combined with a missing SOF/EOF check in… > > > > -- > > > > net: lan966x: add PCIe FDMA MTU change support > > > > This adds MTU change support to the PCIe FDMA path by reallocating the > > contiguous ATU-mapped RX and TX rings at the new size and falling back to > > the old buffers on failure. It also caps the PCIe DCB ring at 256 entries > > and advertises a derived dev->max_mtu (FDMA_PCI_MAX_MTU) when the FDMA is > > in use. > > > > A few questions below about the new reload path and the new size macros. > > > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c > > > index 5d6902459f20..940425beec2f 100644 > > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c > > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c > > > @@ -358,7 +358,7 @@ static int lan966x_fdma_pci_init(struct lan966x *lan966x) > > > lan966x->rx.lan966x = lan966x; > > > lan966x->rx.max_mtu = lan966x_fdma_get_max_frame(lan966x); > > > rx_fdma->channel_id = FDMA_XTR_CHANNEL; > > > - rx_fdma->n_dcbs = FDMA_DCB_MAX; > > > + rx_fdma->n_dcbs = FDMA_PCI_DCB_MAX; > > > rx_fdma->n_dbs = FDMA_RX_DCB_MAX_DBS; > > > rx_fdma->priv = lan966x; > > > rx_fdma->db_size = FDMA_PCI_DB_SIZE(lan966x->rx.max_mtu); > > > > [Severity: High] > > Can an MTU change race with ring construction here? lan966x_fdma_pci_init() > > sets rx.lan966x before it fills in channel_id/n_dcbs/n_dbs/db_size/size and > > before the two sleeping allocations in lan966x_fdma_pci_rx_alloc() and > > lan966x_fdma_pci_tx_alloc(). > > > > The netdevs are already live at that point: lan966x_probe() runs the whole > > port loop, including lan966x_probe_port()->register_netdev() and > > lan966x_port_init()->lan966x_fdma_netdev_init()->netif_napi_add()+ > > napi_enable(), and only calls ops->fdma_init() afterwards, without holding > > RTNL. > > > > So an rtnl-held ndo_change_mtu in that window sees rx.lan966x != NULL, passes > > the new guard in lan966x_fdma_pci_resize(), finds a max_mtu that differs from > > the partially initialised rx.max_mtu, and enters __lan966x_fdma_pci_reload(). > > Both contexts then write the same lan966x->rx.fdma / lan966x->tx.fdma: > > > > lan966x_fdma_pci_reload() > > lan966x_fdma_rx_disable(&lan966x->rx) /* channel_id still 0 */ > > lan966x->rx.fdma.atu_region = NULL; /* under probe's feet */ > > lan966x_fdma_pci_rx_alloc(&lan966x->rx) /* second ring */ > > > > Does this leak probe's ring plus its ATU region (only > > FDMA_PCI_ATU_REGION_MAX == 6 exist), or trip WARN_ON(fdma->atu_region) / > > -ENOSPC in fdma_pci_atu_region_map()? And on the failure branch, > > lan966x_fdma_pci_reset_mem() dereferences tx->fdma.atu_region->base_addr, > > which is NULL in a snapshot taken before probe allocated it. > > Yes, same issue as adressed in #4. Theres a race because netdevs are registered > before initialization (in this case FDMA) has happened. The proper fix is a > reordering of the probe initialization. Hi Daniel, I'm wondering if you could expand on this a little. I'm unsure what #4 refers to. And I'm unclear if you are planing the reordering as part of this patchset. > > > > > [ ... ] > > > > > @@ -391,9 +391,156 @@ static int lan966x_fdma_pci_init(struct lan966x *lan966x) > > > return 0; > > > } > > > > > > +/* Reset existing rx and tx buffers. */ > > > +static void lan966x_fdma_pci_reset_mem(struct lan966x *lan966x) > > > +{ > > > + struct lan966x_rx *rx = &lan966x->rx; > > > + struct lan966x_tx *tx = &lan966x->tx; > > > + > > > + memset(rx->fdma.dcbs, 0, rx->fdma.size); > > > + memset(tx->fdma.dcbs, 0, tx->fdma.size); > > > + > > > + fdma_dcbs_init(&rx->fdma, > > > + FDMA_DCB_INFO_DATAL(rx->fdma.db_size - XDP_PACKET_HEADROOM), > > > + FDMA_DCB_STATUS_INTR); > > > + > > > + fdma_dcbs_init(&tx->fdma, > > > + FDMA_DCB_INFO_DATAL(tx->fdma.db_size), > > > + FDMA_DCB_STATUS_DONE); > > > + > > > + lan966x_fdma_llp_configure(lan966x, > > > + tx->fdma.atu_region->base_addr, > > > + tx->fdma.channel_id); > > > + lan966x_fdma_llp_configure(lan966x, > > > + rx->fdma.atu_region->base_addr, > > > + rx->fdma.channel_id); > > > +} > > > + > > > > [ ... ] > > > > > +static int lan966x_fdma_pci_reload(struct lan966x *lan966x, int new_mtu) > > > +{ > > > + struct fdma tx_fdma_old = lan966x->tx.fdma; > > > + struct fdma rx_fdma_old = lan966x->rx.fdma; > > > + u32 old_mtu = lan966x->rx.max_mtu; > > > + int err; > > > + > > > + napi_disable(&lan966x->napi); > > > + lan966x_fdma_tx_disable_netdev(lan966x); > > > + lan966x_fdma_rx_disable(&lan966x->rx); > > > + lan966x_fdma_tx_disable(&lan966x->tx); > > > > [Severity: High] > > What happens if an MTU change lands after the PCIe FDMA has already been torn > > down? lan966x_fdma_pci_deinit() disables NAPI and frees both coherent rings, > > but it leaves lan966x->rx.lan966x set, lan966x->fdma true and fdma->dcbs > > dangling. It also runs before the netdevs are unregistered and without RTNL: > > > > lan966x_remove() > > lan966x->ops->fdma_deinit(lan966x); > > lan966x_cleanup_ports(lan966x); /* unregister_netdev() here */ > > > > The new guard in lan966x_fdma_pci_resize() only rejects "rings not built > > yet", so an ndo_change_mtu in that window passes it and reaches this > > function. Three things then follow in order: > > > > napi_disable(&lan966x->napi) on an already-disabled NAPI. Does > > napi_disable_locked() spin forever in > > > > while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) > > usleep_range(20, 200); > > > > since nothing clears those bits until napi_enable()? That task holds RTNL, > > so unregister_netdev() in lan966x_remove() can never complete. > > > > If that is somehow passed, the success path below calls > > fdma_free_coherent_and_unmap() on the stale rx_fdma_old/tx_fdma_old > > snapshots, so dma_free_coherent(dev, fdma->size, fdma->dcbs, fdma->dma) runs > > a second time on the multi-MB blocks deinit already released. > > > > And the restore: path calls lan966x_fdma_pci_reset_mem(), which memsets up to > > rx->fdma.size bytes into freed DMA memory and then dereferences > > tx->fdma.atu_region->base_addr, which fdma_free_coherent_and_unmap() already > > set to NULL. > > > > Would a "rings are live" state check (or clearing rx.lan966x and dcbs in > > deinit, or unregistering the netdevs before fdma_deinit) close this? > > lan966x_remove() is not touched by this series (apart from calling > ops->fdma_deinit), so I'd say pre-existing. But I believe this issue will also > disappear, when we refactor probe init as described above. ...