* [PATCH v3 2/2] irqchip/imx-irqsteer: Validate IRQ count before creating domain
2026-08-05 19:27 [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable() Fabio Estevam
@ 2026-08-05 19:27 ` Fabio Estevam
2026-08-05 20:37 ` Frank Li
2026-08-05 20:36 ` [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable() Frank Li
2026-08-06 21:28 ` Radu Rendec
2 siblings, 1 reply; 5+ messages in thread
From: Fabio Estevam @ 2026-08-05 19:27 UTC (permalink / raw)
To: tglx; +Cc: radu, Frank.Li, imx, linux-kernel, Fabio Estevam
From: Fabio Estevam <festevam@nabladev.com>
The IRQ count is validated after creating the IRQ domain. If it is
invalid, probe returns without removing the domain, leaving its host
data pointing at devm-managed memory that is freed on probe failure.
Validate the count before allocating resources to avoid the leak and
dangling pointer.
Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
Signed-off-by: Fabio Estevam <festevam@nabladev.com>
---
Changes since v2:
- Newly introduced.
drivers/irqchip/irq-imx-irqsteer.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 653e25115083..55aec60dee40 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -217,6 +217,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
*/
data->irq_count = DIV_ROUND_UP(irqs_num, 64);
data->reg_num = irqs_num / 32;
+ if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT)
+ return -EINVAL;
if (IS_ENABLED(CONFIG_PM)) {
data->saved_reg = devm_kzalloc(&pdev->dev,
@@ -250,11 +252,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
}
irq_domain_set_pm_device(data->domain, &pdev->dev);
- if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
- ret = -EINVAL;
- goto out;
- }
-
for (i = 0; i < data->irq_count; i++) {
data->irq[i] = irq_of_parse_and_map(np, i);
if (!data->irq[i])
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable()
2026-08-05 19:27 [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable() Fabio Estevam
2026-08-05 19:27 ` [PATCH v3 2/2] irqchip/imx-irqsteer: Validate IRQ count before creating domain Fabio Estevam
@ 2026-08-05 20:36 ` Frank Li
2026-08-06 21:28 ` Radu Rendec
2 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2026-08-05 20:36 UTC (permalink / raw)
To: Fabio Estevam; +Cc: tglx, radu, Frank.Li, imx, linux-kernel, Fabio Estevam
On Wed, Aug 05, 2026 at 04:27:42PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
>
> imx_irqsteer_probe() enables runtime PM, but imx_irqsteer_remove() does
> not disable it. Consequently, runtime PM remains enabled after unbinding
> the device, and rebinding it triggers:
>
> Unbalanced pm_runtime_enable!
>
> Use devm_pm_runtime_enable() to automatically disable runtime PM when
> the device is removed. Set up runtime PM before creating the IRQ domain
> and registering chained handlers so that a failure cannot leave either
> resource pointing at freed driver data.
>
> Fixes: 4730d2233311 ("irqchip/imx-irqsteer: Add runtime PM support")
> Signed-off-by: Fabio Estevam <festevam@nabladev.com>
> ---
> Changes since v1:
> - Move devm_pm_runtime_enable() prior to irq_domain_create_linear(). (Frank)
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> drivers/irqchip/irq-imx-irqsteer.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be3..653e25115083 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -236,6 +236,11 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> if (irqsteer_has_chanctrl(data->devtype_data))
> writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>
> + pm_runtime_set_active(&pdev->dev);
> + ret = devm_pm_runtime_enable(&pdev->dev);
> + if (ret)
> + goto out;
> +
> data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
> &imx_irqsteer_domain_ops, data);
> if (!data->domain) {
> @@ -262,9 +267,6 @@ 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);
> -
> return 0;
> out:
> clk_disable_unprepare(data->ipg_clk);
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable()
2026-08-05 19:27 [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable() Fabio Estevam
2026-08-05 19:27 ` [PATCH v3 2/2] irqchip/imx-irqsteer: Validate IRQ count before creating domain Fabio Estevam
2026-08-05 20:36 ` [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable() Frank Li
@ 2026-08-06 21:28 ` Radu Rendec
2 siblings, 0 replies; 5+ messages in thread
From: Radu Rendec @ 2026-08-06 21:28 UTC (permalink / raw)
To: Fabio Estevam, tglx; +Cc: Frank.Li, imx, linux-kernel, Fabio Estevam
On Wed, 2026-08-05 at 16:27 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
>
> imx_irqsteer_probe() enables runtime PM, but imx_irqsteer_remove() does
> not disable it. Consequently, runtime PM remains enabled after unbinding
> the device, and rebinding it triggers:
>
> Unbalanced pm_runtime_enable!
>
> Use devm_pm_runtime_enable() to automatically disable runtime PM when
> the device is removed. Set up runtime PM before creating the IRQ domain
> and registering chained handlers so that a failure cannot leave either
> resource pointing at freed driver data.
>
> Fixes: 4730d2233311 ("irqchip/imx-irqsteer: Add runtime PM support")
> Signed-off-by: Fabio Estevam <festevam@nabladev.com>
> ---
> Changes since v1:
> - Move devm_pm_runtime_enable() prior to irq_domain_create_linear(). (Frank)
>
> drivers/irqchip/irq-imx-irqsteer.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be3..653e25115083 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -236,6 +236,11 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> if (irqsteer_has_chanctrl(data->devtype_data))
> writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>
> + pm_runtime_set_active(&pdev->dev);
> + ret = devm_pm_runtime_enable(&pdev->dev);
> + if (ret)
> + goto out;
> +
> data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
> &imx_irqsteer_domain_ops, data);
> if (!data->domain) {
> @@ -262,9 +267,6 @@ 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);
> -
> return 0;
> out:
> clk_disable_unprepare(data->ipg_clk);
I still believe there is something off with the way pm_runtime is
handled, and that the double clock disable is possible.
Since (like I said) I have very limited understanding of the runtime_pm
framework, I decided to make a little experiment.
With the dummy module below, I see this:
[ 548.253591] pm_dummy pm_dummy: pm_dummy_probe() executed
[ 548.254209] pm_dummy pm_dummy: clock enabled; refcount: 1
[ 548.255028] pm_dummy pm_dummy: pm_dummy_runtime_suspend() triggered
[ 548.255845] pm_dummy pm_dummy: clock disabled; refcount: 0
[ 554.261820] pm_dummy pm_dummy: pm_dummy_runtime_resume() triggered
[ 554.263100] pm_dummy pm_dummy: clock enabled; refcount: 1
[ 554.264006] pm_dummy pm_dummy: pm_dummy_runtime_suspend() triggered
[ 554.264997] pm_dummy pm_dummy: clock disabled; refcount: 0
[ 554.265819] pm_dummy pm_dummy: pm_dummy_remove() executed
[ 554.266609] pm_dummy pm_dummy: clock disabled; refcount: -1
[ 554.267468] pm_dummy pm_dummy: **************************************************
[ 554.268554] pm_dummy pm_dummy: [BUG DETECTED] Clock disable count underflow! (-1)
[ 554.269479] pm_dummy pm_dummy: **************************************************
What I find interesting is that the device is suspended immediately
during probe(), then it's automatically resumed and immediately
suspended again right before remove(). The latter is probably a side
effect of devm_pm_runtime_enable(). But in any case, the clock *is*
disabled twice, and that's even without any explicit suspend or resume,
it's just by loading and unloading the module.
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Radu Rendec <radu@rendec.net>");
MODULE_DESCRIPTION("runtime_pm playground");
static int mock_clk_count = 0;
static int mock_clk_prepare_enable(struct device *dev)
{
mock_clk_count++;
dev_info(dev, "clock enabled; refcount: %d\n", mock_clk_count);
return 0;
}
static void mock_clk_disable_unprepare(struct device *dev)
{
mock_clk_count--;
dev_info(dev, "clock disabled; refcount: %d\n", mock_clk_count);
if (mock_clk_count < 0) {
dev_err(dev, "**************************************************\n");
dev_err(dev, "[BUG DETECTED] Clock disable count underflow! (%d)\n", mock_clk_count);
dev_err(dev, "**************************************************\n");
}
}
static int pm_dummy_runtime_suspend(struct device *dev)
{
dev_info(dev, "%s() triggered\n", __func__);
mock_clk_disable_unprepare(dev);
return 0;
}
static int pm_dummy_runtime_resume(struct device *dev)
{
dev_info(dev, "%s() triggered\n", __func__);
return mock_clk_prepare_enable(dev);
}
static const struct dev_pm_ops pm_dummy_pm_ops = {
SET_RUNTIME_PM_OPS(pm_dummy_runtime_suspend, pm_dummy_runtime_resume, NULL)
};
static int pm_dummy_probe(struct platform_device *pdev)
{
dev_info(&pdev->dev, "%s() executed\n", __func__);
mock_clk_prepare_enable(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
devm_pm_runtime_enable(&pdev->dev);
return 0;
}
static void pm_dummy_remove(struct platform_device *pdev)
{
dev_info(&pdev->dev, "%s() executed\n", __func__);
mock_clk_disable_unprepare(&pdev->dev);
}
static struct platform_driver pm_dummy = {
.probe = pm_dummy_probe,
.remove = pm_dummy_remove,
.driver = {
.name = "pm_dummy",
.pm = &pm_dummy_pm_ops,
},
};
static struct platform_device *pdev;
static int __init pm_demo_init(void)
{
int ret;
ret = platform_driver_register(&pm_dummy);
if (ret)
return ret;
pdev = platform_device_register_simple("pm_dummy", -1, NULL, 0);
if (IS_ERR(pdev)) {
platform_driver_unregister(&pm_dummy);
return PTR_ERR(pdev);
}
return 0;
}
static void __exit pm_demo_exit(void)
{
platform_device_unregister(pdev);
platform_driver_unregister(&pm_dummy);
}
module_init(pm_demo_init);
module_exit(pm_demo_exit);
^ permalink raw reply [flat|nested] 5+ messages in thread