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

On Wed, Sep 23, 2026 at 10:07:12PM +0200, Daniel Machon wrote:

quoted

On Wed, Sep 23, 2026 at 11:46:22AM +0200, Daniel Machon wrote:

quoted

quoted

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.

quoted

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>