Re: [PATCH net-next v9 10/15] net: lan966x: add shutdown callback to stop the FDMA on reboot

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: lan966x_shutdown() disables lan966x->napi and then
  releases rtnl.
- [Low] Concern: lan966x_fdma_rx_disable() and lan966x_fdma_tx_disable()
  ignore the return value of readx_poll_timeout_atomic() on…
- [Low] Concern: after lan966x_shutdown(), a devmap or bpf_redirect from
  another NIC could reach lan966x_xdp_xmit() -> lan966x_fdma_xmit_xdpf()…

--

net: lan966x: add shutdown callback to stop the FDMA on reboot

This commit adds a platform shutdown callback for the PCIe instance of
lan966x. The callback frees the irqs, masks the analyzer interrupt,
disables NAPI, detaches the netdevs, stops both FDMA channels and unmaps
the outbound ATU windows. It also restores the full FDMA_INTR_ENA enable
in lan966x_fdma_rx_start(), because that register persists across a warm
reboot.

> NAPI is skipped when fdma_ndev is unset (a probed switch with no
> usable port never adds one), and XDP attach cannot re-enter the FDMA
> either: on PCIe, lan966x_xdp_setup() returns before the page pool
> reload, which is the only point where it touches the FDMA.

[Severity: Low]
Is this accurate at this point in the series? At this commit,
lan966x_xdp_setup() checks only lan966x->fdma before the reload:

lan966x_xdp.c:lan966x_xdp_setup() {
	if (!lan966x->fdma) {
		NL_SET_ERR_MSG_MOD(xdp->extack,
				   "Allow to set xdp only when using fdma");
		return -EOPNOTSUPP;
	}
	...
	err = lan966x_fdma_reload_page_pool(lan966x);
	...
}

Suppose lan966x_shutdown() has called napi_disable() and dropped rtnl.
An XDP attach or detach would then reach:

  lan966x_xdp_setup()
    lan966x_fdma_reload_page_pool()
      lan966x_fdma_reload()
        napi_synchronize(&lan966x->napi);
        napi_disable(&lan966x->napi);

That would hang on the already disabled NAPI with rtnl held. It could
also re-arm RX through lan966x_fdma_rx_start().

In practice this doesn't look reachable at this revision.
lan966x_probe() sets lan966x->fdma only when an "fdma" interrupt exists.
The in-tree lan966x_pci.dtso overlay doesn't declare one until the last
patch of the series ("misc: lan966x-pci: dts: add fdma interrupt to
overlay"). So lan966x_shutdown() returns before napi_disable(), and XDP
attach returns -EOPNOTSUPP.

The lan966x_is_pci() early return in lan966x_xdp_setup() is added later
by "net: lan966x: add PCIe FDMA support" and "net: lan966x: add PCIe
FDMA XDP support". Could the commit message describe the state at this
revision, or say that the early return is added later in the series?

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 259d81e75907..024ce9f9916c 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -1324,9 +1324,65 @@ static void lan966x_remove(struct platform_device *pdev)
>  	debugfs_remove_recursive(lan966x->debugfs_root);
>  }
>  
> +static void lan966x_shutdown(struct platform_device *pdev)
> +{
> +	struct lan966x *lan966x = platform_get_drvdata(pdev);
> +

[ ... ]

> +	if (!lan966x->fdma)
> +		return;
> +
> +	rtnl_lock();
> +
> +	if (lan966x->fdma_ndev)
> +		napi_disable(&lan966x->napi);
> +
> +	for (int p = 0; p < lan966x->num_phys_ports; p++) {
> +		if (!lan966x->ports[p] || !lan966x->ports[p]->dev)
> +			continue;
> +
> +		netif_tx_disable(lan966x->ports[p]->dev);
> +		netif_device_detach(lan966x->ports[p]->dev);
> +	}
> +
> +	lan966x_fdma_rx_disable(&lan966x->rx);
> +	lan966x_fdma_tx_disable(&lan966x->tx);

[Severity: Low]
Can anything re-arm the TX channel after this point?
lan966x_fdma_tx_disable() clears tx->activated without taking tx_lock.
A later devmap or bpf_redirect from another NIC would then go:

  lan966x_xdp_xmit()
    lan966x_fdma_xmit_xdpf()
      lan966x_fdma_tx_start()
        lan966x->tx.activated = true;
        lan966x_fdma_tx_activate(tx);

As far as I can tell this path is blocked:

- At this commit, PCIe instances have lan966x->fdma == false with the
  in-tree overlay, so shutdown never reaches this code.

- At the end of the series, lan966x_probe_port() adds
  NETDEV_XDP_ACT_REDIRECT and NETDEV_XDP_ACT_NDO_XMIT only when
  !lan966x_is_pci(). __xdp_enqueue() in kernel/bpf/devmap.c rejects
  devices that lack NETDEV_XDP_ACT_NDO_XMIT.

- Regular xmit is drained by netif_tx_disable() above. The PCIe XDP_TX
  path runs only from NAPI, which is disabled first.

Is the xdp_features gate the intended guarantee that nothing re-arms
this channel after shutdown?

> +
> +	lan_wr(0, lan966x, FDMA_INTR_ENA);
> +	lan_wr(0, lan966x, FDMA_INTR_DB_ENA);
> +
> +#if IS_ENABLED(CONFIG_MCHP_LAN966X_PCI)
> +	fdma_pci_atu_region_unmap(lan966x->rx.fdma.atu_region);
> +	fdma_pci_atu_region_unmap(lan966x->tx.fdma.atu_region);
> +#endif

[Severity: Low]
This isn't a bug, but lan966x_fdma_rx_disable() and
lan966x_fdma_tx_disable() both discard the result of the FDMA_CH_ACTIVE
poll:

	readx_poll_timeout_atomic(lan966x_fdma_channel_active, lan966x,
				  val, !(val & BIT(fdma->channel_id)),
				  READL_SLEEP_US, READL_TIMEOUT_US);

So the ATU windows are unmapped here even if a channel never went idle,
and nothing is logged. READL_TIMEOUT_US is 100 s, so a timeout means the
engine is stuck. Unmapping the windows is still the right step in that
case.

A stale FDMA error carried into the next kernel is not a concern either.
lan966x_reset_switch() clears FDMA_ERRORS, FDMA_INTR_ERR and
FDMA_INTR_DB before lan966x_probe() requests the fdma irq.

Would a warning on timeout help make a stuck FDMA visible at shutdown?
The disable helpers themselves predate this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com