[PATCH v4 0/5] irqchip/imx-irqsteer: Allow building as module

[PATCH v4 0/5] irqchip/imx-irqsteer: Allow building as module

[Zhipeng.wang_1]

From: Zhipeng Wang <zhipeng.wang_1@nxp.com>

This series makes the i.MX IRQSTEER driver buildable as a module.

v3 was a four-patch series: three fixes for pre-existing bugs that only
become reachable once the driver can be unbound/reloaded, followed by
the module conversion. On v3, Frank Li reviewed the unload-path fixes
and asked for three changes, all addressed here:

 - The probe() error-path irq_domain leak was fixed by hand-rolling an
   err_domain label. Frank suggested a devres-managed domain instead,
   and to add a reusable devm_irq_domain_create_linear() helper so other
   drivers benefit. Patch 1/5 adds that helper (the devres sibling of
   irq_domain_create_linear()); patch 2/5 switches imx-irqsteer to it,
   which fixes the leak structurally and lets remove() drop its explicit
   irq_domain_remove().

 - Frank questioned why remove() disposed of the mappings "twice". The
   two loops disposed of two different sets: the parent output mappings
   the driver created with irq_of_parse_and_map(), and the child input
   mappings the domain handed out. The child mappings are freed by their
   consumers and, with the domain now owned by devres, are torn down
   automatically; only the parent mappings need explicit disposal. Patch
   3/5 disposes of the parent mappings only, dropping the child loop.

 - Frank pointed out that masking in remove() touches CHANMASK while the
   device may be runtime-suspended with the clock gated. The remove()
   masking is unnecessary: the next probe() quiesces the hardware before
   it re-maps and unmasks the parent interrupts, which is the only window
   in which a stale line could storm. Patch 4/5 therefore masks only in
   probe().

Patch 5/5 then converts the driver to a module. As in v3, it lets devres
own the clock and runtime PM (devm_clk_get_enabled() +
devm_pm_runtime_set_active_enabled()).

Changes in v4:
 - New patch 1/5: add devm_irq_domain_create_linear() (Frank Li).
 - Patch 2/5 (was "Fix error handling path in probe()"): use the new
   devm helper to manage the domain instead of hand-rolling an
   err_domain label; remove() drops irq_domain_remove() (Frank Li).
 - Patch 3/5 (was 2/4): dispose of the parent mappings only; the child
   mapping disposal loop is removed (Frank Li).
 - Patch 4/5 (was 3/4): mask interrupts in probe() only; the remove()
   masking, which could touch registers while runtime-suspended, is
   dropped (Frank Li).
 - Patch 5/5 (was 4/4): no functional change; remove() and the probe()
   error path now only dispose of the parent mappings.

v3: https://lore.kernel.org/r/20260807072346.1222389-1-Zhipeng.wang_1@oss.nxp.com
v2: https://lore.kernel.org/r/20260728092219.525449-1-Zhipeng.wang_1@oss.nxp.com
v1: https://lore.kernel.org/r/20260724090136.3595894-1-Zhipeng.wang_1@oss.nxp.com

Jindong Yue (1):
  irqchip/imx-irqsteer: Allow building as module

Zhipeng Wang (4):
  genirq/irqdomain: Add devm_irq_domain_create_linear()
  irqchip/imx-irqsteer: Use devm to manage the IRQ domain
  irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove()
  irqchip/imx-irqsteer: Mask all interrupts in probe()

 drivers/irqchip/Kconfig            |  2 +-
 drivers/irqchip/irq-imx-irqsteer.c | 52 +++++++++++++++++++-----------
 include/linux/irqdomain.h          | 30 +++++++++++++++++
 3 files changed, 64 insertions(+), 20 deletions(-)


base-commit: bd5f485f3f026225b86573e559af0b7254ef4184
-- 
2.34.1

[PATCH v4 1/5] genirq/irqdomain: Add devm_irq_domain_create_linear()

[Zhipeng.wang_1]

From: Zhipeng Wang <zhipeng.wang_1@nxp.com>

irq_domain_create_linear() has no devres-managed counterpart, so every
driver that wants the domain torn down automatically on unbind has to
either open-code an irq_domain_info and call
devm_irq_domain_instantiate() directly, or register a manual devm action.

Add devm_irq_domain_create_linear() as the devres sibling of
irq_domain_create_linear(): it builds the same linear-revmap
irq_domain_info and hands it to devm_irq_domain_instantiate(), so the
domain is removed when the owning device is unbound. The return
convention matches irq_domain_create_linear() (NULL on failure) so
existing callers can switch over without changing their error checks.

Suggested-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 include/linux/irqdomain.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 73c25d40846c..b6b360cb6525 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -457,6 +457,36 @@ static inline struct irq_domain *irq_domain_create_linear(struct fwnode_handle *
 	return IS_ERR(d) ? NULL : d;
 }
 
+/**
+ * devm_irq_domain_create_linear - Allocate and register a linear revmap
+ *				   irq_domain tied to the device lifetime.
+ * @dev:	Device that owns the domain. The domain is removed via devres
+ *		when the device is unbound.
+ * @fwnode:	pointer to interrupt controller's FW node.
+ * @size:	Number of interrupts in the domain.
+ * @ops:	map/unmap domain callbacks
+ * @host_data:	Controller private data pointer
+ *
+ * Returns: Newly created irq_domain, or NULL on failure.
+ */
+static inline struct irq_domain *devm_irq_domain_create_linear(struct device *dev,
+							       struct fwnode_handle *fwnode,
+							       unsigned int size,
+							       const struct irq_domain_ops *ops,
+							       void *host_data)
+{
+	const struct irq_domain_info info = {
+		.fwnode		= fwnode,
+		.size		= size,
+		.hwirq_max	= size,
+		.ops		= ops,
+		.host_data	= host_data,
+	};
+	struct irq_domain *d = devm_irq_domain_instantiate(dev, &info);
+
+	return IS_ERR(d) ? NULL : d;
+}
+
 static inline struct irq_domain *irq_domain_create_tree(struct fwnode_handle *fwnode,
 							const struct irq_domain_ops *ops,
 							void *host_data)
-- 
2.34.1

[PATCH v4 2/5] irqchip/imx-irqsteer: Use devm to manage the IRQ domain

[Zhipeng.wang_1]

From: Zhipeng Wang <zhipeng.wang_1@nxp.com>

probe() creates the IRQ domain with irq_domain_create_linear() and only
tears it down on the remove() path. On the probe() error path after the
domain has been created (the fsl,num-irqs sanity check), the single
error label just calls clk_disable_unprepare() and returns, leaking the
freshly created domain. The domain-creation failure path happens to
share the same label correctly only because the domain is NULL there.

Create the domain with the new devm_irq_domain_create_linear() so the
domain is removed automatically on unbind and on any probe() failure
after it has been created. This fixes the leak on the sanity-check error
path and lets remove() drop its explicit irq_domain_remove().

Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/irqchip/irq-imx-irqsteer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 87b07f517be3..359b8ebc1bae 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -236,8 +236,9 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 	if (irqsteer_has_chanctrl(data->devtype_data))
 		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
 
-	data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
-						&imx_irqsteer_domain_ops, data);
+	data->domain = devm_irq_domain_create_linear(&pdev->dev, dev_fwnode(&pdev->dev),
+						     data->reg_num * 32,
+						     &imx_irqsteer_domain_ops, data);
 	if (!data->domain) {
 		dev_err(&pdev->dev, "failed to create IRQ domain\n");
 		ret = -ENOMEM;
@@ -284,8 +285,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
 						 NULL, NULL);
 	}
 
-	irq_domain_remove(irqsteer_data->domain);
-
 	clk_disable_unprepare(irqsteer_data->ipg_clk);
 }
 
-- 
2.34.1

[PATCH v4 3/5] irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove()

[Zhipeng.wang_1]

From: Zhipeng Wang <zhipeng.wang_1@nxp.com>

probe() maps the parent output interrupts with irq_of_parse_and_map(),
but remove() only unchains the handlers and never disposes of those
mappings, leaking them on unbind. The child mappings handed out by the
domain are freed by their consumers and, together with the domain, are
now torn down by devres, so remove() only has to dispose of the parent
mappings it created itself.

Dispose of the parent mappings alongside the chained-handler teardown.

Fixes: 0136afa08967 ("irqchip: Add driver for imx-irqsteer controller")
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/irqchip/irq-imx-irqsteer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 359b8ebc1bae..320082d3a632 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -283,6 +283,7 @@ 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]);
 	}
 
 	clk_disable_unprepare(irqsteer_data->ipg_clk);
-- 
2.34.1

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

[Zhipeng.wang_1]

From: Zhipeng Wang <zhipeng.wang_1@nxp.com>

probe() sets up the chained handlers without first masking the input
interrupts. For a built-in driver this happened to be harmless because
CHANMASK resets to all-masked, but once the driver can be unloaded and
reloaded a child interrupt left unmasked at unload time survives in
hardware. On the next probe() the parent interrupts are re-mapped and
unmasked before the new domain is ready, so a still-asserted line
immediately storms the parent with no handler to service it.

Mask all interrupts in probe() before wiring up the chained handlers.
CHANMASK uses inverted polarity (a set bit enables the interrupt), so
masking means writing zero. This mirrors the sibling NXP chained mux
irq-imx-intmux.c, which masks all sources at probe() time.

Masking is only done in probe(), not in remove(): the next probe()
quiesces the hardware before it re-maps and unmasks the parent
interrupts, which is the only window in which a stale line could storm.
Masking in remove() would also mean touching CHANMASK while the device
may already be runtime-suspended with the clock gated.

Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/irqchip/irq-imx-irqsteer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 320082d3a632..a9909ecb6fef 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));
+
 	data->domain = devm_irq_domain_create_linear(&pdev->dev, dev_fwnode(&pdev->dev),
 						     data->reg_num * 32,
 						     &imx_irqsteer_domain_ops, data);
-- 
2.34.1

[PATCH v4 5/5] 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().

Now that the driver can be unloaded and reloaded, let the driver core
own the clock and runtime PM lifetime so that the probe() error path and
remove() do not have to hand-balance them:

 - acquire the clock with devm_clk_get_enabled() instead of a bare
   devm_clk_get() followed by a manual clk_prepare_enable(), so it is
   prepared/enabled for the device lifetime and released on unbind;
 - keep only clk_enable()/clk_disable() in the runtime PM callbacks,
   since prepare/unprepare is now handled once by devres;
 - enable runtime PM with devm_pm_runtime_set_active_enabled(), which
   marks the device active (matching the enabled clock) and disables
   runtime PM on unbind.

With the clock, runtime PM and IRQ domain all owned by devres, remove()
and the probe() error path only have to dispose of the parent IRQ
mappings.

Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/irqchip/Kconfig            |  2 +-
 drivers/irqchip/irq-imx-irqsteer.c | 36 +++++++++++++++++-------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 20b77fbc51ee..105108d2e6ff 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -555,7 +555,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 a9909ecb6fef..a60cc527e619 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>
@@ -193,7 +194,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 		return PTR_ERR(data->regs);
 	}
 
-	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+	data->ipg_clk = devm_clk_get_enabled(&pdev->dev, "ipg");
 	if (IS_ERR(data->ipg_clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(data->ipg_clk),
 				     "failed to get ipg clk\n");
@@ -226,12 +227,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
-	ret = clk_prepare_enable(data->ipg_clk);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
-		return ret;
-	}
-
 	/* steer all IRQs into configured channel */
 	if (irqsteer_has_chanctrl(data->devtype_data))
 		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
@@ -271,12 +266,21 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
+	if (ret)
+		goto err_irq;
 
 	return 0;
+
+err_irq:
+	for (i = 0; i < data->irq_count; i++) {
+		if (!data->irq[i])
+			break;
+
+		irq_set_chained_handler_and_data(data->irq[i], NULL, NULL);
+		irq_dispose_mapping(data->irq[i]);
+	}
 out:
-	clk_disable_unprepare(data->ipg_clk);
 	return ret;
 }
 
@@ -293,8 +297,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
 						 NULL, NULL);
 		irq_dispose_mapping(irqsteer_data->irq[i]);
 	}
-
-	clk_disable_unprepare(irqsteer_data->ipg_clk);
 }
 
 #ifdef CONFIG_PM
@@ -324,7 +326,7 @@ static int imx_irqsteer_suspend(struct device *dev)
 	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
 
 	imx_irqsteer_save_regs(irqsteer_data);
-	clk_disable_unprepare(irqsteer_data->ipg_clk);
+	clk_disable(irqsteer_data->ipg_clk);
 
 	return 0;
 }
@@ -334,7 +336,7 @@ static int imx_irqsteer_resume(struct device *dev)
 	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(irqsteer_data->ipg_clk);
+	ret = clk_enable(irqsteer_data->ipg_clk);
 	if (ret) {
 		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
 		return ret;
@@ -357,6 +359,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 = {
@@ -367,4 +370,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 v4 3/5] irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove()

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbalanced PM runtime teardown leads to clock underflow on driver unbind.
--

--- Patch [3]: [PATCH v4 3/5] irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove() ---
commit 58fbfcc1ec63bf5165fed17c40b2a3a04a6235ef
Author: Zhipeng Wang <zhipeng.wang_1@nxp.com>

irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove()

This commit fixes a resource leak in the imx-irqsteer driver by properly
disposing of parent output interrupt mappings during the remove callback.
It ensures that the mappings created in probe are torn down when the
driver unbinds.

> 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
> @@ -283,6 +283,7 @@ 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]);
>  	}
>  
>  	clk_disable_unprepare(irqsteer_data->ipg_clk);

[Severity: High]
This is a pre-existing issue, but does this clock disable cause an underflow
when unbinding the driver?

When the driver unbinds, the driver core calls pm_runtime_put_sync(), which
suspends the device and calls imx_irqsteer_suspend(). That suspend function
already calls clk_disable_unprepare(ipg_clk).

Immediately after, imx_irqsteer_remove() executes and unconditionally calls
clk_disable_unprepare() again on the already suspended device. Can this
double disable lead to a clock count underflow WARN and potentially disable
a shared clock?

Additionally, since imx_irqsteer_remove() lacks a pm_runtime_disable()
call, will leaving the PM runtime disable depth unbalanced cause an
unbalanced pm_runtime_enable warning if the driver is later rebound?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Unloading the module while the device is runtime-suspended causes a clock enable count underflow and kernel WARN.
- [High] Spurious parent interrupts during runtime suspend cause a bus error/system crash due to unprotected hardware access.
- [High] Missing synchronize_irq() in remove() leads to a Use-After-Free race condition with in-flight chained handlers.
- [High] An unvalidated device tree property `fsl,num-irqs` leads to an out-of-bounds memory write.
--

--- Patch [5]: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module ---
commit 4fdbf892274cef735a839f80bcfd62708a887cbc
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(). Let the driver core own the clock and
runtime PM lifetime using devres.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index a9909ecb6fef7..a60cc527e6191 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c

[ ... ]

> @@ -193,7 +194,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  		return PTR_ERR(data->regs);
>  	}
>  
> -	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	data->ipg_clk = devm_clk_get_enabled(&pdev->dev, "ipg");

[Severity: High]
This is a pre-existing issue, but since the patch modifies
imx_irqsteer_probe(), could the fsl,num-irqs property cause an out-of-bounds
write?

In imx_irqsteer_probe(), data->reg_num is calculated from fsl,num-irqs and
then used in a loop to clear CHANMASK:

for (i = 0; i < data->reg_num; i++)
        writel_relaxed(0, data->regs + CHANMASK(i, data->reg_num));

However, the bounds check to ensure data->irq_count does not exceed
CHAN_MAX_OUTPUT_INT happens after this loop. Could a large value in the
device tree lead to writing past the mapped IOMEM region?

>  	if (IS_ERR(data->ipg_clk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(data->ipg_clk),
>  				     "failed to get ipg clk\n");

[ ... ]

> @@ -293,8 +297,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  						 NULL, NULL);

[Severity: High]
This is a pre-existing issue, but when clearing the chained handlers here,
is there a risk of a use-after-free?

The chained handler is cleared via irq_set_chained_handler_and_data(), but
this does not wait for handlers executing on other CPUs to finish. When
devres subsequently unmaps the I/O memory, could an in-flight
imx_irqsteer_irq_handler() concurrently read data->regs and cause a bus
fault? Would calling synchronize_irq() be needed here?

>  		irq_dispose_mapping(irqsteer_data->irq[i]);
>  	}
> -
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
>  }

[Severity: High]
This is a pre-existing issue, but does removing manual clock management in
favor of devres introduce a clock enable count underflow during unbind?

If the module is unloaded while the device is auto-suspended, the clock is
disabled by imx_irqsteer_suspend(). Because the device is left in a
suspended state when imx_irqsteer_remove() returns, devm_clk_get_enabled()
teardown will unconditionally call clk_disable_unprepare().

Since pm_runtime_disable() does not automatically resume the device, will
this result in clk_disable() being called with an enable count of 0? Should
we call pm_runtime_get_sync() in imx_irqsteer_remove() to ensure the clock
is active before devres cleanup?

>  
>  #ifdef CONFIG_PM
> @@ -324,7 +326,7 @@ static int imx_irqsteer_suspend(struct device *dev)
>  	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
>  
>  	imx_irqsteer_save_regs(irqsteer_data);
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	clk_disable(irqsteer_data->ipg_clk);

[Severity: High]
This is a pre-existing issue, but since we are modifying the clock disable
logic in suspend, what happens if a spurious interrupt arrives while the
device is suspended?

imx_irqsteer_irq_handler() unconditionally reads the CHANSTATUS register
using readl_relaxed(). If imx_irqsteer_suspend() has disabled the hardware
clock, wouldn't reading from an unclocked peripheral trigger a bus error or
system crash?

Does the interrupt handler need to use pm_runtime_get_if_active() to verify
the power state before attempting hardware access?

>  
>  	return 0;
>  }

[ ... ]

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

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

[sashiko-bot]

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

Re: [PATCH v4 1/5] genirq/irqdomain: Add devm_irq_domain_create_linear()

[Frank Li]

On Wed, Aug 19, 2026 at 06:05:39PM +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Zhipeng Wang <zhipeng.wang_1@nxp.com>
>
> irq_domain_create_linear() has no devres-managed counterpart, so every
> driver that wants the domain torn down automatically on unbind has to
> either open-code an irq_domain_info and call
> devm_irq_domain_instantiate() directly, or register a manual devm action.
>
> Add devm_irq_domain_create_linear() as the devres sibling of
> irq_domain_create_linear(): it builds the same linear-revmap
> irq_domain_info and hands it to devm_irq_domain_instantiate(), so the
> domain is removed when the owning device is unbound. The return
> convention matches irq_domain_create_linear() (NULL on failure) so
> existing callers can switch over without changing their error checks.
>
> Suggested-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  include/linux/irqdomain.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 73c25d40846c..b6b360cb6525 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -457,6 +457,36 @@ static inline struct irq_domain *irq_domain_create_linear(struct fwnode_handle *
>  	return IS_ERR(d) ? NULL : d;
>  }
>
> +/**
> + * devm_irq_domain_create_linear - Allocate and register a linear revmap
> + *				   irq_domain tied to the device lifetime.
> + * @dev:	Device that owns the domain. The domain is removed via devres
> + *		when the device is unbound.
> + * @fwnode:	pointer to interrupt controller's FW node.
> + * @size:	Number of interrupts in the domain.
> + * @ops:	map/unmap domain callbacks
> + * @host_data:	Controller private data pointer
> + *
> + * Returns: Newly created irq_domain, or NULL on failure.
> + */
> +static inline struct irq_domain *devm_irq_domain_create_linear(struct device *dev,
> +							       struct fwnode_handle *fwnode,
> +							       unsigned int size,
> +							       const struct irq_domain_ops *ops,
> +							       void *host_data)
> +{
> +	const struct irq_domain_info info = {
> +		.fwnode		= fwnode,
> +		.size		= size,
> +		.hwirq_max	= size,
> +		.ops		= ops,
> +		.host_data	= host_data,
> +	};
> +	struct irq_domain *d = devm_irq_domain_instantiate(dev, &info);
> +
> +	return IS_ERR(d) ? NULL : d;
> +}
> +
>  static inline struct irq_domain *irq_domain_create_tree(struct fwnode_handle *fwnode,
>  							const struct irq_domain_ops *ops,
>  							void *host_data)
> --
> 2.34.1
>
>

Re: [PATCH v4 2/5] irqchip/imx-irqsteer: Use devm to manage the IRQ domain

[Frank Li]

On Wed, Aug 19, 2026 at 06:05:40PM +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Zhipeng Wang <zhipeng.wang_1@nxp.com>
>
> probe() creates the IRQ domain with irq_domain_create_linear() and only
> tears it down on the remove() path. On the probe() error path after the
> domain has been created (the fsl,num-irqs sanity check), the single
> error label just calls clk_disable_unprepare() and returns, leaking the
> freshly created domain. The domain-creation failure path happens to
> share the same label correctly only because the domain is NULL there.
>
> Create the domain with the new devm_irq_domain_create_linear() so the
> domain is removed automatically on unbind and on any probe() failure
> after it has been created. This fixes the leak on the sanity-check error
> path and lets remove() drop its explicit irq_domain_remove().
>
> Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")

Nit: First patch also need fixes tags to backport.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be3..359b8ebc1bae 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -236,8 +236,9 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	if (irqsteer_has_chanctrl(data->devtype_data))
>  		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>
> -	data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
> -						&imx_irqsteer_domain_ops, data);
> +	data->domain = devm_irq_domain_create_linear(&pdev->dev, dev_fwnode(&pdev->dev),
> +						     data->reg_num * 32,
> +						     &imx_irqsteer_domain_ops, data);
>  	if (!data->domain) {
>  		dev_err(&pdev->dev, "failed to create IRQ domain\n");
>  		ret = -ENOMEM;
> @@ -284,8 +285,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  						 NULL, NULL);
>  	}
>
> -	irq_domain_remove(irqsteer_data->domain);
> -
>  	clk_disable_unprepare(irqsteer_data->ipg_clk);
>  }
>
> --
> 2.34.1
>
>

Re: [PATCH v4 3/5] irqchip/imx-irqsteer: Dispose of parent IRQ mappings in remove()

[Frank Li]

On Wed, Aug 19, 2026 at 06:05:41PM +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Zhipeng Wang <zhipeng.wang_1@nxp.com>
>
> probe() maps the parent output interrupts with irq_of_parse_and_map(),
> but remove() only unchains the handlers and never disposes of those
> mappings, leaking them on unbind. The child mappings handed out by the
> domain are freed by their consumers and, together with the domain, are
> now torn down by devres, so remove() only has to dispose of the parent
> mappings it created itself.
>
> Dispose of the parent mappings alongside the chained-handler teardown.
>
> Fixes: 0136afa08967 ("irqchip: Add driver for imx-irqsteer controller")
> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/irqchip/irq-imx-irqsteer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 359b8ebc1bae..320082d3a632 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -283,6 +283,7 @@ 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]);
>  	}
>
>  	clk_disable_unprepare(irqsteer_data->ipg_clk);
> --
> 2.34.1
>
>

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

[Frank Li]

On Wed, Aug 19, 2026 at 09:25:06AM +0000, sashiko-bot@kernel.org 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 <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;
> 	}
>

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

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

[Fabio Estevam]

On Wed, Aug 19, 2026 at 11:43 AM Frank Li <Frank.li@oss.nxp.com> wrote:

> Make sense, this check should just after
>
>         data->irq_count = DIV_ROUND_UP(irqs_num, 64);
>         data->reg_num = irqs_num / 32;

This is what I did at:

https://lore.kernel.org/lkml/20260817190621.335492-2-festevam@gmail.com/

Zhipeng, feel free to include my two patches in your series.

You also forgot to CC the drivers/irqchip maintainer.