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

[Radu Rendec]

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?

> The irqchip callback that has no device_node in scope now prints the
> instance with %pOF, which formats the node on demand, and the name argument
> threaded through al_fic_wire_init() goes away.
> 
> irq_alloc_domain_generic_chips() keeps the pointer it is given, so it now
> uses node->full_name. This changes the generic chip name from the bare
> node name (e.g. "interrupt-controller") to the full node name including
> its unit address (e.g. "interrupt-controller@fd8a8500"), which keeps
> instances that share a bare name distinguishable.
> 
> Signed-off-by: Eliav Farber <farbere@xxxxxxxxxx>
> ---
> v2: new patch. Keep the device_node in struct al_fic instead of a cached
>     name string that aliased node storage. Introduced here so the struct
>     holds the node before the next patch requests the parent interrupt by
>     node->full_name, keeping every commit buildable on its own.
> 
>  drivers/irqchip/irq-al-fic.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index 760bd08dcff4..c7cc2631caf8 100644
> --- a/drivers/irqchip/irq-al-fic.c
> +++ b/drivers/irqchip/irq-al-fic.c
> @@ -36,7 +36,7 @@ enum al_fic_state {
>  struct al_fic {
>  	void __iomem *base;
>  	struct irq_domain *domain;
> -	const char *name;
> +	struct device_node *node;
>  	unsigned int parent_irq;
>  	enum al_fic_state state;
>  };
> @@ -89,7 +89,7 @@ static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
>  	if (fic->state == AL_FIC_UNCONFIGURED) {
>  		al_fic_set_trigger(fic, gc, new_state);
>  	} else if (fic->state != new_state) {
> -		pr_debug("fic %s state already configured to %d\n", fic->name, fic->state);
> +		pr_debug("fic %pOF state already configured to %d\n", fic->node, fic->state);
>  		return -EINVAL;
>  	}
>  	return 0;
> @@ -142,7 +142,7 @@ static int al_fic_register(struct device_node *node,
>  
>  	ret = irq_alloc_domain_generic_chips(fic->domain,
>  					     NR_FIC_IRQS,
> -					     1, fic->name,
> +					     1, fic->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?

>  					     handle_level_irq,
>  					     0, 0, IRQ_GC_INIT_MASK_CACHE);
>  	if (ret) {
> @@ -175,9 +175,8 @@ static int al_fic_register(struct device_node *node,
>  
>  /*
>   * al_fic_wire_init() - initialize and configure fic in wire mode
> - * @of_node: optional pointer to interrupt controller's device tree node.
> + * @node: pointer to the interrupt controller's device tree node
>   * @base: mmio to fic register
> - * @name: name of the fic
>   * @parent_irq: interrupt of parent
>   *
>   * This API will configure the fic hardware to work in wire mode.
> @@ -187,7 +186,6 @@ static int al_fic_register(struct device_node *node,
>   */
>  static struct al_fic *al_fic_wire_init(struct device_node *node,
>  				       void __iomem *base,
> -				       const char *name,
>  				       unsigned int parent_irq)
>  {
>  	struct al_fic *fic;
> @@ -200,7 +198,7 @@ static struct al_fic *al_fic_wire_init(struct device_node *node,
>  
>  	fic->base = base;
>  	fic->parent_irq = parent_irq;
> -	fic->name = name;
> +	fic->node = node;
>  
>  	/* mask out all interrupts */
>  	writel_relaxed(0xFFFFFFFF, fic->base + AL_FIC_MASK);
> @@ -254,7 +252,6 @@ static int __init al_fic_init_dt(struct device_node *node,
>  
>  	fic = al_fic_wire_init(node,
>  			       base,
> -			       node->name,
>  			       parent_irq);
>  	if (IS_ERR(fic)) {
>  		pr_err("%pOF: fail to initialize irqchip (%lu)\n",