Re: [PATCH v5 4/9] irqchip/al-fic: keep the device_node instead of a cached name string

[Radu Rendec]

On Sun, 2026-10-11 at 05:04 +0000, Eliav Farber wrote:
> struct al_fic cached a const char *name that al_fic_wire_init() set
> from node->name. That string pointed into the device_node, so the
> driver already depended on the node outliving init, but nothing in the
> struct said so.
> 
> Keep the device_node itself and derive the name where it is needed. The
> dependency does not go away: of_node_full_name() also returns a pointer
> into the node, and irq_alloc_domain_generic_chips() keeps the string it
> is given.
> 
> The driver therefore needs its own reference. of_irq_init() holds one
> across the init callback, but since commit 30724547b221 ("of/irq: Fix
> remaining refcount leaks in of_irq_init()") it drops that reference
> again when it drains its parent list. Take a reference where the
> pointer is stored and drop it on the error path. The driver is
> __init-only with no teardown, so on success it is held for the life of
> the system.
> 
> Deriving the name this way also changes the generic chip name from the
> bare node name (e.g. "interrupt-controller") to the full name including
> the unit address (e.g. "interrupt-controller@fd8a8500"), which keeps
> instances that share a bare name distinguishable.
> 
> Signed-off-by: Eliav Farber <farbere@xxxxxxxxxx>
> ---
> v5:
>  - Take a reference on the device_node where the driver stores it, and
>    drop it on the error path. of_irq_init() no longer holds one past the
>    init callback: commit 30724547b221 ("of/irq: Fix remaining refcount
>    leaks in of_irq_init()") drops it when the parent list is drained.
>  - Rewrite the commit message accordingly. It previously argued that
>    of_irq_init() never drops the reference on a successful init, which is
>    no longer true, and that this patch therefore fixed no lifetime bug.
> 
> 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 | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index 1a7f851731b2..32d8098699a4 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 = of_node_get(node);
>  
>  	/* mask out all interrupts */
>  	writel_relaxed(0xFFFFFFFF, fic->base + AL_FIC_MASK);
> @@ -222,6 +220,7 @@ static struct al_fic *al_fic_wire_init(struct device_node *node,
>  	return fic;
>  
>  err_free:
> +	of_node_put(fic->node);
>  	kfree(fic);
>  	return ERR_PTR(ret);
>  }
> @@ -254,7 +253,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 (%pe)\n",

Reviewed-by: Radu Rendec <radu@xxxxxxxxxx>