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

On Wed, Aug 19, 2026 at 09:25:06AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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;
> 	}
>

Make sense, this check should just after

	data->irq_count = DIV_ROUND_UP(irqs_num, 64);
        data->reg_num = irqs_num / 32;

Frank

> 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/[email protected]?part=4