RE: [PATCH v3 1/4] irqchip/imx-irqsteer: Fix error handling path in probe()

[Zhipeng Wang (OSS)]

> > From: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> >
> > If the fsl,num-irqs sanity check rejects the value after the IRQ
> > domain has already been created, probe() jumps to a single label that
> > only calls clk_disable_unprepare(), leaving the freshly created IRQ domain
> leaked.
> > The domain-creation failure path shares the same label, which is
> > correct only because the domain is NULL there.
> >
> > Split the error path so that a failure after the domain has been
> > created removes it before disabling the clock, and a failure before
> > that goes straight to the clock cleanup.
> >
> > Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output
> > interrupts support")
> > Signed-off-by: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> > ---
> > Changes in v3:
> >  - New patch, split out of the single v2 patch. Fixes the irq_domain
> >    leak on the probe() error path reported by Sashiko AI on v2.
> >
> >  drivers/irqchip/irq-imx-irqsteer.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
>
> I suggest create helper devm_irq_domain_create_leaner()
>
> static inline struct irq_domain *
> devm_irq_domain_create_linear(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(&info);
>
>       return IS_ERR(d) ? NULL : d;
> }
>
> Then imx-irqsteer this devm version. So other drivers can get beneafit also
>
> Frank
>
>
Thanks, good idea. In v4 I added devm_irq_domain_create_linear() as the
devres sibling of irq_domain_create_linear() (patch 1/5), and switched
imx-irqsteer over to it (patch 2/5). With the domain owned by devres the
leak on the probe() error path is fixed structurally, so the hand-rolled
err_domain label is gone and remove() no longer needs irq_domain_remove().
The helper lives in include/linux/irqdomain.h so other drivers can use it
too.

BRs,
Zhipeng
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 87b07f517be3..a2f0629b22a3 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -241,13 +241,13 @@ static int imx_irqsteer_probe(struct
> platform_device *pdev)
> >     if (!data->domain) {
> >             dev_err(&pdev->dev, "failed to create IRQ domain\n");
> >             ret = -ENOMEM;
> > -           goto out;
> > +           goto err_clk;
> >     }
> >     irq_domain_set_pm_device(data->domain, &pdev->dev);
> >
> >     if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> >             ret = -EINVAL;
> > -           goto out;
> > +           goto err_domain;
> >     }
> >
> >     for (i = 0; i < data->irq_count; i++) { @@ -266,7 +266,10 @@ static
> > int imx_irqsteer_probe(struct platform_device *pdev)
> >     pm_runtime_enable(&pdev->dev);
> >
> >     return 0;
> > -out:
> > +
> > +err_domain:
> > +   irq_domain_remove(data->domain);
> > +err_clk:
> >     clk_disable_unprepare(data->ipg_clk);
> >     return ret;
> >  }
> > --
> > 2.34.1
> >
> >

NXP Confidential