Re: [PATCH 2/3] irqchip/econet-en751221: Chain the handler to all parent interrupts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] irqchip/econet-en751221: local parent variable shadows parameter
  in econet_intc_of_init()
- [Low] irqchip/econet-en751221: missing Fixes trailer on non-VEIC IRQ
  chaining fix
--

commit 341fd2b16f35f804366b103223f85a72065ed255
Author: Akshat Singh <automobilehacks@gmail.com>

> irqchip/econet-en751221: Chain the handler to all parent interrupts
>
> The EN751221 interrupt controller does not drive one CPU interrupt line
> per source. It presents the number of the pending interrupt plus one, in
> binary, on the CPU interrupt lines IP2-IP7. In VEIC mode the CPU takes
> that number as the vector, but without VEIC the driver chains its handler
> to the first parent interrupt only, IP2 in the existing device trees. An
> odd-numbered interrupt then never interrupts the CPU by itself: it is
> handled only when an even-numbered one happens to run the handler, which
> on an idle system is the next timer interrupt. That covers the frame
> engine's QDMA (21), xHCI (17) and the first PCIe port (23).
>
> On a Nokia G-140W-C, whose 34Kc does not report VEIC, this delayed
> received packets by up to 10 ms, and ping round-trip times to the device
> cycled through 1-11 ms.
>
> Without VEIC, chain the handler to every parent interrupt listed in the
> device tree. One interrupt usually raises several lines, so the later
> runs of the handler find nothing pending; don't count those as spurious.
>
> VEIC mode is unchanged.
>
> Signed-off-by: Akshat Singh <automobilehacks@gmail.com>

[Severity: Low]
This fixes broken delivery of odd-numbered interrupts on non-VEIC systems.
Should this include:

  Fixes: 1902a59cf5f9 ("irqchip: Add EcoNet EN751221 INTC")

> diff --git a/drivers/irqchip/irq-econet-en751221.c b/drivers/irqchip/irq-econet-en751221.c
> index 2ca5d901866fe..5930c67c37261 100644
> --- a/drivers/irqchip/irq-econet-en751221.c
> +++ b/drivers/irqchip/irq-econet-en751221.c

[ ... ]

> @@ -461,7 +473,22 @@ static int __init econet_intc_of_init(struct device_node *node, struct device_no
>  		if (ret)
>  			return ret;
>  	} else {
> +		int nr_parents = of_irq_count(node);
> +
>  		irq_set_chained_handler_and_data(irq, econet_intc_from_parent, domain);
> +
> +		for (int i = 1; i < nr_parents; i++) {
> +			int parent = irq_of_parse_and_map(node, i);

[Severity: Low]
Does declaring the local variable parent here trigger a -Wshadow warning
under W=2 builds because econet_intc_of_init() already takes a
struct device_node *parent parameter?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-en751221-intc-all-lines-v1-0-dc03a40f47f0@gmail.com?part=2