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

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

[Zhipeng.wang_1]

From: Jindong Yue <jindong.yue@nxp.com>

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 existing remove() was incomplete for the module case, where the
driver can now be unloaded and reloaded. Fix it up:

 - dispose the parent IRQ mappings from irq_of_parse_and_map(), which
   were leaked on unload;
 - dispose the child mappings before irq_domain_remove(), otherwise the
   irq_descs keep pointing at imx_irqsteer_irq_chip in the freed module,
   which blows up on the next /proc/interrupts read;
 - resume the device before clk_disable_unprepare(), since it may be
   runtime suspended with the clock already off, and skip the disable if
   the resume fails;
 - disable runtime PM to balance the pm_runtime_enable() from probe().

Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
Changes in v2 (addressing Sashiko AI review of v1):
 - remove(): dispose child and parent IRQ mappings before
   irq_domain_remove(), fixing a use-after-free of imx_irqsteer_irq_chip
   (in freed module memory) on the next /proc/interrupts read after
   unload;
 - remove(): disable runtime PM to balance the pm_runtime_enable() from
   probe(), so reload no longer warns about unbalanced runtime PM; resume
   the device first and skip clk_disable_unprepare() if the resume fails,
   keeping the ipg clock balanced.

v1: https://lore.kernel.org/r/20260724090136.3595894-1-Zhipeng.wang_1@oss.nxp.com
 drivers/irqchip/Kconfig            |  2 +-
 drivers/irqchip/irq-imx-irqsteer.c | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 42f2278a702d..07db3b678f94 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -560,7 +560,7 @@ config CSKY_APB_INTC
 	  the controller's register.
 
 config IMX_IRQSTEER
-	bool "i.MX IRQSTEER support"
+	tristate "i.MX IRQSTEER support"
 	depends on ARCH_MXC || ARCH_S32 || COMPILE_TEST
 	default y if ARCH_MXC || ARCH_S32
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 87b07f517be3..da3889036182 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -10,6 +10,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
@@ -274,7 +275,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 static void imx_irqsteer_remove(struct platform_device *pdev)
 {
 	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
-	int i;
+	int hwirq, i, ret;
 
 	for (i = 0; i < irqsteer_data->irq_count; i++) {
 		if (!irqsteer_data->irq[i])
@@ -282,11 +283,26 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
 
 		irq_set_chained_handler_and_data(irqsteer_data->irq[i],
 						 NULL, NULL);
+		irq_dispose_mapping(irqsteer_data->irq[i]);
 	}
 
+	for (hwirq = 0; hwirq < irqsteer_data->reg_num * 32; hwirq++)
+		irq_dispose_mapping(irq_find_mapping(irqsteer_data->domain,
+						     hwirq));
+
 	irq_domain_remove(irqsteer_data->domain);
 
-	clk_disable_unprepare(irqsteer_data->ipg_clk);
+	/*
+	 * Device may be runtime suspended, with the ipg clock already gated.
+	 * Resume it so the clock is balanced by the disable below; if resume
+	 * fails the clock stays gated, so skip the disable in that case.
+	 */
+	ret = pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
+	if (ret >= 0)
+		clk_disable_unprepare(irqsteer_data->ipg_clk);
 }
 
 #ifdef CONFIG_PM
@@ -349,6 +365,7 @@ static const struct of_device_id imx_irqsteer_dt_ids[] = {
 	{ .compatible = "nxp,s32n79-irqsteer",	.data = &s32n79_data },
 	{},
 };
+MODULE_DEVICE_TABLE(of, imx_irqsteer_dt_ids);
 
 static struct platform_driver imx_irqsteer_driver = {
 	.driver = {
@@ -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);
+
+MODULE_DESCRIPTION("i.MX IRQSTEER interrupt multiplexer/remapper driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

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

[sashiko-bot]

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 <jindong.yue@nxp.com>

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/20260728092219.525449-1-Zhipeng.wang_1@oss.nxp.com?part=1

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

[Frank Li]

On Tue, Jul 28, 2026 at 06:22:19PM +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Jindong Yue <jindong.yue@nxp.com>
>
> 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 existing remove() was incomplete for the module case, where the
> driver can now be unloaded and reloaded. Fix it up:
>
>  - dispose the parent IRQ mappings from irq_of_parse_and_map(), which
>    were leaked on unload;
>  - dispose the child mappings before irq_domain_remove(), otherwise the
>    irq_descs keep pointing at imx_irqsteer_irq_chip in the freed module,
>    which blows up on the next /proc/interrupts read;
>  - resume the device before clk_disable_unprepare(), since it may be
>    runtime suspended with the clock already off, and skip the disable if
>    the resume fails;
>  - disable runtime PM to balance the pm_runtime_enable() from probe().
>
> Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---
> Changes in v2 (addressing Sashiko AI review of v1):
>  - remove(): dispose child and parent IRQ mappings before
>    irq_domain_remove(), fixing a use-after-free of imx_irqsteer_irq_chip
>    (in freed module memory) on the next /proc/interrupts read after
>    unload;
>  - remove(): disable runtime PM to balance the pm_runtime_enable() from
>    probe(), so reload no longer warns about unbalanced runtime PM; resume
>    the device first and skip clk_disable_unprepare() if the resume fails,
>    keeping the ipg clock balanced.
>
> v1: https://lore.kernel.org/r/20260724090136.3595894-1-Zhipeng.wang_1@oss.nxp.com
>  drivers/irqchip/Kconfig            |  2 +-
>  drivers/irqchip/irq-imx-irqsteer.c | 26 +++++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 42f2278a702d..07db3b678f94 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -560,7 +560,7 @@ config CSKY_APB_INTC
>  	  the controller's register.
>
>  config IMX_IRQSTEER
> -	bool "i.MX IRQSTEER support"
> +	tristate "i.MX IRQSTEER support"
>  	depends on ARCH_MXC || ARCH_S32 || COMPILE_TEST
>  	default y if ARCH_MXC || ARCH_S32
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be3..da3889036182 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -10,6 +10,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> @@ -274,7 +275,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  static void imx_irqsteer_remove(struct platform_device *pdev)
>  {
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> -	int i;
> +	int hwirq, i, ret;
>
>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
> @@ -282,11 +283,26 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>
>  		irq_set_chained_handler_and_data(irqsteer_data->irq[i],
>  						 NULL, NULL);
> +		irq_dispose_mapping(irqsteer_data->irq[i]);
>  	}
>
> +	for (hwirq = 0; hwirq < irqsteer_data->reg_num * 32; hwirq++)
> +		irq_dispose_mapping(irq_find_mapping(irqsteer_data->domain,
> +						     hwirq));
> +
>  	irq_domain_remove(irqsteer_data->domain);
>
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	/*
> +	 * Device may be runtime suspended, with the ipg clock already gated.
> +	 * Resume it so the clock is balanced by the disable below; if resume
> +	 * fails the clock stays gated, so skip the disable in that case.
> +	 */
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +
> +	if (ret >= 0)
> +		clk_disable_unprepare(irqsteer_data->ipg_clk);

Is it okay by use devm_pm_rutime_set_acitve_enable() at probe, so you
needn't manage clock.

Frank

>  }
>
>  #ifdef CONFIG_PM
> @@ -349,6 +365,7 @@ static const struct of_device_id imx_irqsteer_dt_ids[] = {
>  	{ .compatible = "nxp,s32n79-irqsteer",	.data = &s32n79_data },
>  	{},
>  };
> +MODULE_DEVICE_TABLE(of, imx_irqsteer_dt_ids);
>
>  static struct platform_driver imx_irqsteer_driver = {
>  	.driver = {
> @@ -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);
> +
> +MODULE_DESCRIPTION("i.MX IRQSTEER interrupt multiplexer/remapper driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>