Re: [PATCH net-next v6 11/14] net: lan966x: add PCIe FDMA MTU change support

[Daniel Machon]

> 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