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

[Marc Zyngier <maz@kernel.org>]

On Tue, 14 Oct 2025 10:58:45 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> 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?

> - 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 <sascha.bischoff@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> 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.

> +	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?

> +
> +			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.

	M.

diff --git a/drivers/irqchip/irq-gic-its-msi-parent.c b/drivers/irqchip/irq-gic-its-msi-parent.c
index a65f762b7dd4d..7c82fd152655e 100644
--- a/drivers/irqchip/irq-gic-its-msi-parent.c
+++ b/drivers/irqchip/irq-gic-its-msi-parent.c
@@ -142,26 +142,27 @@ 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,
-				phys_addr_t *pa, bool is_v5)
+static int of_pmsi_get_msi_info(struct irq_domain *domain, struct device *dev, u32 *dev_id,
+				phys_addr_t *pa)
 {
 	struct of_phandle_iterator it;
-	uint32_t args;
 	int ret;
 
 	/* Suck the DeviceID out of the msi-parent property */
 	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)
+		struct device_node *np __free(device_node) = pa ? of_get_parent(it.node)
 							     : of_node_get(it.node);
 
 		if (np == irq_domain_get_of_node(domain)) {
+			u32 args;
+
 			if (of_phandle_iterator_args(&it, &args, 1) != 1) {
 				dev_warn(dev, "Bogus msi-parent property\n");
 				ret = -EINVAL;
 			}
 
-			if (!ret && is_v5)
+			if (!ret && pa)
 				ret = its_translate_frame_address(it.node, pa);
 
 			if (!ret)
@@ -177,18 +178,6 @@ static int __of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev, u
 	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);
-}
-
-static int of_v5_pmsi_get_msi_info(struct irq_domain *domain, struct device *dev,
-				   u32 *dev_id, phys_addr_t *pa)
-{
-	return __of_pmsi_get_dev_id(domain, dev, dev_id, pa, true);
-}
-
 int __weak iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 {
 	return -1;
@@ -202,7 +191,7 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
 	int ret;
 
 	if (dev->of_node)
-		ret = of_pmsi_get_dev_id(domain->parent, dev, &dev_id);
+		ret = of_pmsi_get_msi_info(domain->parent, dev, &dev_id, NULL);
 	else
 		ret = iort_pmsi_get_dev_id(dev, &dev_id);
 	if (ret)
@@ -230,7 +219,7 @@ static int its_v5_pmsi_prepare(struct irq_domain *domain, struct device *dev,
 	if (!dev->of_node)
 		return -ENODEV;
 
-	ret = of_v5_pmsi_get_msi_info(domain->parent, dev, &dev_id, &pa);
+	ret = of_pmsi_get_msi_info(domain->parent, dev, &dev_id, &pa);
 	if (ret)
 		return ret;
 

-- 
Jazz isn't dead. It just smells funny.