[PATCH v2 0/2] of/irq: msi-parent handling in of_msi_xlate()

[PATCH v2 0/2] of/irq: msi-parent handling in of_msi_xlate()

[Fuad Tabba]

Hi folks,

Changes since v1 [1]:
  - Added patch 2: end the walk at the first node that declares an
    msi-parent, with the reference handling moved into of_msi_xlate().
    (Lorenzo)
  - Patch 1 is unchanged.

Patch 1 is the v1 fix: of_check_msi_parent() hands back a node reference
even when the caller passed one in as a filter, which of_msi_xlate()
documents as receiving none. Patch 2 is what Lorenzo asked for [2]: a
node declaring an msi-parent names the MSI controller for what sits
below it, so the walk ends there rather than carrying on to an
ancestor's msi-map.

Patch 2 removes the line patch 1 adds, along with the rest of the
reference handling. They stay separate because patch 1 is a small fix
for a leak that has been there since v6.18, and patch 2 changes
behaviour: folding them would hide the fix inside the change.

Based on Linux 7.3-rc1 (cee9395acd804).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20260904073424.3855365-1-fuad.tabba@linux.dev/
[2] https://lore.kernel.org/all/apqnaNmeuUjfC8Ng@red-moon/

Fuad Tabba (2):
  of/irq: Fix device node refcount leak in of_check_msi_parent()
  of/irq: Stop the MSI walk at the first msi-parent

 drivers/of/irq.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
2.39.5

[PATCH v2 1/2] of/irq: Fix device node refcount leak in of_check_msi_parent()

[Fuad Tabba]

of_msi_xlate() documents @msi_np as a filter when it points at a node on
entry, handing back a reference only when it points at NULL. The msi-map
branch does that. of_check_msi_parent() keeps the reference from the 1:1
msi-parent match either way.

pci_msi_domain_get_msi_rid() is the only caller that passes a node in,
from irq_domain_get_of_node(), and it puts nothing, so a 1:1 match there
leaks the MSI controller node.

Fixes: 119aaeed0b67 ("of/irq: Add msi-parent check to of_msi_xlate()")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260826113604.010C01F000E9@smtp.kernel.org/
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 drivers/of/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index ec035367c9500..109c54f346479 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -785,7 +785,7 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
 	if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
 		ret = -EINVAL;
 
-	if (!ret) {
+	if (!ret && !*msi_node) {
 		/* Return with a node reference held */
 		*msi_node = msi_spec.np;
 		return 0;
-- 
2.39.5

Re: [PATCH v2 1/2] of/irq: Fix device node refcount leak in of_check_msi_parent()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] of_msi_xlate() prematurely aborts device tree hierarchy traversal when msi_np is NULL.
--

commit d227a3bc08eef0cbb22a6f227edca082ac29a9b8
Author: Fuad Tabba <fuad.tabba@linux.dev>
of/irq: Fix device node refcount leak in of_check_msi_parent()

This patch prevents a device node reference count leak in
of_check_msi_parent(). It ensures that the msi_node reference is
only kept when the incoming node pointer is NULL.

Link: https://lore.kernel.org/all/20260826113604.010C01F000E9@smtp.kernel.org/

> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index ec035367c9500..109c54f346479 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -785,7 +785,7 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
>  	if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
>  		ret = -EINVAL;
>  
> -	if (!ret) {
> +	if (!ret && !*msi_node) {
>  		/* Return with a node reference held */
>  		*msi_node = msi_spec.np;
>  		return 0;

[Severity: High]
This isn't a regression introduced by this patch, but could the traversal
loop in of_msi_xlate() prematurely abort when msi_np is NULL?

If a driver calls of_msi_xlate(dev, NULL, id) and the direct parent lacks an
msi-map property, of_map_msi_id() passes the NULL msi_np to of_map_id()
as filter_np:

drivers/of/base.c:of_map_id() {
	...
	map = of_get_property(np, map_name, &map_bytes);
	if (!map) {
		if (filter_np)
			return -ENODEV;
		/* Otherwise, no map implies no translation */
		arg->args[0] = id;
		arg->args_count = 1;
		return 0;
	}
	...
}

Because filter_np is NULL, it returns 0 instead of -ENODEV.

This translates to !of_map_msi_id(...) evaluating to true in the loop in
of_msi_xlate(), hitting the break statement:

drivers/of/irq.c:of_msi_xlate() {
	...
	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
		struct of_phandle_args msi_spec = {};

		if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
			if (msi_spec.np) {
				...
			}
			/* msi-map present but no match → stop walking */
			break;
		}
		...
	}
	...
}

Will this cause the function to incorrectly terminate the loop on the very
first node, conflating an absent msi-map with a present but unmatched
msi-map, and failing to find the MSI mapping in ancestor nodes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907112944.313618-1-fuad.tabba@linux.dev?part=1

Re: [PATCH v2 1/2] of/irq: Fix device node refcount leak in of_check_msi_parent()

[Fuad Tabba]

On Mon, 7 Sept 2026 at 12:45, <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] of_msi_xlate() prematurely aborts device tree hierarchy traversal when msi_np is NULL.

Already covered in my reply to v1 [1]. No in-tree caller hits it:
fsl_mc_get_msi_id() is the only one passing NULL, and an fsl_mc_device
has no of_node, so the walk doesn't abort there.

Cheers,
/fuad

[1] https://lore.kernel.org/all/CA+EHjTx6ZC90qHoccTE6CLvGvxrXBe3Ph0bFKcgBvfRpCCAVuA@mail.gmail.com/

> --
>
> commit d227a3bc08eef0cbb22a6f227edca082ac29a9b8
> Author: Fuad Tabba <fuad.tabba@linux.dev>
> of/irq: Fix device node refcount leak in of_check_msi_parent()
>
> This patch prevents a device node reference count leak in
> of_check_msi_parent(). It ensures that the msi_node reference is
> only kept when the incoming node pointer is NULL.
>
> Link: https://lore.kernel.org/all/20260826113604.010C01F000E9@smtp.kernel.org/
>
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index ec035367c9500..109c54f346479 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -785,7 +785,7 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
> >       if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
> >               ret = -EINVAL;
> >
> > -     if (!ret) {
> > +     if (!ret && !*msi_node) {
> >               /* Return with a node reference held */
> >               *msi_node = msi_spec.np;
> >               return 0;
>
> [Severity: High]
> This isn't a regression introduced by this patch, but could the traversal
> loop in of_msi_xlate() prematurely abort when msi_np is NULL?
>
> If a driver calls of_msi_xlate(dev, NULL, id) and the direct parent lacks an
> msi-map property, of_map_msi_id() passes the NULL msi_np to of_map_id()
> as filter_np:
>
> drivers/of/base.c:of_map_id() {
>         ...
>         map = of_get_property(np, map_name, &map_bytes);
>         if (!map) {
>                 if (filter_np)
>                         return -ENODEV;
>                 /* Otherwise, no map implies no translation */
>                 arg->args[0] = id;
>                 arg->args_count = 1;
>                 return 0;
>         }
>         ...
> }
>
> Because filter_np is NULL, it returns 0 instead of -ENODEV.
>
> This translates to !of_map_msi_id(...) evaluating to true in the loop in
> of_msi_xlate(), hitting the break statement:
>
> drivers/of/irq.c:of_msi_xlate() {
>         ...
>         for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>                 struct of_phandle_args msi_spec = {};
>
>                 if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
>                         if (msi_spec.np) {
>                                 ...
>                         }
>                         /* msi-map present but no match → stop walking */
>                         break;
>                 }
>                 ...
>         }
>         ...
> }
>
> Will this cause the function to incorrectly terminate the loop on the very
> first node, conflating an absent msi-map with a present but unmatched
> msi-map, and failing to find the MSI mapping in ancestor nodes?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907112944.313618-1-fuad.tabba@linux.dev?part=1

[PATCH v2 2/2] of/irq: Stop the MSI walk at the first msi-parent

[Fuad Tabba]

of_check_msi_parent() returns -EINVAL both when the msi-parent names a
controller other than the one the caller filters on and when #msi-cells
isn't 0, so of_msi_xlate() can't tell the two apart and carries on up
the hierarchy. An ancestor's msi-map then maps the device onto a
controller its own node didn't name.

Hand the parsed specifier back to of_msi_xlate(), which owns the
reference as it already does on the msi-map path, and end the walk at
any node that declares an msi-parent.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260904074800.875391F00A3F@smtp.kernel.org/
Suggested-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Link: https://lore.kernel.org/all/apqnaNmeuUjfC8Ng@red-moon/
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 drivers/of/irq.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 109c54f346479..4fbbd13f4d0a8 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -765,34 +765,27 @@ 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)
+static int of_check_msi_parent(struct device_node *dev_node, struct device_node **msi_node,
+			       struct of_phandle_args *msi_spec)
 {
-	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.
+	 * @msi_spec keeps a reference to the target node whenever the
+	 * phandle parses, -EINVAL included, and the caller releases it.
 	 */
 	ret = of_parse_phandle_with_optional_args(dev_node, "msi-parent", "#msi-cells",
-						  0, &msi_spec);
+						  0, msi_spec);
 	if (ret)
 		return ret;
 
-	if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
-		ret = -EINVAL;
+	if ((*msi_node && *msi_node != msi_spec->np) || msi_spec->args_count != 0)
+		return -EINVAL;
 
-	if (!ret && !*msi_node) {
-		/* Return with a node reference held */
-		*msi_node = msi_spec.np;
-		return 0;
-	}
-	of_node_put(msi_spec.np);
-
-	return ret;
+	return 0;
 }
 
 /**
@@ -806,7 +799,9 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
  * @id_in: Device ID.
  *
  * Walk up the device hierarchy looking for devices with a "msi-map"
- * or "msi-parent" property. If found, apply the mapping to @id_in.
+ * or "msi-parent" property. If found, apply the mapping to @id_in. With
+ * @msi_np non-NULL, a device declaring an msi-parent ends the walk, usable
+ * or not.
  *
  * Returns: The mapped MSI id.
  */
@@ -821,6 +816,7 @@ u32 of_msi_xlate(struct device *dev, struct device_node **msi_np, u32 id_in)
 	 */
 	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
 		struct of_phandle_args msi_spec = {};
+		int ret;
 
 		if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
 			if (msi_spec.np) {
@@ -835,8 +831,17 @@ u32 of_msi_xlate(struct device *dev, struct device_node **msi_np, u32 id_in)
 			break;
 		}
 		/* -ENODEV: msi-map absent → check for msi-parent */
-		if (msi_np && !of_check_msi_parent(parent_dev->of_node, msi_np))
+		if (!msi_np)
+			continue;
+
+		ret = of_check_msi_parent(parent_dev->of_node, msi_np, &msi_spec);
+		if (msi_spec.np) {
+			/* A declared msi-parent names the controller, usable or not */
+			if (!ret && !*msi_np)
+				*msi_np = of_node_get(msi_spec.np);
+			of_node_put(msi_spec.np);
 			break;
+		}
 	}
 	return id_out;
 }
-- 
2.39.5