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 10:07:12PM +0200, Daniel Machon wrote:
> > 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.

Understood. I think that from the perspective of avoiding scope-creep
your approach makes sense.

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

Reviewed-by: Simon Horman <horms@kernel.org>