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

[Radu Rendec <radu@rendec.net>]

On Thu, 2026-10-08 at 09:00 +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 aliased
> storage inside the device_node rather than being owned by the driver, but
> nothing in the struct expressed that dependency - al_fic just held a bare
> pointer with no indication of what it pointed into or why it stayed valid.
> 
> This is not fixing a lifetime bug - of_irq_init() takes a reference on
> the node before calling the driver's init callback and never drops it on
> a successful init, so the node is pinned for the life of the system
> either way, and node->name was never actually at risk of dangling.

It looks like this was *not* the intended behavior of of_irq_init()
after all; it was a bug and it was fixed in commit 30724547b221
("of/irq: Fix remaining refcount leaks in of_irq_init()") [1].

When we discussed it, both of us missed this. The patch was queued in
Rob's tree but not merged into mainline yet. I even tried to "fix" the
documentation [2], and my patch was flagged immediately by sashiko
(because the other patch had just made it into mainline).

Anyway, you'll have to address this in v5 because it's clearly not
going to work with that patch merged. I guess just take the refcount
explicitly in al_fic_wire_init() where you store the pointer.

[1] https://lore.kernel.org/all/20260917124247.2151674-1-vulab@iscas.ac.cn/
[2] https://lore.kernel.org/all/20261009011324.1503697-1-radu@rendec.net/

> Keeping the device_node in the struct instead of the bare name is about
> making the dependency explicit rather than closing a real one: it holds
> the object the name is derived from, and lets each site derive the name
> on demand instead of carrying a pointer whose validity nothing in the
> struct asserts.
> 
> 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 of_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@amazon.com>
> Reviewed-by: Radu Rendec <radu@rendec.net>
> ---
> v4: no change.
> 
> v3:
>  - Use of_node_full_name() instead of reaching into node->full_name
>    directly, as Radu Rendec suggested.
>  - Rewrite the commit message to say plainly that this patch does not fix
>    a lifetime bug. of_irq_init() takes a reference on the node before
>    calling the driver's init callback and does not drop it on a
>    successful init, so the node, and the storage node->name points into,
>    is pinned for the life of the system either way. The value of keeping
>    the device_node is making that dependency explicit, not closing a
>    real one.
> 
> 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..ee06d0123b7a 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, of_node_full_name(fic->node),
>  					     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",