Re: [PATCH v2 4/4] irqchip/gic-its: Rework platform MSI deviceID detection

On Tue, Oct 14, 2025 at 06:12:11PM +0100, Marc Zyngier wrote:

On Tue, 14 Oct 2025 10:58:45 +0100,
Lorenzo Pieralisi [off-list ref] wrote:

quoted

Current code retrieving platform devices MSI devID in the GIC ITS MSI
parent helpers suffers from some minor issues:

- It leaks a struct device_node reference
- It triggers an excessive WARN_ON on wrong of_phandle_args count detection

Well, if your DT is that rotten, maybe you actually deserve some
console spamming, don't you think?

Yes from that standpoint it would make sense to leave the WARN_ON there,
I can add it back.

quoted

- It is duplicated between GICv3 and GICv5 for no good reason
- It does not use the OF phandle iterator code that simplifies
  the msi-parent property parsing

Implement a helper function that addresses the full set of issues in one go
by consolidating GIC v3 and v5 code and converting the msi-parent parsing
loop to the more modern OF phandle iterator API, fixing the
struct device_node reference leak in the process.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sascha Bischoff <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-its-msi-parent.c | 98 ++++++++----------------
 1 file changed, 33 insertions(+), 65 deletions(-)

diff --git a/drivers/irqchip/irq-gic-its-msi-parent.c b/drivers/irqchip/irq-gic-its-msi-parent.c
index eb1473f1448a..a65f762b7dd4 100644
--- a/drivers/irqchip/irq-gic-its-msi-parent.c
+++ b/drivers/irqchip/irq-gic-its-msi-parent.c

@@ -142,83 +142,51 @@ static int its_v5_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 #define its_v5_pci_msi_prepare	NULL
 #endif /* !CONFIG_PCI_MSI */
 
-static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
-				  u32 *dev_id)
+static int __of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev, u32 *dev_id,
+				phys_addr_t *pa, bool is_v5)
 {
-	int ret, index = 0;
+	struct of_phandle_iterator it;
+	uint32_t args;

Use u32, this is not userspace-visible (the OF code will cope). And
move it to where it matters instead of having such a wide scope.

Ok.

quoted

+	int ret;
 
 	/* Suck the DeviceID out of the msi-parent property */
-	do {
-		struct of_phandle_args args;
+	of_for_each_phandle(&it, ret, dev->of_node, "msi-parent", "#msi-cells", -1) {
+		/* GICv5 ITS domain matches the MSI controller node parent */
+		struct device_node *np __free(device_node) = is_v5 ? of_get_parent(it.node)
+							     : of_node_get(it.node);
 
-		ret = of_parse_phandle_with_args(dev->of_node,
-						 "msi-parent", "#msi-cells",
-						 index, &args);
-		if (args.np == irq_domain_get_of_node(domain)) {
-			if (WARN_ON(args.args_count != 1))
-				return -EINVAL;
-			*dev_id = args.args[0];
-			break;
+		if (np == irq_domain_get_of_node(domain)) {
+			if (of_phandle_iterator_args(&it, &args, 1) != 1) {
+				dev_warn(dev, "Bogus msi-parent property\n");
+				ret = -EINVAL;
+			}
+
+			if (!ret && is_v5)
+				ret = its_translate_frame_address(it.node, pa);

Why do you need this is_v5 hack, since the only case were you pass a
pointer to get the translate register address is for v5?

Yep, I thought about this what you are suggesting makes sense - is_v5 is
useless (and terrible).

quoted

+
+			if (!ret)
+				*dev_id = args;
+
+			of_node_put(it.node);
+			return ret;
 		}
-		index++;
-	} while (!ret);
-
-	if (ret) {
-		struct device_node *np = NULL;
-
-		ret = of_map_id(dev->of_node, dev->id, "msi-map", "msi-map-mask", &np, dev_id);
-		if (np)
-			of_node_put(np);
 	}
 
-	return ret;
+	struct device_node *msi_ctrl __free(device_node) = NULL;
+
+	return of_map_id(dev->of_node, dev->id, "msi-map", "msi-map-mask", &msi_ctrl, dev_id);
+}
+
+static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
+			      u32 *dev_id)
+{
+	return __of_pmsi_get_dev_id(domain, dev, dev_id, NULL, false);
 }

At this stage, we really don't need these on-liners, as they only
obfuscate the logic. Just use the main helper directly. Something like
the hack below.

That makes sense.

Thanks !
Lorenzo