Re: [PATCH net-next v9 15/15] misc: lan966x-pci: dts: add fdma interrupt to overlay

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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

- [Medium] Once this overlay adds the "fdma" interrupt, PCIe instances
  use lan966x_fdma_pci_ops, and a teardown ordering bug in that backend…

--

misc: lan966x-pci: dts: add fdma interrupt to overlay

This commit adds the fdma interrupt (OIC interrupt 14) to the lan966x PCI
device tree overlay. This turns on FDMA-based frame injection and extraction
when the switch is connected over PCIe.

> diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> index 7bb726550cafd..5bb12dbc08431 100644
> --- a/drivers/misc/lan966x_pci.dtso
> +++ b/drivers/misc/lan966x_pci.dtso
> @@ -141,8 +141,9 @@ switch: switch@e0000000 {
>  
>  					interrupt-parent = <&oic>;
>  					interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
> +						     <14 IRQ_TYPE_LEVEL_HIGH>,
>  						     <9 IRQ_TYPE_LEVEL_HIGH>;
> -					interrupt-names = "xtr", "fdma", "ana";
> +					interrupt-names = "xtr", "fdma", "ana";

[Severity: Medium]
Once the "fdma" interrupt is present, PCIe instances use
lan966x_fdma_pci_ops. Does that make a teardown ordering problem in the PCIe
FDMA backend reachable?

lan966x_remove() tears down the FDMA before it unregisters the netdevs:

lan966x_main.c:lan966x_remove() {
    ...
	lan966x->ops->fdma_deinit(lan966x);
	lan966x_cleanup_ports(lan966x);
    ...
}

lan966x_fdma_pci_deinit() disables NAPI and frees both rings. It does not
clear lan966x->rx.lan966x, it does not detach the netdevs, and rtnl is not
held at this point:

lan966x_fdma_pci.c:lan966x_fdma_pci_deinit() {
	if (lan966x->fdma_ndev)
		napi_disable(&lan966x->napi);
    ...
	fdma_free_coherent_and_unmap(lan966x->dma_dev, &lan966x->rx.fdma);
	fdma_free_coherent_and_unmap(lan966x->dma_dev, &lan966x->tx.fdma);
}

fdma_free_coherent_and_unmap() only clears atu_region, so dcbs and dma
still hold their old values.

netif_set_mtu_ext() only checks netif_device_present(). That means an MTU
change that alters the max frame size can still run during this window:

lan966x_port_change_mtu()
  lan966x_fdma_pci_resize()
    __lan966x_fdma_pci_reload()
      lan966x_fdma_pci_reload()
        napi_disable(&lan966x->napi)

The resize guard is only this check:

	if (!lan966x->rx.lan966x)
		return 0;

rx.lan966x is still non-NULL after deinit. Would the second napi_disable()
on an already disabled NAPI spin forever while holding rtnl? That would also
block the unregister_netdev() that lan966x_remove() calls next.

The probe error path in lan966x_fdma_pci_init() looks like it has a similar
gap:

	err = lan966x_fdma_pci_tx_alloc(&lan966x->tx);
	if (err) {
		fdma_free_coherent_and_unmap(lan966x->dma_dev, rx_fdma);
		return err;
	}

rx.lan966x has already been set earlier in lan966x_fdma_pci_init(). The
netdevs registered by lan966x_probe_port() stay live until the cleanup_ports
label.

If an MTU change arrives in that window, NAPI is still enabled from
lan966x_port_init(). lan966x_fdma_pci_reload() then allocates new rings and
does:

	fdma_free_coherent_and_unmap(lan966x->dma_dev, &rx_fdma_old);

Is this a second dma_free_coherent() of the RX ring that the error path
already freed?

After that, reload re-arms the FDMA on the new rings. The probe error path
never calls fdma_deinit for them. Would the coherent memory for the new rings
leak, with the outbound ATU windows still mapped and DMA active after probe
fails?

The resize guard added in "net: lan966x: add PCIe FDMA MTU change support"
is meant to skip the resize until lan966x_fdma_pci_init() has built the
rings. It doesn't seem to cover rings that were torn down or only partly
built.

The shutdown callback from "net: lan966x: add shutdown callback to stop the
FDMA on reboot" avoids this by taking rtnl and calling netif_device_detach()
so that ndo_open and ndo_change_mtu can't re-enter the FDMA. Should the
remove and probe error paths get the same protection?

The platform backend already has the same remove ordering. The PCIe deinit,
resize and reload code is new in this series, though, and this overlay change
is what first makes it reachable.

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