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

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

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.

quoted

[ ... ]

quoted

@@ -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);
+}
+

[ ... ]

quoted

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

...