drivers/of/irq.c | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-)
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 #msi-cells == 0.
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.
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 e7c12abd10ab..d0e2dfd0ee28 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -670,6 +670,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.
@@ -677,7 +706,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.
@@ -691,12 +720,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.48.0
On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > 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. Your line wrapping is a bit short. I had a look at who is parsing "msi-parent" themselves as that's typically a recipe for doing it incorrectly ('interrupt-map' anyone). Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching into the GICv3 node... And perhaps irq-gic-its-msi-parent.c could use this? And looks like pcie-layerscape-gen4 is leaking a node reference... > > 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 e7c12abd10ab..d0e2dfd0ee28 100644 > --- a/drivers/of/irq.c > +++ b/drivers/of/irq.c > @@ -670,6 +670,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. > @@ -677,7 +706,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. > @@ -691,12 +720,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.48.0 >
On Thu, Sep 18, 2025 at 08:55:55AM -0500, Rob Herring wrote: > On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > > > 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. > > Your line wrapping is a bit short. > > I had a look at who is parsing "msi-parent" themselves as that's > typically a recipe for doing it incorrectly ('interrupt-map' anyone). > Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching > into the GICv3 node... > > And perhaps irq-gic-its-msi-parent.c could use this? > > And looks like pcie-layerscape-gen4 is leaking a node reference... Hi Rob, given where we are in the development cycle and that technically this is a GICv5 fix I would send a patch to fix the issue in GICv5 MSI parent code in drivers/irqchip (yes, it is more code handling the msi-parent property scattered around) and then will resume this thread for v6.19 - starting the clean-up. This patch affects more platforms than GICv5 and I am not sure it is safe to rush it in. Thanks, Lorenzo > > > > 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 e7c12abd10ab..d0e2dfd0ee28 100644 > > --- a/drivers/of/irq.c > > +++ b/drivers/of/irq.c > > @@ -670,6 +670,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. > > @@ -677,7 +706,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. > > @@ -691,12 +720,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.48.0 > >
On Thu, Sep 18, 2025 at 08:55:55AM -0500, Rob Herring wrote: > On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > > > 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. > > Your line wrapping is a bit short. > > I had a look at who is parsing "msi-parent" themselves as that's > typically a recipe for doing it incorrectly ('interrupt-map' anyone). > Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching > into the GICv3 node... I am not sure I get what you mean here. Possibly iproc_pcie_msi_enable() can reuse this patch's code if I extend it and make it a global function, yes and somehow use that function to carry out the check for an msi-parent property with no #msi-cells property or an #msi-cells == 0. Don't get what GICv3 node has to do with that though, sorry. > And perhaps irq-gic-its-msi-parent.c could use this? Yes and I have to fix leaks there too (and there are other of_phandle_with_args() calls in the kernel that don't put the args.np pointer on success - though not to retrieve msi-parent). > > And looks like pcie-layerscape-gen4 is leaking a node reference... It looks like it is in good company I am afraid :( Thanks, Lorenzo > > > > 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 e7c12abd10ab..d0e2dfd0ee28 100644 > > --- a/drivers/of/irq.c > > +++ b/drivers/of/irq.c > > @@ -670,6 +670,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. > > @@ -677,7 +706,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. > > @@ -691,12 +720,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.48.0 > >
On Thu, Sep 18, 2025 at 10:21 AM Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > On Thu, Sep 18, 2025 at 08:55:55AM -0500, Rob Herring wrote: > > On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > > > > > 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. > > > > Your line wrapping is a bit short. > > > > I had a look at who is parsing "msi-parent" themselves as that's > > typically a recipe for doing it incorrectly ('interrupt-map' anyone). > > Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching > > into the GICv3 node... > > I am not sure I get what you mean here. Possibly iproc_pcie_msi_enable() > can reuse this patch's code if I extend it and make it a global function, > yes and somehow use that function to carry out the check for an > msi-parent property with no #msi-cells property or an #msi-cells == 0. I meant using of_msi_xlate() (or even of_msi_get_domain()). > Don't get what GICv3 node has to do with that though, sorry. Just trace what the code there does after it gets the MSI parent. I didn't study it too closely, but why is a iProc PCIe parsing GICv3 MSI stuff itself? There's either some missing feature in the irqchip/domain APIs or it's being dumb. Rob
On Thu, Sep 18, 2025 at 02:44:23PM -0500, Rob Herring wrote: > On Thu, Sep 18, 2025 at 10:21 AM Lorenzo Pieralisi > <lpieralisi@kernel.org> wrote: > > > > On Thu, Sep 18, 2025 at 08:55:55AM -0500, Rob Herring wrote: > > > On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > > > > > > > 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. > > > > > > Your line wrapping is a bit short. > > > > > > I had a look at who is parsing "msi-parent" themselves as that's > > > typically a recipe for doing it incorrectly ('interrupt-map' anyone). > > > Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching > > > into the GICv3 node... > > > > I am not sure I get what you mean here. Possibly iproc_pcie_msi_enable() > > can reuse this patch's code if I extend it and make it a global function, > > yes and somehow use that function to carry out the check for an > > msi-parent property with no #msi-cells property or an #msi-cells == 0. > > I meant using of_msi_xlate() (or even of_msi_get_domain()). https://lore.kernel.org/all/20240818161816.GA173148-robh@kernel.org/ Isn't of_msi_get_domain() leaking a reference when it finds an IRQ domain and return ? Possibly I can generalize it and use it in this patch too. Thanks, Lorenzo > > Don't get what GICv3 node has to do with that though, sorry. > > Just trace what the code there does after it gets the MSI parent. I > didn't study it too closely, but why is a iProc PCIe parsing GICv3 MSI > stuff itself? There's either some missing feature in the > irqchip/domain APIs or it's being dumb. > > Rob
On Thu, Sep 18, 2025 at 02:44:23PM -0500, Rob Herring wrote: > On Thu, Sep 18, 2025 at 10:21 AM Lorenzo Pieralisi > <lpieralisi@kernel.org> wrote: > > > > On Thu, Sep 18, 2025 at 08:55:55AM -0500, Rob Herring wrote: > > > On Tue, Sep 16, 2025 at 11:18:58AM +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 #msi-cells == 0. > > > > > > > > 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. > > > > > > Your line wrapping is a bit short. > > > > > > I had a look at who is parsing "msi-parent" themselves as that's > > > typically a recipe for doing it incorrectly ('interrupt-map' anyone). > > > Can we make iproc_pcie_msi_enable() use this? It's quite ugly reaching > > > into the GICv3 node... > > > > I am not sure I get what you mean here. Possibly iproc_pcie_msi_enable() > > can reuse this patch's code if I extend it and make it a global function, > > yes and somehow use that function to carry out the check for an > > msi-parent property with no #msi-cells property or an #msi-cells == 0. > > I meant using of_msi_xlate() (or even of_msi_get_domain()). > > > Don't get what GICv3 node has to do with that though, sorry. > > Just trace what the code there does after it gets the MSI parent. I > didn't study it too closely, but why is a iProc PCIe parsing GICv3 MSI > stuff itself? There's either some missing feature in the > irqchip/domain APIs or it's being dumb. I think it is setting up the host bridge to steer MSIs to the GIC ITS. Platform specific stuff - nothing to do with irqchip/domain. Lorenzo
© 2016 - 2025 Red Hat, Inc.