[PATCH net-next v6 11/14] net: lan966x: add PCIe FDMA MTU change support
Daniel Machon
daniel.machon at microchip.com
Tue Sep 15 03:23:27 PDT 2026
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
>
> Critical: 0 · High: 2 · Medium: 1 · Low: 1
>
> - [High] NULL pointer dereference in the new lan966x_fdma_pci_reset_mem()
> restore path.
> - [High] Unbalanced/unguarded napi_disable()/napi_enable() in the new MTU
> reload path.
> - [Medium] Netdev MTU contract mismatch on the PCIe FDMA path.
> - [Low] Inverted restart ordering in lan966x_fdma_pci_reload(): both the
> success path (lines 458-460) and the restore path (lines 476-478) call…
>
> --
>
> 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 buffers, falling back to resetting and
> reusing the existing buffers if the reallocation fails. It also caps the
> PCIe DCB ring at 256 entries so the contiguous allocation stays below
> MAX_PAGE_ORDER at jumbo MTU.
>
> A few questions about the new reload and resize paths below.
>
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > index f1f3c789d3a6..6cabbb8b47f2 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > @@ -1,8 +1,15 @@
> > // SPDX-License-Identifier: GPL-2.0+
> >
> > +#include <linux/mmzone.h>
> > +
> > #include "fdma_api.h"
> > #include "lan966x_main.h"
> >
> > +/* Ring must fit in one MAX_PAGE_ORDER DMA block; 512 DCBs overflows
> > + * at jumbo MTU.
> > + */
> > +#define FDMA_PCI_DCB_MAX 256
> > +
>
> [ ... ]
>
> > @@ -374,9 +381,164 @@ 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);
>
> [Severity: High]
> Can tx->fdma.atu_region be NULL here?
>
> lan966x_fdma_pci_reset_mem() is only reached from the restore label of
> lan966x_fdma_pci_reload(), and that label is taken when the ring
> allocation failed. Probe registers the netdevs from
> lan966x_probe_port() -> register_netdev() inside the ports loop, while
> lan966x->fdma is already true, and ops->fdma_init() (which sets
> n_dcbs/db_size/size and maps the ATU regions) only runs after that loop.
>
> An MTU change in that window passes the !lan966x->fdma guard in
> lan966x_port_change_mtu() and reaches lan966x_fdma_pci_resize() with a
> zeroed fdma:
>
> rx.max_mtu == 0, so the "max_mtu == lan966x->rx.max_mtu" early return
> does not fire
>
> n_dcbs == 0, so fdma_get_size_contiguous() returns ALIGN(0, PAGE_SIZE)
> == 0 and both -ERANGE guards pass
>
> lan966x_fdma_pci_reload() then recomputes size, which stays 0, and:
>
> lan966x_fdma_pci_rx_alloc()
> fdma_alloc_coherent_and_map()
> fdma_pci_atu_region_map()
> if (size <= 0)
> return ERR_PTR(-EINVAL);
>
> so control reaches restore, memcpy's the still-zeroed fdma structs back
> (atu_region == NULL, dcbs == NULL, size == 0), and calls reset_mem().
> memset(NULL, 0, 0) and fdma_dcbs_init() with n_dcbs == 0 are both
> no-ops, so nothing stops execution before tx->fdma.atu_region->base_addr
> is evaluated. Would a NULL check on atu_region (or an early bail in
> resize() when the FDMA is not initialized yet) be appropriate here?
Real and the proper fix is to reorder initialization like we did on sparx5 in
b8909aad5b8d ("net: sparx5: move netdev and notifier block registration to
probe"), where the FDMA irq is requested and the FDMA initialized in
sparx5_frame_io_init() and netdev registration happens last.
For now, I can add a small check that makes sure that MTU change bails out in
case FDMA is not initialized yet.
>
> > + lan966x_fdma_llp_configure(lan966x,
> > + rx->fdma.atu_region->base_addr,
> > + rx->fdma.channel_id);
> > +}
> > +
> > +/* Wake all TX queues on every port (undoes lan966x_fdma_tx_disable_netdev). */
> > +static void lan966x_fdma_pci_wakeup_netdev(struct lan966x *lan966x)
> > +{
> > + for (int i = 0; i < lan966x->num_phys_ports; ++i) {
> > + struct lan966x_port *port = lan966x->ports[i];
> > +
> > + if (port)
> > + netif_tx_wake_all_queues(port->dev);
> > + }
> > +}
> > +
> > +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);
>
> [Severity: High]
> Should this napi_disable() be guarded the way the other users of
> lan966x->napi in this driver are?
>
> lan966x_fdma_pci_deinit() does:
>
> if (lan966x->fdma_ndev)
> napi_disable(&lan966x->napi);
>
> and lan966x_shutdown() has the same guard, with a comment noting that the
> reload paths disable this NAPI under rtnl. Two states look problematic
> for the unguarded call:
>
> The NAPI may not have been added yet. netif_napi_add() only runs from
> lan966x_fdma_netdev_init(), called by lan966x_port_init(), which happens
> after lan966x_probe_port() already did register_netdev(). An MTU change
> in that window reaches napi_disable() with n->dev == NULL (lan966x is
> devm_kzalloc'ed), and napi_disable() does netdev_lock(n->dev).
>
I believe the fix mentioned above also covers this.
> The NAPI may already be disabled. lan966x_remove() calls
> ops->fdma_deinit() (which disables the NAPI and frees/unmaps both rings)
> before lan966x_cleanup_ports() unregisters the netdevs, and
> lan966x_shutdown() disables the NAPI without clearing fdma_ndev. A
> concurrent MTU change then calls napi_disable() a second time and
> napi_disable_locked() spins:
>
> net/core/dev.c:napi_disable_locked() {
> ...
> while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
> usleep_range(20, 200);
> val = READ_ONCE(n->state);
> }
> ...
> }
>
> There is no timeout and no other wake condition, and ndo_change_mtu
> holds rtnl throughout, which also blocks the unregister_netdev() that
> would end the window. The matching napi_enable() calls on both exit
> paths below have the same issue.
>
I'd say pre-existing. In order to fix this race, we need a proper net fix to
reorder the probe init/deinit paths.
> > + lan966x_fdma_tx_disable_netdev(lan966x);
> > + lan966x_fdma_rx_disable(&lan966x->rx);
> > + lan966x_fdma_tx_disable(&lan966x->tx);
> > +
> > + lan966x->rx.max_mtu = new_mtu;
> > +
> > + /* Must be NULL'ed in order to realloc them. */
> > + lan966x->rx.fdma.atu_region = NULL;
> > + lan966x->tx.fdma.atu_region = NULL;
> > +
> > + lan966x->tx.fdma.db_size = FDMA_PCI_DB_SIZE(lan966x->rx.max_mtu);
> > + lan966x->tx.fdma.size = fdma_get_size_contiguous(&lan966x->tx.fdma);
> > + lan966x->rx.fdma.db_size = FDMA_PCI_DB_SIZE(lan966x->rx.max_mtu);
> > + lan966x->rx.fdma.size = fdma_get_size_contiguous(&lan966x->rx.fdma);
> > +
> > + err = lan966x_fdma_pci_rx_alloc(&lan966x->rx);
> > + if (err)
> > + goto restore;
> > +
> > + err = lan966x_fdma_pci_tx_alloc(&lan966x->tx);
> > + if (err) {
> > + fdma_free_coherent_and_unmap(lan966x->dma_dev,
> > + &lan966x->rx.fdma);
> > + goto restore;
> > + }
> > +
> > + /* Free and unmap old memory. */
> > + fdma_free_coherent_and_unmap(lan966x->dma_dev, &rx_fdma_old);
> > + fdma_free_coherent_and_unmap(lan966x->dma_dev, &tx_fdma_old);
> > +
> > + napi_enable(&lan966x->napi);
> > + lan966x_fdma_rx_start(&lan966x->rx);
> > + lan966x_fdma_pci_wakeup_netdev(lan966x);
>
> [Severity: Low]
> This isn't a bug as such, but the restart order here is the reverse of
> the pre-existing non-PCI path. lan966x_fdma_reload() does:
>
> lan966x_fdma_rx_start(&lan966x->rx);
> ...
> lan966x_fdma_wakeup_netdev(lan966x);
> napi_enable(&lan966x->napi);
>
> Once napi_enable() clears NAPI_STATE_DISABLE, a pending FDMA DB
> interrupt reaching napi_schedule() from lan966x_fdma_irq_handler() can
> run lan966x_fdma_pci_napi_poll() on another CPU concurrently with
> lan966x_fdma_rx_start(). Both then do unsynchronized read-modify-write
> on FDMA_INTR_DB_ENA:
>
> lan966x_fdma_pci_napi_poll():
> if (counter < weight && napi_complete_done(napi, counter))
> lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
>
> lan966x_fdma_rx_start() instead does lan_rd() followed by lan_rmw() on
> the same register, and the poll can also issue FDMA_CH_RELOAD for a
> channel rx_start() has not activated yet. Would matching the ordering
> used by lan966x_fdma_reload() be preferable?
>
The order is deliberate - it was swapped in v6 after an earlier round of this
review flagged the platform order as the live bug: waking the queues before NAPI
is live lets a TX clobber the extraction bit, with no poll scheduled to restore
it. v7 will carry a comment saying so.
> > +
> > + return err;
> > +restore:
> > +
> > + /* No new buffers are allocated at this point. Use the old buffers,
> > + * but reset them before starting the FDMA again.
> > + */
> > +
> > + memcpy(&lan966x->tx.fdma, &tx_fdma_old, sizeof(struct fdma));
> > + memcpy(&lan966x->rx.fdma, &rx_fdma_old, sizeof(struct fdma));
> > +
> > + lan966x->rx.max_mtu = old_mtu;
> > +
> > + lan966x_fdma_pci_reset_mem(lan966x);
> > +
> > + napi_enable(&lan966x->napi);
> > + lan966x_fdma_rx_start(&lan966x->rx);
> > + lan966x_fdma_pci_wakeup_netdev(lan966x);
> > +
> > + return err;
> > +}
> > +
>
> [ ... ]
>
> > static int lan966x_fdma_pci_resize(struct lan966x *lan966x)
> > {
> > - return -EOPNOTSUPP;
> > + struct fdma rx_fdma;
> > + int max_mtu;
> > +
> > + max_mtu = lan966x_fdma_get_max_frame(lan966x);
> > + if (max_mtu == lan966x->rx.max_mtu)
> > + return 0;
> > +
> > + /* rx and tx have n_dbs == 1, so both rings need the same contiguous
> > + * dma_alloc_coherent() block, which can't exceed MAX_PAGE_ORDER. The
> > + * allocation is padded to the ATU region granularity, so test the
> > + * padded size.
> > + */
> > + rx_fdma = lan966x->rx.fdma;
> > + rx_fdma.db_size = FDMA_PCI_DB_SIZE(max_mtu);
> > + if (ALIGN(fdma_get_size_contiguous(&rx_fdma),
> > + FDMA_PCI_ATU_REGION_ALIGN) > (PAGE_SIZE << MAX_PAGE_ORDER))
> > + return -ERANGE;
>
> [Severity: Medium]
> Does the MTU range the netdev advertises still match what this path can
> actually do?
>
> lan966x_probe_port() sets dev->max_mtu = ETH_MAX_MTU for the PCIe
> instantiation too, but with n_dcbs == FDMA_PCI_DCB_MAX (256),
> n_dbs == FDMA_RX_DCB_MAX_DBS (1) and db_size == ALIGN(max_frame, 128),
> the padded contiguous allocation crosses PAGE_SIZE << MAX_PAGE_ORDER at
> roughly 15.7 KiB of MTU on a 4 KiB-page build, so larger requests get a
> bare -ERANGE that lan966x_port_change_mtu() passes straight to
> userspace, and the cut-off moves with PAGE_SIZE and MAX_PAGE_ORDER.
The dev->max_mtu = ETH_MAX_MTU is intentional. Max MTU is rejected at runtime
and scales with PAGE_SIZE and MAX_PAGE_ORDER, depending on kernel configuration.
>
> There is a second effect for MTUs in [65518, 65535], which the
> advertised max_mtu still permits. lan966x_port_change_mtu() programs
> the register before calling the resize callback:
>
> lan_wr(DEV_MAC_MAXLEN_CFG_MAX_LEN_SET(LAN966X_HW_MTU(new_mtu)),
> lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));
>
> DEV_MAC_MAXLEN_CFG_MAX_LEN is GENMASK(15, 0) and LAN966X_HW_MTU(mtu) is
> mtu + 18, so FIELD_PREP() masks the runtime value, e.g. mtu 65535
> programs MAX_LEN = 65553 & 0xffff = 17. lan966x_fdma_pci_resize() then
> derives max_mtu from that already-truncated register via
> lan966x_fdma_get_max_frame() -> lan966x_fdma_get_max_mtu() ->
> lan_rd(DEV_MAC_MAXLEN_CFG), so it either returns 0 (multi-port case,
> where the max over ports is unchanged) or reloads with a tiny db_size,
> and the MTU change is reported as successful while the MAC is configured
> for a 17-byte frame.
True and pre-existing in the platform path. If I decide to cap at dev->max_mtu,
which requires a PCI special case (lan966x_is_pci()), I might as well fix this
one by ETH_MAX_MTU - (ETH_HLEN + ETH_FCS_LEN).
>
> Before this patch the PCIe callback returned -EOPNOTSUPP for every
> resize, so lan966x_port_change_mtu() always rolled the register back;
> this patch is what makes the path reachable. Would capping dev->max_mtu
> for the PCIe path (to both the ring limit and the 16-bit MAC field) be
> the better fix, so the resize-time rejection becomes unreachable?
>
> > +
> > + /* db_size is also handed to the FDMA in the 16-bit DCB DATAL field,
> > + * where a larger value would be silently truncated.
> > + */
> > + if (rx_fdma.db_size > GENMASK(15, 0))
> > + return -ERANGE;
> > +
> > + return __lan966x_fdma_pci_reload(lan966x, max_mtu);
> > }
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com
More information about the linux-arm-kernel
mailing list