Re: [PATCH v2 3/8] irqchip/al-fic: keep the device_node instead of a cached name string

[Radu Rendec <radu@rendec.net>]

On Mon, 2026-10-05 at 11:17 +0000, Farber, Eliav wrote:
> On Sun, 2026-10-04 at 17:40 +0000, Radu Rendec wrote:
> > On Sun, 2026-09-27 at 08:06 +0000, Eliav Farber wrote:
> > > struct al_fic cached a "const char *name" that al_fic_wire_init() received
> > > as a separate argument and set from node->name. That string was never owned
> > > by the driver: it aliased storage inside the device_node and stayed valid
> > > only as long as the node did, yet nothing in the struct held the node to
> > > express that dependency. Keep the device_node in the struct instead: it
> > > holds the owning object rather than a bare pointer into it, lets each site
> > > derive the name on demand, and gives the driver the node it needs in the
> > > next change, which requests the parent interrupt by the node's full_name.
> > 
> > That makes sense. But keeping a pointer to the whole structure instead
> > of aliasing a pointer inside the structure makes no additional
> > guarantees w.r.t. the lifetime of that structure, it just makes the
> > intention more obvious.
> > 
> > What prevents the "struct device_node" from going away after the init
> > function returns? It cannot go away before it returns because the init
> > function is called as desc->irq_init_cb() from of_irq_init(), while
> > holding a reference to the node. I *think* the assumption that it can
> > never go away (even after the init function returns) is correct because
> > the code in of_irq_init() seems to deliberately "leak" a reference to
> > the node. But this is not documented anywhere. So perhaps it's worth
> > calling it out at least in the commit message.
> > 
> > Rob, you're a maintainer for drivers/of/irq.c and it looks like you
> > merged most (or all?) of the recent patches to it. Perhaps you can help
> > us and explain how this is supposed to work?
> 
> You read it right. of_irq_init() takes a reference with of_node_get()
> before calling the init callback, and on a successful init that
> reference is never put - the of_intc_desc is freed but desc->dev is left
> pinned. On a failed init it is put. So the node is kept alive for the
> life of the system once probe succeeds, same as you described.
> 
> I've added that to the commit message, and made clear this patch is not
> closing a lifetime bug - node->name was never actually at risk of
> dangling, since the storage it points into is pinned the same way. The
> value of keeping the device_node is making that dependency explicit
> rather than fixing something broken.

Thanks! I think it's very clearly explained now.

> I'll leave the question of whether this ought to be documented in
> of_irq_init() itself to Rob.

Of course. After giving it more thought, I'm quite confident it's the
intended behavior. There were some patches to of_irq_init() that fixed
some reference leaks on the error paths. If keeping the reference on
the successful path hadn't been the intended behavior, it would've been
called out when those patches were reviewed.

> > > irq_alloc_domain_generic_chips() keeps the pointer it is given, so it now
> > > uses node->full_name.
> > 
> > nit: should this use of_node_full_name() instead? Not that fic->node
> > can be null, but if there is an accessor, why not use it?
> 
> Done, here and in patch 4's request_irq() call.
> 
> Both changes are in v3, which I will post shortly.
> 
> Thanks,
> Eliav