Re: [PATCH net-next v6 13/14] misc: lan966x-pci: dts: extend cpu reg to cover PCIE DBI space

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
> 
> - [Low] Claimed patch-ordering / bisectability bug: the "cpu" reg window
>   is enlarged from 0x100000 to 0x800000 only in this commit, while the…
> 
> --
> 
> misc: lan966x-pci: dts: extend cpu reg to cover PCIE DBI space
> 
> This grows the "cpu" register window of the switch node in the lan966x PCI
> device tree overlay from 0x100000 to 0x800000 so that the PCIE DBI and iATU
> registers at offset 0x400000+ are covered. Those registers are programmed
> when the FDMA engine sets up the ATU outbound windows.
> 
> No Link: tags in the commit header.
> 
> > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > index 7b196b0a0eb6..7bb726550caf 100644
> > --- a/drivers/misc/lan966x_pci.dtso
> > +++ b/drivers/misc/lan966x_pci.dtso
> > @@ -135,7 +135,7 @@ lan966x_phy1: ethernet-lan966x_phy@2 {
> >
> >                               switch: switch@e0000000 {
> >                                       compatible = "microchip,lan966x-switch";
> > -                                     reg = <0xe0000000 0x0100000>,
> > +                                     reg = <0xe0000000 0x0800000>,
> >                                             <0xe2000000 0x0800000>;
> >                                       reg-names = "cpu", "gcb";
> 
> [Severity: Low]
> 
> This isn't a bug, just a question about the ordering of the series.
> 
> The earlier commits "net: lan966x: add PCIe FDMA support", "net: lan966x: add
> PCIe FDMA MTU change support" and "net: lan966x: add PCIe FDMA XDP support"
> already derive regs[TARGET_PCIE_DBI] from the "cpu" base plus 0x400000, and
> fdma_pci_atu_init() writes at DBI + 0x300000 + idx * 0x200:
> 
> drivers/net/ethernet/microchip/fdma/fdma_pci.c
> static void __iomem *fdma_pci_atu_addr_get(void __iomem *addr, int offset, int idx)
> {
>         return addr + FDMA_PCI_ATU_ADDR + FDMA_PCI_ATU_IDX_SIZE * idx + offset;
> }
> 
> That lands roughly 0x700000 from the "cpu" base, i.e. outside the 0x100000
> mapping that exists until this commit. Would it be clearer to move this
> overlay change before the driver commits that compute the DBI offset, so
> that the mapping is never smaller than the offsets the driver derives from
> it?

Yes, intentional: the window is widened here and only the next commit names the
"fdma" interrupt that lets anything use it.

> 
> For what it is worth, the intermediate commits do look safe in practice,
> since the ATU access in lan966x_fdma_pci_init() is gated:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
>         if (!lan966x->fdma)
>                 return 0;
>         ...
>         fdma_pci_atu_init(&lan966x->atu, lan966x->regs[TARGET_PCIE_DBI]);
> 
> and lan966x->fdma is only set when the node names an "fdma" interrupt in
> lan966x_probe():
> 
>         lan966x->fdma_irq = platform_get_irq_byname(pdev, "fdma");
>         if (lan966x->fdma_irq > 0) {
>                 ...
>                 lan966x->fdma = true;
>         }
> 
> The overlay here still carries only interrupt-names = "xtr", "ana", and the
> "fdma" interrupt is added by the following commit, so the enlarged window is
> in place before the FDMA path can be enabled. Is that ordering intentional?

Yes.

> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com