Re: [PATCH 2/3] of: irq: make callers of of_irq_parse_one() release the device node

Hi Jean-Jacques,

On Sat, Mar 4, 2023 at 11:34 AM Jean-Jacques Hiblot
[off-list ref] wrote:

On 02/03/2023 08:49, Geert Uytterhoeven wrote:

quoted

On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
[off-list ref] wrote:

quoted

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?

quoted

Signed-off-by: Jean-Jacques Hiblot <redacted>

quoted

--- 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.

quoted

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 -- geert@linux-m68k.org

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