Re: [PATCH net-next v6 08/14] net: lan966x: clear FDMA interrupt stickies after switch reset

[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] Commit-message/comment vs. reachability at this revision: the
>   added FDMA sticky clear in lan966x_reset_switch()…
> 
> --
> 
> net: lan966x: clear FDMA interrupt stickies after switch reset
> 
> The patch acks FDMA_ERRORS, FDMA_INTR_ERR and FDMA_INTR_DB right after
> reset_control_reset() in lan966x_reset_switch(), so that spurious sticky
> bits latched by the GCB soft reset in PCI mode do not make the FDMA IRQ
> handler fire as soon as the IRQ is requested. The clear is unconditional and
> is described as a no-op on the platform path.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 6e6c08bb8eea..11094a381ec2 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -1067,6 +1067,15 @@ static int lan966x_reset_switch(struct lan966x *lan966x)
> >
> >       reset_control_reset(switch_reset);
> >
> > +     /* When in PCI mode, the GCB soft reset issued by the reset
> > +      * controller can latch spurious bits in the FDMA error stickies.
> > +      * Clear them before request_irq hooks up the FDMA IRQ line,
> > +      * otherwise the handler fires immediately on probe.
> > +      */
> > +     lan_wr(lan_rd(lan966x, FDMA_ERRORS), lan966x, FDMA_ERRORS);
> > +     lan_wr(lan_rd(lan966x, FDMA_INTR_ERR), lan966x, FDMA_INTR_ERR);
> > +     lan_wr(lan_rd(lan966x, FDMA_INTR_DB), lan966x, FDMA_INTR_DB);
> 
> [Severity: Low]
> This isn't a bug in the register writes themselves, but is the failure
> described in the changelog reachable at this point in the series?

No, but this is a preparation patch that makes sure that once the FDMA support
lands, in a later patch, the failure is not introduced.

> 
> The commit message says the latched bits "sit in FDMA_INTR_ERR until the FDMA
> IRQ is requested later in probe, at which point the handler fires immediately
> and WARNs". In lan966x_probe() the FDMA handler is only installed when a named
> "fdma" interrupt exists:
> 
>         lan966x->fdma_irq = platform_get_irq_byname(pdev, "fdma");
>         if (lan966x->fdma_irq > 0) {
>                 err = devm_request_irq(&pdev->dev, lan966x->fdma_irq,
>                                        lan966x_fdma_irq_handler, 0,
>                                        "fdma irq", lan966x);
>                 ...
>                 lan966x->fdma = true;
>         }
> 
> At this commit the PCI overlay in drivers/misc/lan966x_pci.dtso does not
> describe that interrupt:
> 
>         interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
>                      <9 IRQ_TYPE_LEVEL_HIGH>;
>         interrupt-names = "xtr", "ana";
> 
> so platform_get_irq_byname(pdev, "fdma") fails and lan966x_fdma_irq_handler()
> is never requested on the PCI path.
> 
> The overlay also disagrees with the positional list in
> Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml, where the
> second entry is reserved for "fdma":
> 
>         interrupt-names:
>           minItems: 1
>           items:
>             - const: xtr
>             - const: fdma
>             - const: ana
> 
> Both points are resolved later in the same series by "misc: lan966x-pci: dts:
> add fdma interrupt to overlay", which switches the overlay to
> interrupts <12>,<14>,<9> and interrupt-names = "xtr", "fdma", "ana", so this is
> only an observation about the ordering of the changelog against the code state
> here rather than an actionable defect.
> 
> Would it help to mention in the commit message that the WARN only becomes
> reachable once the overlay gains the "fdma" interrupt?

The changelog already scopes itself with "When in PCI mode".

> 
> >       /* Don't reinitialize the switch core, if it is already initialized. In
> >        * case it is initialized twice, some pointers inside the queue system
> >        * in HW will get corrupted and then after a while the queue system gets
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com