From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.153.233]) (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 B361043DEDD; Fri, 2 Oct 2026 09:02:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=68.232.153.233 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790931777; cv=none; b=HZJOLTSIK7RAMtEiWDbdj0uLCoesJlXF0owqXH2UBOliawHp5hV+l7Qkr5hyNBi43xwqMDbNo1cY54FNGZsmUSnG+gtqTSXNuiPWicUq3HL48ZLqcrzUhl3imQSfpVLAuvEX2mdu4sQ4oAnv9N9gVXsAIFnzPMsgeTLEwB1/nmU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790931777; c=relaxed/simple; bh=NYec4roSXhQYkNBbZqppMpBIcQDhsY53JIrT130NstA=; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=n/5UxR9ViGE1nU5vU+LKXa6Ro0q3s9X1f9exyhcvDeefuwPjOo9PoYhr7qAZYgrwkEAKjz6VAfyNcjiONEojXmrGR9PWswXEaWLjBHb8OTfHDdwSxNKcm+VKKQfP6Z4Nbb9VjS4KazmYhoCCVbCP/r/5tjLroBYd7EZfPqwf2B4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=microchip.com; spf=pass smtp.mailfrom=microchip.com; dkim=pass (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b=e30ZpOVR; arc=none smtp.client-ip=68.232.153.233 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=microchip.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=microchip.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="e30ZpOVR" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1790931775; x=1822467775; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=NYec4roSXhQYkNBbZqppMpBIcQDhsY53JIrT130NstA=; b=e30ZpOVRCJBHWv1Rc7aLQyR1DEDIWzzKh332rqcXGdIyXqkC8wZNRjDp IWtIrMipCzWmr9R49PK9VHIHsE6ku7rE6y4xxnvpwLyU0n8+4KHzS2BvM ZFk81eAzAnjtfkWOMwTsGoeqxGYwhEYPU/G9eFluhjV840jTpf+TfUCeB irnom6mfRk6WLTEaW7v4jCV/zyZsKRq7ICF9Z2qI5DTn2sszi9qD5zYwG Iuzi+H00VYrcJb3dZ1Pl6mPRMxwxtfVSYnOwUJG8oiG//VjM8wjCAaiKu /xbRnoun9hl7AtNHIvpBfp5gzd9TFwNLFptsctOZqpYvQv89qmT0EGXCK g==; X-CSE-ConnectionGUID: 6wAdR5GBRXCFzXHrrka5bQ== X-CSE-MsgGUID: /nJjZBYTT+2pox3PyVJfbA== X-IronPort-AV: E=Sophos;i="6.27,136,1787036400"; d="scan'208";a="74723732" X-Amp-Result: SKIPPED(no attachment in message) Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa1.microchip.iphmx.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Oct 2026 02:02:54 -0700 Received: from chn-vm-ex04.mchp-main.com (10.10.87.151) by chn-vm-ex4.mchp-main.com (10.10.87.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.2.2562.49; Fri, 2 Oct 2026 02:02:53 -0700 Received: from DEN-DL-M70577 (10.10.85.11) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server id 15.1.2507.58 via Frontend Transport; Fri, 2 Oct 2026 02:02:49 -0700 Date: Fri, 2 Oct 2026 11:02:48 +0200 From: Daniel Machon To: CC: , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH net-next v9 11/15] net: lan966x: add PCIe FDMA support Message-ID: <20261002090248.xwlcm3zclt6imaou@DEN-DL-M70577> References: <20260928-lan966x-pci-fdma-v9-11-a17caf60f89a@microchip.com> <179092641772.434549.10428923177677726938@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <179092641772.434549.10428923177677726938@kernel.org> > Thank you for your contribution! Sashiko AI review found 8 potential > issue(s) to consider. > > Critical: 0 · High: 0 (1) · Medium: 3 (1) · Low: 5 (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). > > - [Medium] Double fetch of the RX BLOCKL. > - [Medium] lan966x_fdma_pci_napi_poll() limits RX consumption only by > `counter < weight` and tracks refill progress only through the wrapped… > - [Medium] When lan966x_fdma_pci_get_next_dcb() finds no free TX DCB, > lan966x_fdma_pci_xmit() calls netif_stop_queue(dev) and returns… > - [Low] At this commit the in-tree PCIe overlay > (drivers/misc/lan966x_pci.dtso) has no "fdma" interrupt. > - [Low] The iomap entry { TARGET_PCIE_DBI, 0x400000, 0 } derives > regs[TARGET_PCIE_DBI] 4 MB into IO range 0. > - [Low] lan966x_fdma_pci_resize() always returns -EOPNOTSUPP, so once > PCIe FDMA is active every ndo_change_mtu call is rolled back. > - [Low] In lan966x_fdma_pci_napi_poll(), the FDMA_ERROR branch says "No > rx_dropped increment here because src_port is invalid". > - [Low] With lan966x->ptp left at 0 on PCIe, lan966x_ptp_init() never > initialises port->tx_skbs or the PTP locks, while the "ptp" IRQ… > > Pre-existing issues: > - [High] This problem wasn't introduced by this patch, but the new PCIe > backend inherits it. > - [Medium] __fdma_dcb_add() writes fdma->last_dcb->nextptr (publishing > the new DCB) before it writes the new DCB's own nextptr =… > - [Low] lan966x_fdma_pci_hw_offload() reads ip_hdr(skb)->protocol and > ipv6_hdr(skb)->daddr without checking that skb->len covers an IP… > > -- > > net: lan966x: add PCIe FDMA support > > This commit adds a PCIe FDMA backend for lan966x. It uses contiguous > coherent DMA buffers mapped through the endpoint's ATU and copies frames > with memcpy. The new ops are selected when lan966x_is_pci() is true, and > XDP and PTP are disabled on the PCIe path. > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c > > new file mode 100644 > > index 000000000000..f511e7061314 > > --- /dev/null > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c > > [ ... ] > > > +static bool lan966x_fdma_pci_hw_offload(struct lan966x *lan966x, u32 port, > > + struct sk_buff **pskb) > > +{ > > [ ... ] > > > + if (skb->protocol == htons(ETH_P_IP) && > > + ip_hdr(skb)->protocol == IPPROTO_IGMP) > > + return false; > > + > > + if (IS_ENABLED(CONFIG_IPV6) && > > + skb->protocol == htons(ETH_P_IPV6) && > > + ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && > > + !ipv6_mc_check_mld(skb)) > > + return false; > > [Severity: Low] > This isn't a bug introduced by this patch, because the same code already > exists in lan966x_hw_offload() in lan966x_main.c. Still, > ip_hdr(skb)->protocol and ipv6_hdr(skb)->daddr are read here without > checking that skb->len covers an IP header. > > On a runt frame, or on a frame shrunk by an XDP program once XDP support > lands later in the series, could this read uninitialized skb tailroom? > > The reads stay inside the skb head allocation, and any minimum-size > Ethernet frame covers both fields. So only the offload_fwd_mark decision > for an already malformed packet is affected. > > > +static struct sk_buff *lan966x_fdma_pci_rx_get_frame(struct lan966x_rx *rx, > > + u64 src_port) > > +{ > > [ ... ] > > > + /* Get the received frame and create an SKB for it. */ > > + db = fdma_db_next_get(fdma); > > + data_len = fdma_db_len_get(db); > > + > > + skb = napi_alloc_skb(&lan966x->napi, data_len); > > + if (unlikely(!skb)) > > + return NULL; > > + > > + memcpy(skb->data, > > + fdma_dataptr_virt_addr_contiguous(fdma, > > + fdma->dcb_index, > > + fdma->db_index), > > + data_len); > > [Severity: Medium] > BLOCKL is validated in lan966x_fdma_pci_rx_check_frame() through > lan966x_fdma_pci_rx_size_fits(). Here it is read again from the DCB > status in coherent DMA memory. > > Can the value passed to napi_alloc_skb() and memcpy() differ from the > value that was checked? > > It looks like "net: lan966x: add PCIe FDMA XDP support" later in the > series fixes this. That patch reads blockl once in rx_check_frame() and > passes data and data_len to rx_get_frame(). Would it make sense to fold > that change into this patch? > > At this commit the path isn't enabled in-tree yet, because the overlay > has no "fdma" interrupt. Not enabled, yes. Non-issue. > > [ ... ] > > > +static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh, > > + struct net_device *dev) > > +{ > > [ ... ] > > > + next_to_use = lan966x_fdma_pci_get_next_dcb(fdma); > > + > > + if (next_to_use < 0) { > > + netif_stop_queue(dev); > > + return NETDEV_TX_BUSY; > > + } > > [Severity: Medium] > netif_stop_queue() only stops TX queue 0. Each port netdev is created in > lan966x_probe_port() with 8 TX queues: > > dev = devm_alloc_etherdev_mqs(lan966x->dev, > sizeof(struct lan966x_port), > NUM_PRIO_QUEUES, 1); > > There is no ndo_select_queue, so skbs are spread over all 8 queues. > lan966x_fdma_wakeup_netdev(), called from the PCIe NAPI poll, also only > checks and wakes queue 0. > > When the shared TX ring is full and an skb arrives on one of queues 1 to > 7, does that queue ever get stopped? > > It looks like sch_direct_xmit() would requeue the skb and reschedule the > qdisc. net_tx_action would then keep retrying, taking tx_lock and > scanning the whole DCB ring each time, until the hardware completes a > DCB. > > Would netif_tx_stop_all_queues() and netif_tx_wake_all_queues() be a > better fit here? The platform lan966x_fdma_xmit() has the same pattern, > and lan966x_fdma_pci_xmit_xdpf() from "net: lan966x: add PCIe FDMA XDP > support" repeats it. > This is pre-existing behaviour. The lan966x platform path, sparx5 and lan969x all do it the same way. I dont think this is a bug, so if anything, it should go to net-next with at patchset for all platforms together > [ ... ] > > > + /* Order frame write before DCB status write below. */ > > + dma_wmb(); > > + > > + fdma_dcb_add(fdma, > > + next_to_use, > > + 0, > > + FDMA_DCB_STATUS_INTR | > > + FDMA_DCB_STATUS_SOF | > > + FDMA_DCB_STATUS_EOF | > > + FDMA_DCB_STATUS_BLOCKO(0) | > > + FDMA_DCB_STATUS_BLOCKL(IFH_LEN_BYTES + skb->len + ETH_FCS_LEN)); > > [Severity: Medium] > This is a pre-existing issue in the shared fdma_api.c helper and was not > introduced by this patch, but the new backend depends on it. > __fdma_dcb_add() links the new DCB into the chain before terminating it, > and there is no DMA write barrier between the two steps: > > drivers/net/ethernet/microchip/fdma/fdma_api.c:__fdma_dcb_add() { > ... > fdma->last_dcb->nextptr = cpu_to_le64(nextptr); > > fdma->last_dcb = dcb; > > dcb->nextptr = cpu_to_le64(FDMA_DCB_INVALID_DATA); > dcb->info = cpu_to_le64(info); > ... > } > > If the FDMA channel is still walking the chain, could it follow the new > link and read a stale nextptr or info from the new DCB? > > The window is short, and whether the hardware acts on it depends on its > prefetch behavior. When the channel is stopped, the writel() doorbell in > lan966x_fdma_tx_start() or lan966x_fdma_rx_reload() orders the earlier > stores. > > [ ... ] > > > +static int lan966x_fdma_pci_napi_poll(struct napi_struct *napi, int weight) > > +{ > > [ ... ] > > > + dcb_reload = fdma->dcb_index; > > + > > + /* Get all received skbs. */ > > + while (counter < weight) { > > + if (!fdma_has_frames(fdma)) > > + break; > > + /* Order DONE read before DCB/frame reads below. */ > > + dma_rmb(); > > + counter++; > > + switch (lan966x_fdma_pci_rx_check_frame(rx, &src_port)) { > > + case FDMA_PASS: > > + break; > > + case FDMA_ERROR: > > + /* No rx_dropped increment here because src_port is > > + * invalid. > > + */ > > [Severity: Low] > Is this comment accurate for every FDMA_ERROR return? > lan966x_fdma_pci_rx_check_frame() also returns FDMA_ERROR after src_port > and ports[src_port] have already been validated: > > blockl = fdma_db_len_get(db); > if (!lan966x_fdma_pci_rx_size_fits(fdma, blockl)) > return FDMA_ERROR; > > In that case a frame with a bad BLOCKL on a valid port is dropped without > being counted in rx_dropped or rx_length_errors. This code is unchanged > at the end of the series. > > > + fdma_dcb_advance(fdma); > > + continue; > > + } > > [ ... ] > > > + while (dcb_reload != fdma->dcb_index) { > > + old_dcb = dcb_reload; > > + dcb_reload++; > > + dcb_reload &= fdma->n_dcbs - 1; > > [Severity: Medium] > The RX loop is bounded only by counter < weight. Refill progress is > tracked only through the wrapped dcb_reload and dcb_index values. > > What happens if weight >= n_dcbs and every DCB is DONE? n_dcbs is > FDMA_DCB_MAX, which is 512 here and 256 after "net: lan966x: add PCIe > FDMA MTU change support". > > fdma_dcb_advance() would wrap dcb_index back to dcb_reload. DONE bits are > only cleared by this refill loop, so the RX loop would then reprocess > stale DONE descriptors and pass the same frames to napi_gro_receive() > again. > > When weight is a multiple of n_dcbs, dcb_reload equals fdma->dcb_index at > this point. The refill loop then doesn't run at all, and the ring is left > un-armed. > > The normal NAPI weight of 64 can't reach this, but busy polling can. > __napi_busy_loop() passes the socket's budget straight to napi->poll, and > SO_BUSY_POLL_BUDGET accepts values up to U16_MAX with CAP_NET_ADMIN. > > The native lan966x_fdma_napi_poll() has the same structure. This is > still present at the end of the series. This is a re-raise of v7 finding that was answered and dismissed. > > [ ... ] > > > +static int lan966x_fdma_pci_init(struct lan966x *lan966x) > > +{ > > + struct fdma *rx_fdma = &lan966x->rx.fdma; > > + struct fdma *tx_fdma = &lan966x->tx.fdma; > > + int err; > > + > > + if (!lan966x->fdma) > > + return 0; > > [Severity: Low] > At this commit the in-tree PCIe overlay (drivers/misc/lan966x_pci.dtso) > only declares: > > interrupt-names = "xtr", "ana"; > > As a result lan966x->fdma stays false on the PCIe instance, and this > function returns right away. Is it intended that the new backend is > dormant at this commit? > > The "fdma" interrupt is added later in the series by "misc: lan966x-pci: > dts: add fdma interrupt to overlay". Until then the driver keeps using > register-based I/O. > > [ ... ] > > > + lan966x->tx.lan966x = lan966x; > > + tx_fdma->channel_id = FDMA_INJ_CHANNEL; > > + tx_fdma->n_dcbs = FDMA_DCB_MAX; > > [Severity: High] > This isn't a bug introduced by this patch, because the platform > lan966x_fdma_init() uses the same ordering, but the new PCIe backend > inherits it. > > lan966x_probe() sets lan966x->fdma = true, registers every port netdev, > and enables NAPI (lan966x_port_init() -> lan966x_fdma_netdev_init()), all > before it calls lan966x->ops->fdma_init(). > > Here tx_fdma->n_dcbs is set without holding tx_lock. tx_fdma->dcbs is > only allocated later, in lan966x_fdma_pci_tx_alloc(). That happens after > the GFP_KERNEL coherent allocation in lan966x_fdma_pci_rx_alloc(), which > can sleep. > > If a port is opened and transmits during that window, can the following > happen? > > lan966x_port_xmit() > spin_lock(&lan966x->tx_lock); > if (port->lan966x->fdma) > lan966x->ops->fdma_xmit() > lan966x_fdma_pci_xmit() > lan966x_fdma_pci_get_next_dcb() > fdma_db_get(fdma, i, 0) <- fdma->dcbs is NULL > > Later in the same window, xmit could also pick DCBs that > fdma_dcbs_init() has already marked DONE while last_dcb is still being > set up. It could also start the injection channel before > lan966x_fdma_llp_configure() has programmed the TX LLP. > > Would either of these close the window? > > - Calling fdma_init before the ports are registered. > - Publishing n_dcbs under tx_lock, only after the rings are allocated. > This is a re-raise of v7 finding that was answered and dismissed. Probe order is pre-existing and not something this series can fix. > [ ... ] > > > +static int lan966x_fdma_pci_resize(struct lan966x *lan966x) > > +{ > > + return -EOPNOTSUPP; > > +} > > [Severity: Low] > lan966x_port_change_mtu() rolls back the MTU whenever > lan966x->ops->fdma_resize() fails. With this stub, won't every > ndo_change_mtu call fail once PCIe FDMA is active? > > The next patch, "net: lan966x: add PCIe FDMA MTU change support", > replaces the stub with a real implementation. At this commit the path is > dormant, because the overlay has no "fdma" interrupt. > > [ ... ] > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > index 024ce9f9916c..de2202786826 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > @@ -50,6 +50,7 @@ struct lan966x_main_io_resource { > > static const struct lan966x_main_io_resource lan966x_main_iomap[] = { > > { TARGET_CPU, 0xc0000, 0 }, /* 0xe00c0000 */ > > { TARGET_FDMA, 0xc0400, 0 }, /* 0xe00c0400 */ > > + { TARGET_PCIE_DBI, 0x400000, 0 }, /* 0xe0400000 */ > > [Severity: Low] > This entry places regs[TARGET_PCIE_DBI] 4 MB into IO range 0. At this > commit the overlay's "cpu" reg is only 1 MB: > > reg = <0xe0000000 0x0100000>, > > Wouldn't the iATU accesses from fdma_pci_atu_init(), and the region map > and unmap, fall outside the ioremap window? > > This can't be reached at this commit, because lan966x_fdma_pci_init() > returns early without an "fdma" interrupt. Later in the series, "misc: > lan966x-pci: dts: extend cpu reg to cover PCIE DBI space" enlarges the > reg to 0x0800000, and it comes before the "fdma" interrupt is added. On > the SoC instance the pointer is computed but never used. > > [ ... ] > > > @@ -1187,7 +1190,9 @@ static int lan966x_probe(struct platform_device *pdev) > > if (err) > > return dev_err_probe(&pdev->dev, err, "Unable to use ptp irq"); > > > > - lan966x->ptp = 1; > > + /* PTP is not supported on the PCIe path yet. */ > > + if (!lan966x_is_pci(lan966x)) > > + lan966x->ptp = 1; > > } > > [Severity: Low] > With lan966x->ptp left at 0 on PCIe, lan966x_ptp_init() never > initializes port->tx_skbs or the PTP locks. If a "ptp" interrupt exists, > lan966x_ptp_irq_handler has already been registered just above. Could > that handler then touch uninitialized PTP state? > > This can't happen in-tree, because drivers/misc/lan966x_pci.dtso doesn't > declare a "ptp" interrupt. Would it be more robust to also skip the ptp > IRQ request on PCIe? > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com