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

[Simon Horman <horms@kernel.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.

...