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

[Eliav Farber]

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.

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@xxxxxxxxxx>
Reviewed-by: Radu Rendec <radu@xxxxxxxxxx>
---
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",
-- 
2.47.3