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

[Daniel Machon]

> 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.

Hi Simon,

Yes, sorry for being vague here.

#4 refers to patch 4, where I explained that the issue reported there was really
due to the existing probe initialization order and netdev registration order.
Sashiko seems to report multiple race issues that can happen since the netdevs
are registered before the HW is fully initialized. This is real, and something
that needs to be fixed, just not in this series, IMHO.

For Sparx5 and lan969x, I fixed this not long ago, and made sure that netdevs
were registered last in probe, as they should be. Now, lan966x should get the
same treatment, as a separate fix series for net. :-)

Thanks for the review!

> 
> >
> > >
> > > [ ... ]
> > >
> > > > @@ -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.
> 
> ...