Re: [PATCH 06/12] soc: fsl: qe: Iterate over all pending interrupts in cascade handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Off-by-one error in hardware IRQ calculation causes out-of-bounds domain resolution and misrouted/dropped interrupts.
--

commit 151384fe21d3660403f9ea1eecce7c7057939a64
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Iterate over all pending interrupts in cascade handler

This commit updates the chained interrupt handler for the QE PIC to process
all pending interrupts in a single run. It replaces a single-interrupt read
using ffs() with a loop over all set bits in the event register.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 3bca116fd2f5e..27b62f4e3afb1 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -67,25 +68,17 @@ static struct irq_chip qepic = {
[ ... ]
> -static int qepic_get_irq(struct irq_desc *desc)
> -{
> -	struct qepic_data *data = irq_desc_get_handler_data(desc);
> -	u32 event = in_be32(data->reg + CEPIER);
> -
> -	if (!event)
> -		return -1;
> -
> -	return 32 - ffs(event);
> -}
> -
>  static void qepic_cascade(struct irq_desc *desc)
>  {
>  	struct qepic_data *data = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long event, bit;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	generic_handle_domain_irq(data->host, qepic_get_irq(desc));
> +	event = in_be32(data->reg + CEPIER);
> +	for_each_set_bit(bit, &event, 32)
> +		generic_handle_domain_irq(data->host, 32 - bit);

[Severity: High]
Will this calculation map to the correct hardware IRQ?

The original qepic_get_irq() code used ffs(event), which is 1-indexed and
returns a value from 1 to 32. With for_each_set_bit(), the bit variable is
0-indexed and returns a value from 0 to 31.

Because bit is 0-indexed, if bit 0 is set, the new calculation evaluates to
32 - 0 = 32, which is out of bounds for the 0-31 hwirq range. All other mapped
interrupts appear to be shifted by +1.

Could this result in misrouted interrupts and leave the actual pending hardware
interrupt unserviced? If the interrupt is not cleared, would it cause an
interrupt storm as soon as the parent IRQ handler exits and unmasks it?

Should this subtraction be 31 - bit instead?

>  
>  	chained_irq_exit(chip, desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=6