Re: [PATCH v2] irqchip/imx-irqsteer: Allow building as module

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

New issues:
- [High] CHANMASK registers are not initialized to zero in probe(), which causes an infinite interrupt storm upon module reload.

Pre-existing issues:
- [High] The irq_domain created in probe() is leaked on an error path, causing a Use-After-Free kernel panic on module unload.
- [High] The chained interrupt handler returns without calling chained_irq_exit() on an error path, permanently masking the parent interrupt.
- [High] The chained IRQ handler accesses hardware registers without verifying if the device is active, violating Runtime PM contracts and causing bus faults.
--

--- Patch [1]: [PATCH v2] irqchip/imx-irqsteer: Allow building as module ---
commit c3af83d8dda390fd5164c60f844d81afdbc265df
Author: Jindong Yue <[email protected]>

irqchip/imx-irqsteer: Allow building as module

Make the driver buildable as a module by turning the Kconfig symbol into
a tristate and using module_platform_driver() instead of
builtin_platform_driver(). The remove function is updated to support
module unloading and reloading.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
[ ... ]
> @@ -359,4 +376,7 @@ static struct platform_driver imx_irqsteer_driver = {
>  	.probe		= imx_irqsteer_probe,
>  	.remove		= imx_irqsteer_remove,
>  };
> -builtin_platform_driver(imx_irqsteer_driver);
> +module_platform_driver(imx_irqsteer_driver);

[Severity: High]
Are the CHANMASK registers properly initialized during module reload?

Since this patch allows the driver to be unloaded and reloaded as a module,
during module removal, the driver disposes of the child interrupt mappings but
does not clear the hardware CHANMASK registers.

On the next module reload, imx_irqsteer_probe() re-maps the parent interrupts,
which unmasks them at the parent controller. Because the CHANMASK registers
retain their previous state, any active child interrupt will immediately trigger
the parent interrupt.

Will this result in an infinite interrupt storm since the interrupt is never
cleared? Should the hardware initialization in imx_irqsteer_probe() clear
pending interrupts and mask registers before setting up the chained parent
interrupts?

[Severity: High]
This is a pre-existing issue, but does imx_irqsteer_probe() leak the irq_domain
on an error path?

If the validation of data->irq_count fails in imx_irqsteer_probe():

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

The function returns an error without freeing the newly created irq_domain,
leaving it registered in the system's global irq_domain_list.

Because this patch converts the driver to a module, the module can now be
unloaded after this failure, freeing the memory containing
imx_irqsteer_domain_ops.

Will any subsequent traversal of interrupt domains cause a Use-After-Free kernel
panic when it dereferences the freed ops pointer?

[Severity: High]
This is a pre-existing issue, but does imx_irqsteer_irq_handler() permanently
mask the parent interrupt on its error path?

When the chained handler executes, chained_irq_enter() masks and acknowledges
the parent interrupt. If imx_irqsteer_get_hwirq_base() fails:

        if (hwirq < 0) {
                pr_warn("%s: unable to get hwirq base for irq %d\n",
                        __func__, irq);
                return;
        }

The handler returns immediately without calling chained_irq_exit().

If a spurious or unexpected interrupt number is dispatched by the parent
controller, does this missing unmask cause a complete loss of all interrupts
multiplexed through this line?

[Severity: High]
This is a pre-existing issue, but does imx_irqsteer_irq_handler() access
hardware registers without checking the PM state?

The irq_domain_set_pm_device() mechanism automatically drops PM runtime
references when no child interrupts are active, placing the irqsteer device into
runtime suspend with its ipg_clk disabled. However, the chained parent interrupt
remains unconditionally active at the primary controller.

If a spurious interrupt arrives on the parent interrupt line while the device is
suspended, imx_irqsteer_irq_handler() will execute and unconditionally read
the hardware register:

        irqmap = readl_relaxed(data->regs +
                               CHANSTATUS(idx, data->reg_num));

Could accessing unclocked hardware cause a synchronous external abort
(bus fault) or return 0xffffffff?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1