Re: [PATCH] irqchip: Convert chained handlers to guard_chained_irq()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [High] irqchip/imx-intmux: unclocked MMIO read in
  imx_intmux_irq_handler() [1]
- [High] irqchip/imx-irqsteer: unprotected hardware register access in
  imx_irqsteer_irq_handler() [2]
--

commit 8af6be36eba9f36b4a716dc486539968188a4f42
Author: Peng Fan <peng.fan@nxp.com>

irqchip: Convert chained handlers to guard_chained_irq()

This commit converts chained IRQ handlers across drivers/irqchip/ to use
the scope-based guard_chained_irq() helper instead of explicit
chained_irq_enter() and chained_irq_exit() calls.

[ ... ]

> diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
> index c7914fea3d1f9..73926dec52abf 100644
> --- a/drivers/irqchip/irq-imx-intmux.c
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -187,15 +187,13 @@ static void imx_intmux_irq_handler(struct irq_desc *desc)
>  	unsigned long irqstat;
>  	int pos;
>  
> -	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	guard_chained_irq(irq_desc_get_chip(desc), desc);
>  
>  	/* read the interrupt source pending status of this channel */
>  	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
>  
>  	for_each_set_bit(pos, &irqstat, 32)
>  		generic_handle_domain_irq(irqchip_data->domain, pos);
> -
> -	chained_irq_exit(irq_desc_get_chip(desc), desc);
>  }
>  
>  static int imx_intmux_probe(struct platform_device *pdev)

[Severity: High]
This problem wasn't introduced by this patch, but can
imx_intmux_irq_handler() perform an unclocked MMIO read [1] of
CHANIPR(idx) when the device is runtime-suspended?

irq_set_chained_handler_and_data() leaves the upstream parent IRQ unmasked
while runtime PM disables ipg_clk when no child interrupts are active.  A
spurious interrupt on the parent line invokes imx_intmux_irq_handler()
without checking pm_runtime_active(), causing a bus fault and external
abort.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 4e0f7eac0c8b5..e117c8a1102d3 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -148,14 +148,14 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>  	int hwirq;
>  	int irq, i;
>  
> -	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	guard_chained_irq(irq_desc_get_chip(desc), desc);
>  
>  	irq = irq_desc_get_irq(desc);
>  	hwirq = imx_irqsteer_get_hwirq_base(data, irq);
>  	if (hwirq < 0) {
>  		pr_warn("%s: unable to get hwirq base for irq %d\n",
>  			__func__, irq);
> -		goto out;
> +		return;
>  	}
>  
>  	for (i = 0; i < 2; i++, hwirq += 32) {
> @@ -172,9 +172,6 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>  		for_each_set_bit(pos, &irqmap, 32)
>  			generic_handle_domain_irq(data->domain, pos + hwirq);
>  	}
> -
> -out:
> -	chained_irq_exit(irq_desc_get_chip(desc), desc);
>  }
>  
>  static int imx_irqsteer_probe(struct platform_device *pdev)

[Severity: High]
This problem wasn't introduced by this patch, but can
imx_irqsteer_irq_handler() read CHANSTATUS while the device is
runtime-suspended [2] and ipg_clk is gated?

If a spurious or shared interrupt fires on the parent IRQ line while
imx-irqsteer is suspended, reading data->regs + CHANSTATUS(idx,
data->reg_num) without checking pm_runtime_active() accesses unclocked
registers and triggers a fatal bus fault or system hang.

[1] https://sashiko.dev/#/bug/linux-ade1ea4a-cd13-49ad-9345-1f0f1b497bba
[2] https://sashiko.dev/#/bug/linux-3ace51f1-8ee3-4863-912d-72f04665b41b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-irqchip-cleanup-api-v1-1-4a9d598b0bdb@nxp.com?part=1