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

[Daniel Machon]

> 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

Pre-existing. Lan966x_remove() is a platform function. It's a re-raise of v7
11/14 / v8 15/15, the probe/remove ordering. The issue is real, but out-of-scope
for this series.