of_irq_parse_one() does a get() on the device node returned in out_irq->np.
Callers of of_irq_parse_one() must do a put() when they are done with it.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
.../mach-shmobile/regulator-quirk-rcar-gen2.c | 1 +
arch/powerpc/sysdev/mpic_msi.c | 1 +
drivers/clocksource/timer-clint.c | 1 +
drivers/irqchip/irq-mchp-eic.c | 1 +
drivers/irqchip/irq-owl-sirq.c | 1 +
drivers/irqchip/irq-renesas-rzg2l.c | 1 +
drivers/irqchip/irq-sifive-plic.c | 1 +
drivers/irqchip/irq-sun6i-r.c | 2 ++
drivers/of/irq.c | 22 ++++++++++++++-----
drivers/of/unittest.c | 7 ++++++
drivers/pci/of.c | 6 ++++-
drivers/soc/ti/knav_qmss_queue.c | 3 +++
drivers/usb/host/xhci-tegra.c | 1 +
13 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 117e7b07995b9..af1c04f0705aa 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
kfree(quirk);
continue;
}
+ of_node_put(argsa->np);
list_for_each_entry(pos, &quirk_list, list) {
argsb = &pos->irq_args;
diff --git a/arch/powerpc/sysdev/mpic_msi.c b/arch/powerpc/sysdev/mpic_msi.c
index 34246c8e01c2b..bac8b12291a87 100644
--- a/arch/powerpc/sysdev/mpic_msi.c
+++ b/arch/powerpc/sysdev/mpic_msi.c
@@ -63,6 +63,7 @@ static int __init mpic_msi_reserve_u3_hwirqs(struct mpic *mpic)
ops->xlate(mpic->irqhost, NULL, oirq.args,
oirq.args_count, &hwirq, &flags);
msi_bitmap_reserve_hwirq(&mpic->msi_bitmap, hwirq);
+ of_node_put(oirq.np);
}
}
diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 6cfe2ab73eb0c..126912674610c 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -175,6 +175,7 @@ static int __init clint_timer_init_dt(struct device_node *np)
oirq.args[0] == RV_IRQ_TIMER &&
irq_find_host(oirq.np))
clint_timer_irq = irq_of_parse_and_map(np, i);
+ of_node_put(oirq.np);
}
/* If CLINT timer irq not found then fail */
diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
index c726a19837d29..f020d28efc6a4 100644
--- a/drivers/irqchip/irq-mchp-eic.c
+++ b/drivers/irqchip/irq-mchp-eic.c
@@ -239,6 +239,7 @@ static int mchp_eic_init(struct device_node *node, struct device_node *parent)
ret = of_irq_parse_one(node, i, &irq);
if (ret)
goto clk_unprepare;
+ of_node_put(irq.np);
if (WARN_ON(irq.args_count != 3)) {
ret = -EINVAL;
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
index 6e4127465094f..1ac31acb8e792 100644
--- a/drivers/irqchip/irq-owl-sirq.c
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -311,6 +311,7 @@ static int __init owl_sirq_init(const struct owl_sirq_params *params,
pr_err("%pOF: failed to parse interrupt %d\n", node, i);
goto out_unmap;
}
+ of_node_put(irq.np);
if (WARN_ON(irq.args_count != 3)) {
ret = -EINVAL;
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 25fd8ee66565b..095d2263e2dac 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -310,6 +310,7 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
return ret;
of_phandle_args_to_fwspec(np, map.args, map.args_count,
&priv->fwspec[i]);
+ of_node_put(map.np);
}
return 0;
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index ff47bd0dec455..1a863ab2abd60 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -420,6 +420,7 @@ static int __init __plic_init(struct device_node *node,
irq_set_chained_handler(plic_parent_irq,
plic_handle_irq);
}
+ of_node_put(parent.np);
/*
* When running in M-mode we need to ignore the S-mode handler.
diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
index a01e440494154..29e5917d1fc9d 100644
--- a/drivers/irqchip/irq-sun6i-r.c
+++ b/drivers/irqchip/irq-sun6i-r.c
@@ -317,6 +317,8 @@ static int __init sun6i_r_intc_init(struct device_node *node,
ret = of_irq_parse_one(node, 0, &nmi_parent);
if (ret)
return ret;
+ of_node_put(nmi_parent.np);
+
if (nmi_parent.args_count < 3 ||
nmi_parent.args[0] != GIC_SPI ||
nmi_parent.args[2] != IRQ_TYPE_LEVEL_HIGH)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e9bf5236ed892..95da943fcf075 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -35,12 +35,16 @@
*/
unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
{
+ unsigned int res;
struct of_phandle_args oirq;
if (of_irq_parse_one(dev, index, &oirq))
return 0;
- return irq_create_of_mapping(&oirq);
+ res = irq_create_of_mapping(&oirq);
+ of_node_put(oirq.np);
+
+ return res;
}
EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
@@ -438,10 +442,16 @@ int of_irq_get(struct device_node *dev, int index)
return rc;
domain = irq_find_host(oirq.np);
- if (!domain)
- return -EPROBE_DEFER;
+ if (!domain) {
+ rc = -EPROBE_DEFER;
+ goto out;
+ }
- return irq_create_of_mapping(&oirq);
+ rc = irq_create_of_mapping(&oirq);
+
+out:
+ of_node_put(oirq.np);
+ return rc;
}
EXPORT_SYMBOL_GPL(of_irq_get);
@@ -478,8 +488,10 @@ int of_irq_count(struct device_node *dev)
struct of_phandle_args irq;
int nr = 0;
- while (of_irq_parse_one(dev, nr, &irq) == 0)
+ while (of_irq_parse_one(dev, nr, &irq) == 0) {
nr++;
+ of_node_put(irq.np);
+ }
return nr;
}
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index bc0f1e50a4bee..801a42c20fc79 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1021,6 +1021,8 @@ static void __init of_unittest_parse_interrupts(void)
memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
+ if (!rc)
+ of_node_put(args.np);
passed &= !rc;
passed &= (args.args_count == 1);
@@ -1028,6 +1030,7 @@ static void __init of_unittest_parse_interrupts(void)
unittest(passed, "index %i - data error on node %pOF rc=%i\n",
i, args.np, rc);
+
}
of_node_put(np);
@@ -1042,6 +1045,8 @@ static void __init of_unittest_parse_interrupts(void)
memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
+ if (!rc)
+ of_node_put(args.np);
/* Test the values from tests-phandle.dtsi */
switch (i) {
@@ -1098,6 +1103,8 @@ static void __init of_unittest_parse_interrupts_extended(void)
memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
+ if (!rc)
+ of_node_put(args.np);
/* Test the values from tests-phandle.dtsi */
switch (i) {
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 196834ed44fe8..a3a047f57e439 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -535,7 +535,11 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
if (ret)
return 0; /* Proper return code 0 == NO_IRQ */
- return irq_create_of_mapping(&oirq);
+ ret = irq_create_of_mapping(&oirq);
+
+ of_node_put(oirq.np);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(of_irq_parse_and_map_pci);
#endif /* CONFIG_OF_IRQ */
diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 8fb76908be704..0ac4a0f219127 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -1240,6 +1240,9 @@ static int knav_setup_queue_range(struct knav_device *kdev,
break;
range->irqs[i].irq = irq_create_of_mapping(&oirq);
+
+ of_node_put(oirq.np);
+
if (range->irqs[i].irq == IRQ_NONE)
break;
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index bdb776553826b..538eedac1c71e 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -1458,6 +1458,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
/* Older device-trees don't have padctrl interrupt */
err = of_irq_parse_one(np, 0, &args);
if (!err) {
+ of_node_put(args.np);
tegra->padctl_irq = of_irq_get(np, 0);
if (tegra->padctl_irq <= 0) {
err = (tegra->padctl_irq == 0) ? -ENODEV : tegra->padctl_irq;
--
2.25.1
Hi Jean-Jacques,
Thanks for your patch!
On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
<[email protected]> wrote:
> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
> Callers of of_irq_parse_one() must do a put() when they are done with it.
What does "be done with it" really mean here?
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
> kfree(quirk);
> continue;
> }
> + of_node_put(argsa->np);
The quirk object, which is a container of argsa, is still used below,
and stored in a linked list. I agree argsa->np is not dereferenced,
but the pointer itself is still compared to other pointers.
IIUIC, calling of_node_put() might cause the reference count to drop to
zero, and the underlying struct node object to be deallocated.
So when a future reference to the same DT node will be taken, a new
struct node object will be allocated, and the pointer comparison below
will fail?
Or am I missing something?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 02/03/2023 08:49, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
>
> Thanks for your patch!
>
> On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
> <[email protected]> wrote:
>> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
>> Callers of of_irq_parse_one() must do a put() when they are done with it.
>
> What does "be done with it" really mean here?
>
>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
>> kfree(quirk);
>> continue;
>> }
>> + of_node_put(argsa->np);
>
> The quirk object, which is a container of argsa, is still used below,
> and stored in a linked list. I agree argsa->np is not dereferenced,
> but the pointer itself is still compared to other pointers.
Hi Geert,
I fail to see when the pointers are compared. It looks to me that only
the args are compared. Am I missing something ?
In any case, looking more closely at the code, I guess that indeed the
of_node_put() shouldn't be added here because this code expects that the
nodes never go away. That is probably a good assertion in case of PMICs
JJ
> IIUIC, calling of_node_put() might cause the reference count to drop to
> zero, and the underlying struct node object to be deallocated.
> So when a future reference to the same DT node will be taken, a new
> struct node object will be allocated, and the pointer comparison below
> will fail?
>
> Or am I missing something?
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Jean-Jacques,
On Sat, Mar 4, 2023 at 11:34 AM Jean-Jacques Hiblot
<[email protected]> wrote:
> On 02/03/2023 08:49, Geert Uytterhoeven wrote:
> > On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
> > <[email protected]> wrote:
> >> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
> >> Callers of of_irq_parse_one() must do a put() when they are done with it.
> >
> > What does "be done with it" really mean here?
> >
> >> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> >
> >> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> >> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> >> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
> >> kfree(quirk);
> >> continue;
> >> }
> >> + of_node_put(argsa->np);
> >
> > The quirk object, which is a container of argsa, is still used below,
> > and stored in a linked list. I agree argsa->np is not dereferenced,
> > but the pointer itself is still compared to other pointers.
>
> I fail to see when the pointers are compared. It looks to me that only
> the args are compared. Am I missing something ?
You're right, in upstream, there is no such check.
In my local tree, I have converted the comparisons below to use a new
helper of_phandle_args_eq() (which does compare the np member, too),
but that change never went upstream, as the other user of that helper
was rejected.
> In any case, looking more closely at the code, I guess that indeed the
> of_node_put() shouldn't be added here because this code expects that the
> nodes never go away. That is probably a good assertion in case of PMICs
OK.
> > IIUIC, calling of_node_put() might cause the reference count to drop to
> > zero, and the underlying struct node object to be deallocated.
> > So when a future reference to the same DT node will be taken, a new
> > struct node object will be allocated, and the pointer comparison below
> > will fail?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds