Re: [PATCH v4 4/5] irqchip/imx-irqsteer: Mask all interrupts in probe()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces an unvalidated loop that performs out-of-bounds MMIO writes if an excessively large `fsl,num-irqs` value is provided.

Pre-existing issues:
- [High] Missing Runtime PM cleanup causes unbalanced clock disable and potential external abort on device unbind.
- [High] Missing synchronization with chained parent IRQ handler in `remove()` leading to potential external abort.
--

--- Patch [4]: [PATCH v4 4/5] irqchip/imx-irqsteer: Mask all interrupts in probe() ---
commit ecdadc89363972642c46a3ef5443dd52db1ecece
Author: Zhipeng Wang <zhipeng.wang_1@nxp.com>

irqchip/imx-irqsteer: Mask all interrupts in probe()

probe() sets up the chained handlers without first masking the input
interrupts. Mask all interrupts in probe() before wiring up the chained
handlers to prevent storms from stale asserted lines.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 320082d3a6325..a9909ecb6fef7 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -236,6 +236,14 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	if (irqsteer_has_chanctrl(data->devtype_data))
>  		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>  
> +	/*
> +	 * Mask all interrupts before wiring up the chained handlers. CHANMASK
> +	 * has inverted polarity (a set bit enables the interrupt), so writing
> +	 * zero masks the source.
> +	 */
> +	for (i = 0; i < data->reg_num; i++)
> +		writel_relaxed(0, data->regs + CHANMASK(i, data->reg_num));
> +

[Severity: High]
Can this loop perform out-of-bounds MMIO writes?

If a large value is provided for "fsl,num-irqs" in the device tree,
data->reg_num could be oversized. The bounds check for the maximum
number of interrupts happens later in the function:

	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
		ret = -EINVAL;
		goto out;
	}

Since this validation executes after the new loop, an oversized value
would cause out-of-bounds writes to data->regs before being caught.

Could the validation check be moved before this loop?

[ ... ]

While reviewing this file, I also noticed a couple of pre-existing issues in
imx_irqsteer_remove().

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_remove() {
...
	for (i = 0; i < irqsteer_data->irq_count; i++) {
		irq_set_chained_handler_and_data(irqsteer_data->irq[i],
						 NULL, NULL);
		irq_dispose_mapping(irqsteer_data->irq[i]);
	}

	clk_disable_unprepare(irqsteer_data->ipg_clk);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but can clearing the chained
handler here race with an executing handler and cause an external abort?

Chained handlers bypass desc->lock and do not set IRQS_INPROGRESS, so there
is no natural synchronization when irq_set_chained_handler_and_data() is
called.

If a parent interrupt fires right before the handler is cleared, the chained
handler might be running concurrently:

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_irq_handler() {
...
	irqmap = readl_relaxed(data->regs +
			       CHANSTATUS(idx, data->reg_num));
...
}

If imx_irqsteer_remove() proceeds to disable the hardware clock while the
handler is still reading the CHANSTATUS MMIO registers, it could crash the
system. Is there a way to synchronize with the executing handler before
disabling the clock?


[Severity: High]
This is a pre-existing issue, but does imx_irqsteer_remove() miss PM
runtime cleanup, leading to an unbalanced clock disable?

The function unconditionally disables the clock with clk_disable_unprepare(),
but fails to call pm_runtime_disable(). If the device is unbound via sysfs,
the driver core calls pm_runtime_put_sync(), which executes
imx_irqsteer_suspend() and disables the clock:

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_suspend() {
...
	imx_irqsteer_save_regs(irqsteer_data);
	clk_disable_unprepare(irqsteer_data->ipg_clk);
}

Then imx_irqsteer_remove() unconditionally disables the clock again, causing
an unbalanced WARN.

Alternatively, if pm_runtime_put_sync() does not immediately suspend the
device, imx_irqsteer_remove() disables the clock but leaves the device active
in PM runtime. A later suspend would call imx_irqsteer_suspend(), which tries
to access hardware registers with the clock already gated, crashing the system.

Should pm_runtime_disable() be called in remove()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819090543.585131-1-Zhipeng.wang_1@oss.nxp.com?part=4