[PATCH v2 0/4] of/irq: Misc msi-parent handling fixes/clean-ups

[PATCH v2 0/4] of/irq: Misc msi-parent handling fixes/clean-ups

[Lorenzo Pieralisi]

This is a follow up to [1] - with additional patches that are addressing
Rob's feedback (pcie-layerscape-gen4 was removed from the kernel, Yay !)
and other bits and bobs I noticed while staring at the code.

Patch (1) is a fix and technically we would like to get it in v6.18 please.

Patch (3) is compile-tested only, I can not run it on HW, I do not have it,
Scott, Ray please test it if you can.

[1] https://lore.kernel.org/lkml/20250916091858.257868-1-lpieralisi@kernel.org/

Cc: Sascha Bischoff <sascha.bischoff@arm.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>

Lorenzo Pieralisi (4):
  of/irq: Add msi-parent check to of_msi_xlate()
  of/irq: Fix OF node refcount in of_msi_get_domain()
  PCI: iproc: Implement MSI controller node detection with
    of_msi_xlate()
  irqchip/gic-its: Rework platform MSI deviceID detection

 drivers/irqchip/irq-gic-its-msi-parent.c | 98 ++++++++----------------
 drivers/of/irq.c                         | 42 +++++++++-
 drivers/pci/controller/pcie-iproc.c      | 22 ++----
 3 files changed, 76 insertions(+), 86 deletions(-)

-- 
2.50.1

[PATCH v2 1/4] of/irq: Add msi-parent check to of_msi_xlate()

[Lorenzo Pieralisi]

In some legacy platforms the MSI controller for a PCI host bridge is
identified by an msi-parent property whose phandle points at an MSI
controller node with no #msi-cells property, that implicitly means

For such platforms, mapping a device ID and retrieving the MSI controller
node becomes simply a matter of checking whether in the device hierarchy
there is an msi-parent property pointing at an MSI controller node with
such characteristics.

Add a helper function to of_msi_xlate() to check the msi-parent property in
addition to msi-map and retrieve the MSI controller node (with a 1:1 ID
deviceID-IN<->deviceID-OUT  mapping) to provide support for deviceID
mapping and MSI controller node retrieval for such platforms.

Fixes: 57d72196dfc8 ("irqchip/gic-v5: Add GICv5 ITS support")
Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sascha Bischoff <sascha.bischoff@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
---
 drivers/of/irq.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 65c3c23255b7..e67b2041e73b 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -671,6 +671,35 @@ void __init of_irq_init(const struct of_device_id *matches)
 	}
 }
 
+static int of_check_msi_parent(struct device_node *dev_node, struct device_node **msi_node)
+{
+	struct of_phandle_args msi_spec;
+	int ret;
+
+	/*
+	 * An msi-parent phandle with a missing or == 0 #msi-cells
+	 * property identifies a 1:1 ID translation mapping.
+	 *
+	 * Set the msi controller node if the firmware matches this
+	 * condition.
+	 */
+	ret = of_parse_phandle_with_optional_args(dev_node, "msi-parent", "#msi-cells",
+						  0, &msi_spec);
+	if (!ret) {
+		if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
+			ret = -EINVAL;
+
+		if (!ret) {
+			/* Return with a node reference held */
+			*msi_node = msi_spec.np;
+			return 0;
+		}
+		of_node_put(msi_spec.np);
+	}
+
+	return ret;
+}
+
 /**
  * of_msi_xlate - map a MSI ID and find relevant MSI controller node
  * @dev: device for which the mapping is to be done.
@@ -678,7 +707,7 @@ void __init of_irq_init(const struct of_device_id *matches)
  * @id_in: Device ID.
  *
  * Walk up the device hierarchy looking for devices with a "msi-map"
- * property. If found, apply the mapping to @id_in.
+ * or "msi-parent" property. If found, apply the mapping to @id_in.
  * If @msi_np points to a non-NULL device node pointer, only entries targeting
  * that node will be matched; if it points to a NULL value, it will receive the
  * device node of the first matching target phandle, with a reference held.
@@ -692,12 +721,15 @@ u32 of_msi_xlate(struct device *dev, struct device_node **msi_np, u32 id_in)
 
 	/*
 	 * Walk up the device parent links looking for one with a
-	 * "msi-map" property.
+	 * "msi-map" or an "msi-parent" property.
 	 */
-	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
 		if (!of_map_id(parent_dev->of_node, id_in, "msi-map",
 				"msi-map-mask", msi_np, &id_out))
 			break;
+		if (!of_check_msi_parent(parent_dev->of_node, msi_np))
+			break;
+	}
 	return id_out;
 }
 
-- 
2.50.1

Re: [PATCH v2 1/4] of/irq: Add msi-parent check to of_msi_xlate()

[Bjorn Helgaas]

On Tue, Oct 14, 2025 at 11:58:42AM +0200, Lorenzo Pieralisi wrote:
> In some legacy platforms the MSI controller for a PCI host bridge is
> identified by an msi-parent property whose phandle points at an MSI
> controller node with no #msi-cells property, that implicitly means

Looks like you intended to continue the sentence here?

> For such platforms, mapping a device ID and retrieving the MSI controller
> node becomes simply a matter of checking whether in the device hierarchy
> there is an msi-parent property pointing at an MSI controller node with
> such characteristics.
> 
> Add a helper function to of_msi_xlate() to check the msi-parent property in
> addition to msi-map and retrieve the MSI controller node (with a 1:1 ID
> deviceID-IN<->deviceID-OUT  mapping) to provide support for deviceID
> mapping and MSI controller node retrieval for such platforms.

Re: [PATCH v2 1/4] of/irq: Add msi-parent check to of_msi_xlate()

[Lorenzo Pieralisi]

On Tue, Oct 14, 2025 at 05:29:40PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 14, 2025 at 11:58:42AM +0200, Lorenzo Pieralisi wrote:
> > In some legacy platforms the MSI controller for a PCI host bridge is
> > identified by an msi-parent property whose phandle points at an MSI
> > controller node with no #msi-cells property, that implicitly means
> 
> Looks like you intended to continue the sentence here?

Sigh. "that implicitly means #msi-cells == 0", the `#` caused this.

Apologies.

Lorenzo

> > For such platforms, mapping a device ID and retrieving the MSI controller
> > node becomes simply a matter of checking whether in the device hierarchy
> > there is an msi-parent property pointing at an MSI controller node with
> > such characteristics.
> > 
> > Add a helper function to of_msi_xlate() to check the msi-parent property in
> > addition to msi-map and retrieve the MSI controller node (with a 1:1 ID
> > deviceID-IN<->deviceID-OUT  mapping) to provide support for deviceID
> > mapping and MSI controller node retrieval for such platforms.

[PATCH v2 2/4] of/irq: Fix OF node refcount in of_msi_get_domain()

[Lorenzo Pieralisi]

In of_msi_get_domain() if the iterator loop stops early because an
irq_domain match is detected, an of_node_put() on the iterator node is
needed to keep the OF node refcount in sync.

Add it.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Rob Herring <robh@kernel.org>
---
 drivers/of/irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e67b2041e73b..9f6cd5abba76 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -773,8 +773,10 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
 
 	of_for_each_phandle(&it, err, np, "msi-parent", "#msi-cells", 0) {
 		d = irq_find_matching_host(it.node, token);
-		if (d)
+		if (d) {
+			of_node_put(it.node);
 			return d;
+		}
 	}
 
 	return NULL;
-- 
2.50.1

Re: [PATCH v2 2/4] of/irq: Fix OF node refcount in of_msi_get_domain()

[Frank Li]

On Tue, Oct 14, 2025 at 11:58:43AM +0200, Lorenzo Pieralisi wrote:
> In of_msi_get_domain() if the iterator loop stops early because an
> irq_domain match is detected, an of_node_put() on the iterator node is
> needed to keep the OF node refcount in sync.
>
> Add it.
>
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

After go though of_for_each_phandle, I understand why need of_node_put()
at break branch.

It will be nice if add of_for_each_phandle_scoped() help macro.


>  drivers/of/irq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index e67b2041e73b..9f6cd5abba76 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -773,8 +773,10 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
>
>  	of_for_each_phandle(&it, err, np, "msi-parent", "#msi-cells", 0) {
>  		d = irq_find_matching_host(it.node, token);
> -		if (d)
> +		if (d) {
> +			of_node_put(it.node);
>  			return d;
> +		}
>  	}
>
>  	return NULL;
> --
> 2.50.1
>

Re: [PATCH v2 2/4] of/irq: Fix OF node refcount in of_msi_get_domain()

[Lorenzo Pieralisi]

On Tue, Oct 14, 2025 at 06:20:55PM -0400, Frank Li wrote:
> On Tue, Oct 14, 2025 at 11:58:43AM +0200, Lorenzo Pieralisi wrote:
> > In of_msi_get_domain() if the iterator loop stops early because an
> > irq_domain match is detected, an of_node_put() on the iterator node is
> > needed to keep the OF node refcount in sync.
> >
> > Add it.
> >
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > ---
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> After go though of_for_each_phandle, I understand why need of_node_put()
> at break branch.
> 
> It will be nice if add of_for_each_phandle_scoped() help macro.

Yes because this fix is not the end of it AFAICS.

Please review and test patch (4) as well since I slightly change
the existing logic there, I don't want to break the EP mapping code you
added in f1680d9081e1 (by the way, I don't get that commit logic, if the
msi-parent loop would match it could just return and we could have
removed the

if (ret)

guarding of_map_id(), correct ?).

That's what I did in patch (4), please have a look.

Thanks,
Lorenzo

> 
> 
> >  drivers/of/irq.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index e67b2041e73b..9f6cd5abba76 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -773,8 +773,10 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
> >
> >  	of_for_each_phandle(&it, err, np, "msi-parent", "#msi-cells", 0) {
> >  		d = irq_find_matching_host(it.node, token);
> > -		if (d)
> > +		if (d) {
> > +			of_node_put(it.node);
> >  			return d;
> > +		}
> >  	}
> >
> >  	return NULL;
> > --
> > 2.50.1
> >

Re: [PATCH v2 2/4] of/irq: Fix OF node refcount in of_msi_get_domain()

[Frank Li]

On Wed, Oct 15, 2025 at 10:03:44AM +0200, Lorenzo Pieralisi wrote:
> On Tue, Oct 14, 2025 at 06:20:55PM -0400, Frank Li wrote:
> > On Tue, Oct 14, 2025 at 11:58:43AM +0200, Lorenzo Pieralisi wrote:
> > > In of_msi_get_domain() if the iterator loop stops early because an
> > > irq_domain match is detected, an of_node_put() on the iterator node is
> > > needed to keep the OF node refcount in sync.
> > >
> > > Add it.
> > >
> > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > Cc: Rob Herring <robh@kernel.org>
> > > ---
> >
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >
> > After go though of_for_each_phandle, I understand why need of_node_put()
> > at break branch.
> >
> > It will be nice if add of_for_each_phandle_scoped() help macro.
>
> Yes because this fix is not the end of it AFAICS.
>
> Please review and test patch (4) as well since I slightly change
> the existing logic there, I don't want to break the EP mapping code you
> added in f1680d9081e1 (by the way, I don't get that commit logic, if the
> msi-parent loop would match it could just return and we could have
> removed the
>
> if (ret)
>
> guarding of_map_id(), correct ?).
>
> That's what I did in patch (4), please have a look.

It looks correct. PCIE-EP use msi-map. There are comments from Marc Zyngier.
Suppose you will update new version. I will test after new version posted.

Frank
>
> Thanks,
> Lorenzo
>
> >
> >
> > >  drivers/of/irq.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > index e67b2041e73b..9f6cd5abba76 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -773,8 +773,10 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
> > >
> > >  	of_for_each_phandle(&it, err, np, "msi-parent", "#msi-cells", 0) {
> > >  		d = irq_find_matching_host(it.node, token);
> > > -		if (d)
> > > +		if (d) {
> > > +			of_node_put(it.node);
> > >  			return d;
> > > +		}
> > >  	}
> > >
> > >  	return NULL;
> > > --
> > > 2.50.1
> > >

[PATCH v2 3/4] PCI: iproc: Implement MSI controller node detection with of_msi_xlate()

[Lorenzo Pieralisi]

The functionality implemented in the iproc driver in order to detect an
OF MSI controller node is now fully implemented in of_msi_xlate().

Replace the current msi-map/msi-parent parsing code with of_msi_xlate().

Since of_msi_xlate() is also a deviceID mapping API, pass in a fictitious
0 as deviceID - the driver only requires detecting the OF MSI controller
node not the deviceID mapping per-se (of_msi_xlate() return value is
ignored for the same reason).

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
---
 drivers/pci/controller/pcie-iproc.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index 22134e95574b..ccf71993ea35 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -17,6 +17,7 @@
 #include <linux/irqchip/arm-gic-v3.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/phy/phy.h>
@@ -1337,29 +1338,16 @@ static int iproc_pcie_msi_steer(struct iproc_pcie *pcie,
 
 static int iproc_pcie_msi_enable(struct iproc_pcie *pcie)
 {
-	struct device_node *msi_node;
+	struct device_node *msi_node = NULL;
 	int ret;
 
 	/*
 	 * Either the "msi-parent" or the "msi-map" phandle needs to exist
 	 * for us to obtain the MSI node.
 	 */
-
-	msi_node = of_parse_phandle(pcie->dev->of_node, "msi-parent", 0);
-	if (!msi_node) {
-		const __be32 *msi_map = NULL;
-		int len;
-		u32 phandle;
-
-		msi_map = of_get_property(pcie->dev->of_node, "msi-map", &len);
-		if (!msi_map)
-			return -ENODEV;
-
-		phandle = be32_to_cpup(msi_map + 1);
-		msi_node = of_find_node_by_phandle(phandle);
-		if (!msi_node)
-			return -ENODEV;
-	}
+	of_msi_xlate(pcie->dev, &msi_node, 0);
+	if (!msi_node)
+		return -ENODEV;
 
 	/*
 	 * Certain revisions of the iProc PCIe controller require additional
-- 
2.50.1

Re: [PATCH v2 3/4] PCI: iproc: Implement MSI controller node detection with of_msi_xlate()

[Lorenzo Pieralisi]

On Tue, Oct 14, 2025 at 11:58:44AM +0200, Lorenzo Pieralisi wrote:
> The functionality implemented in the iproc driver in order to detect an
> OF MSI controller node is now fully implemented in of_msi_xlate().
> 
> Replace the current msi-map/msi-parent parsing code with of_msi_xlate().
> 
> Since of_msi_xlate() is also a deviceID mapping API, pass in a fictitious
> 0 as deviceID - the driver only requires detecting the OF MSI controller
> node not the deviceID mapping per-se (of_msi_xlate() return value is
> ignored for the same reason).
> 
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> ---
>  drivers/pci/controller/pcie-iproc.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)

Heads-up (I will have to respin anyway), of_msi_xlate() needs to be
exported for this to work when compiled as a module, kbuild-bot barfed
at it, I have fixed it already - if anyone can help me test it anyway
that would be great.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index 22134e95574b..ccf71993ea35 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -17,6 +17,7 @@
>  #include <linux/irqchip/arm-gic-v3.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
> @@ -1337,29 +1338,16 @@ static int iproc_pcie_msi_steer(struct iproc_pcie *pcie,
>  
>  static int iproc_pcie_msi_enable(struct iproc_pcie *pcie)
>  {
> -	struct device_node *msi_node;
> +	struct device_node *msi_node = NULL;
>  	int ret;
>  
>  	/*
>  	 * Either the "msi-parent" or the "msi-map" phandle needs to exist
>  	 * for us to obtain the MSI node.
>  	 */
> -
> -	msi_node = of_parse_phandle(pcie->dev->of_node, "msi-parent", 0);
> -	if (!msi_node) {
> -		const __be32 *msi_map = NULL;
> -		int len;
> -		u32 phandle;
> -
> -		msi_map = of_get_property(pcie->dev->of_node, "msi-map", &len);
> -		if (!msi_map)
> -			return -ENODEV;
> -
> -		phandle = be32_to_cpup(msi_map + 1);
> -		msi_node = of_find_node_by_phandle(phandle);
> -		if (!msi_node)
> -			return -ENODEV;
> -	}
> +	of_msi_xlate(pcie->dev, &msi_node, 0);
> +	if (!msi_node)
> +		return -ENODEV;
>  
>  	/*
>  	 * Certain revisions of the iProc PCIe controller require additional
> -- 
> 2.50.1
> 

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

[Lorenzo Pieralisi]

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
- 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;
+	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);
+
+			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);
 }
 
 static int of_v5_pmsi_get_msi_info(struct irq_domain *domain, struct device *dev,
 				   u32 *dev_id, phys_addr_t *pa)
 {
-	int ret, index = 0;
-	/*
-	 * Retrieve the DeviceID and the ITS translate frame node pointer
-	 * out of the msi-parent property.
-	 */
-	do {
-		struct of_phandle_args args;
-
-		ret = of_parse_phandle_with_args(dev->of_node,
-						 "msi-parent", "#msi-cells",
-						 index, &args);
-		if (ret)
-			break;
-		/*
-		 * The IRQ domain fwnode is the msi controller parent
-		 * in GICv5 (where the msi controller nodes are the
-		 * ITS translate frames).
-		 */
-		if (args.np->parent == irq_domain_get_of_node(domain)) {
-			if (WARN_ON(args.args_count != 1))
-				return -EINVAL;
-			*dev_id = args.args[0];
-
-			ret = its_translate_frame_address(args.np, pa);
-			if (ret)
-				return -ENODEV;
-			break;
-		}
-		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) {
-			ret = its_translate_frame_address(np, pa);
-			of_node_put(np);
-		}
-	}
-
-	return ret;
+	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)
-- 
2.50.1

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

[Marc Zyngier]

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.

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

[Lorenzo Pieralisi]

On Tue, Oct 14, 2025 at 06:12:11PM +0100, Marc Zyngier wrote:
> 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?

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

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

Ok.

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

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

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