Re: [PATCH 01/12] soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unrecoverable infinite IRQ storm and spurious processing due to unmasked and unmapped interrupts.
- [High] Race condition in shared interrupt controller registers due to missing spinlock protection.
- [High] Missing synchronize_irq() after clearing the chained handler allows use-after-free of the qepic_data structure.
--

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

soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler

This commit wraps the cascade handler body with chained_irq_enter() and
chained_irq_exit(). This properly informs the parent IRQ chip that a chained
interrupt is being serviced.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 9b0bba64e91e4..73a77763210d7 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -79,7 +80,13 @@ static int qepic_get_irq(struct irq_desc *desc)
>  
>  static void qepic_cascade(struct irq_desc *desc)
>  {
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +	chained_irq_enter(chip, desc);
> +
>  	generic_handle_irq(qepic_get_irq(desc));
[Severity: High]
This isn't a bug introduced by this patch, but can this lead to an unrecoverable
infinite IRQ storm?

In qepic_get_irq(), the code reads the raw CEPIER event register without masking
it against the CEPIMR mask register:

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 irq_find_mapping(data->host, 32 - ffs(event));
}

If an unmapped interrupt fires (e.g., left unmasked by the bootloader),
irq_find_mapping() returns 0. Then, generic_handle_irq(0) is called, which
typically returns -EINVAL without clearing the hardware event bit in CEPIER.

When chained_irq_exit() unmasks the parent, will the uncleared event immediately
re-trigger the parent interrupt and cause a storm?

> +
> +	chained_irq_exit(chip, desc);
>  }

[Severity: High]
This is a pre-existing issue, but is there a race condition in the shared
interrupt controller registers due to a missing spinlock?

The mask and unmask functions modify the shared CEPIMR register using non-atomic
macros:

static void qepic_mask(struct irq_data *d)
{
	struct qepic_data *data = irq_data_get_irq_chip_data(d);

	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
}

Since the Linux IRQ core invokes these callbacks holding only the per-child
desc->lock (and not a chip-level lock), concurrent mask or unmask operations on
different child IRQs belonging to the same controller could race on SMP systems.

Could this lead to corrupted mask or type registers, resulting in lost or
stuck interrupts?

[Severity: High]
This isn't a bug introduced by this patch, but does the devres cleanup path risk
a use-after-free by missing synchronize_irq()?

During driver removal or probe failure, qepic_remove() is called:

static void qepic_remove(void *res)
{
	struct qepic_data *data = res;

	irq_set_chained_handler_and_data(data->irq, NULL, NULL);
	irq_domain_remove(data->host);
}

This removes the chained handler, but fails to call synchronize_irq() to ensure
concurrent hardware interrupt execution has terminated. Immediately after this
returns, the devres framework will free the qepic_data structure.

If qepic_cascade() is concurrently executing on another CPU, won't it dereference
the freed data pointer, causing a use-after-free crash?

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