[PATCH 0/3] irqdomain fixes for 4.5-rc1

[PATCH 0/3] irqdomain fixes for 4.5-rc1

[Marc Zyngier]

This short series addresses a couple of shortcomings of the irqdomain
subsystem:

(1) The use of DOMAIN_BUS_ANY for looking up wired interrupts can
    result in interesting situations if a given interrupt controller
    implements both wired and message interrupts...

(2) Using DOMAIN_BUS_ANY for MSI lookup is completely redundant (it
    can never match with any of the existing drivers), and is likely
    to fail when we introduce HW similar to what is described in (1).

(3) platform_msi_domain_{alloc,free}_irqs need to be exported so that
    drivers using MSIs can be built as modules.

These patches are a pre-requisite for Thomas Petazzoni's work on the
MVEBU interrupt controller, which outlined the above issues:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395429.html

Thanks,

	M.

Marc Zyngier (2):
  irqdomain: Allow domain lookup with DOMAIN_BUS_WIRED token
  of: MSI: Simplify irqdomain lookup

Thomas Petazzoni (1):
  base: Export platform_msi_domain_{alloc,free}_irqs

 drivers/base/platform-msi.c |  2 ++
 drivers/of/irq.c            | 18 +++---------------
 include/linux/irqdomain.h   |  1 +
 kernel/irq/irqdomain.c      | 11 ++++++++---
 4 files changed, 14 insertions(+), 18 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] irqdomain: Allow domain lookup with DOMAIN_BUS_WIRED token

[Marc Zyngier]

Let's take the (outlandish) example of an interrupt controller
capable of handling both wired interrupts and PCI MSIs.

With the current code, the PCI MSI domain is going to be tagged
with DOMAIN_BUS_PCI_MSI, and the wired domain with DOMAIN_BUS_ANY.

Things get hairy when we start looking up the domain for a wired
interrupt (typically when creating it based on some firmware
information - DT or ACPI).

In irq_create_fwspec_mapping(), we perform the lookup using
DOMAIN_BUS_ANY, which is actually used as a wildcard. This gives
us one chance out of two to end up with the wrong domain, and
we try to configure a wired interrupt with the MSI domain.
Everything grinds to a halt pretty quickly.

What we really need to do is to start looking for a domain that
would uniquely identify a wired interrupt domain, and only use
DOMAIN_BUS_ANY as a fallback.

In order to solve this, let's introduce a new DOMAIN_BUS_WIRED
token, which is going to be used exactly as described above.
Of course, this depends on the irqchip to setup the domain
bus_token, and nobody had to implement this so far.

Only so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irqdomain.h |  1 +
 kernel/irq/irqdomain.c    | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index d5e5c5b..b236d82 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -70,6 +70,7 @@ struct irq_fwspec {
  */
 enum irq_domain_bus_token {
 	DOMAIN_BUS_ANY		= 0,
+	DOMAIN_BUS_WIRED,
 	DOMAIN_BUS_PCI_MSI,
 	DOMAIN_BUS_PLATFORM_MSI,
 	DOMAIN_BUS_NEXUS,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa961..def2664 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -573,10 +573,15 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	unsigned int type = IRQ_TYPE_NONE;
 	int virq;
 
-	if (fwspec->fwnode)
-		domain = irq_find_matching_fwnode(fwspec->fwnode, DOMAIN_BUS_ANY);
-	else
+	if (fwspec->fwnode) {
+		domain = irq_find_matching_fwnode(fwspec->fwnode,
+						  DOMAIN_BUS_WIRED);
+		if (!domain)
+			domain = irq_find_matching_fwnode(fwspec->fwnode,
+							  DOMAIN_BUS_ANY);
+	} else {
 		domain = irq_default_domain;
+	}
 
 	if (!domain) {
 		pr_warn("no irq domain found for %s !\n",
-- 
2.1.4

Re: [PATCH 1/3] irqdomain: Allow domain lookup with DOMAIN_BUS_WIRED token

[Thomas Petazzoni]

Dear Marc Zyngier,

On Tue, 26 Jan 2016 13:52:25 +0000, Marc Zyngier wrote:
> Let's take the (outlandish) example of an interrupt controller
> capable of handling both wired interrupts and PCI MSIs.
> 
> With the current code, the PCI MSI domain is going to be tagged
> with DOMAIN_BUS_PCI_MSI, and the wired domain with DOMAIN_BUS_ANY.
> 
> Things get hairy when we start looking up the domain for a wired
> interrupt (typically when creating it based on some firmware
> information - DT or ACPI).
> 
> In irq_create_fwspec_mapping(), we perform the lookup using
> DOMAIN_BUS_ANY, which is actually used as a wildcard. This gives
> us one chance out of two to end up with the wrong domain, and
> we try to configure a wired interrupt with the MSI domain.
> Everything grinds to a halt pretty quickly.
> 
> What we really need to do is to start looking for a domain that
> would uniquely identify a wired interrupt domain, and only use
> DOMAIN_BUS_ANY as a fallback.
> 
> In order to solve this, let's introduce a new DOMAIN_BUS_WIRED
> token, which is going to be used exactly as described above.
> Of course, this depends on the irqchip to setup the domain
> bus_token, and nobody had to implement this so far.
> 
> Only so far.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

 - On Marvell Armada XP, which uses the Marvell MPIC for both wired
   interrupts and MSI interrupts

 - On Marvell Armada 38x, which uses the ARM GIC for most wired
   interrupts and the Marvell MPIC for MSI interrupts

With an Intel e1000e PCIe NIC, with both PCI_MSI=y and PCI_MSI disabled
cases have been tested. When MSI support is disabled it gracefully
falls back to a wired interrupt, as expected.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/3] of: MSI: Simplify irqdomain lookup

[Marc Zyngier]

So far, when trying to associate a device with its MSI domain,
we first lookup the domain using a MSI token, and if this
doesn't return anything useful, we pick up any domain matching
the same node.

This logic is broken for two reasons:
1) Only the generic MSI code (PCI or platform) sets this token
   to PCI/MSI or platform MSI. So we're guaranteed that if there
   is something to be found, we will find it with the first call.
2) If we have a convoluted situation where:
   - a single node implements both wired and MSI interrupts
   - MSI support for that HW hasn't been compiled in
   we'll end up using the wired domain for MSIs anyway, and things
   break badly.

So let's just remove __of_get_msi_domain, and replace it by a direct
call to irq_find_matching_host, because that's what we really want.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/of/irq.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 4fa916d..a9ea552 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -680,18 +680,6 @@ u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
 	return __of_msi_map_rid(dev, &msi_np, rid_in);
 }
 
-static struct irq_domain *__of_get_msi_domain(struct device_node *np,
-					      enum irq_domain_bus_token token)
-{
-	struct irq_domain *d;
-
-	d = irq_find_matching_host(np, token);
-	if (!d)
-		d = irq_find_host(np);
-
-	return d;
-}
-
 /**
  * of_msi_map_get_device_domain - Use msi-map to find the relevant MSI domain
  * @dev: device for which the mapping is to be done.
@@ -707,7 +695,7 @@ struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 rid)
 	struct device_node *np = NULL;
 
 	__of_msi_map_rid(dev, &np, rid);
-	return __of_get_msi_domain(np, DOMAIN_BUS_PCI_MSI);
+	return irq_find_matching_host(np, DOMAIN_BUS_PCI_MSI);
 }
 
 /**
@@ -731,7 +719,7 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
 	/* Check for a single msi-parent property */
 	msi_np = of_parse_phandle(np, "msi-parent", 0);
 	if (msi_np && !of_property_read_bool(msi_np, "#msi-cells")) {
-		d = __of_get_msi_domain(msi_np, token);
+		d = irq_find_matching_host(msi_np, token);
 		if (!d)
 			of_node_put(msi_np);
 		return d;
@@ -745,7 +733,7 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
 		while (!of_parse_phandle_with_args(np, "msi-parent",
 						   "#msi-cells",
 						   index, &args)) {
-			d = __of_get_msi_domain(args.np, token);
+			d = irq_find_matching_host(args.np, token);
 			if (d)
 				return d;
 
-- 
2.1.4

Re: [PATCH 2/3] of: MSI: Simplify irqdomain lookup

[Rob Herring]

On Tue, Jan 26, 2016 at 7:52 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> So far, when trying to associate a device with its MSI domain,
> we first lookup the domain using a MSI token, and if this
> doesn't return anything useful, we pick up any domain matching
> the same node.
>
> This logic is broken for two reasons:
> 1) Only the generic MSI code (PCI or platform) sets this token
>    to PCI/MSI or platform MSI. So we're guaranteed that if there
>    is something to be found, we will find it with the first call.
> 2) If we have a convoluted situation where:
>    - a single node implements both wired and MSI interrupts
>    - MSI support for that HW hasn't been compiled in
>    we'll end up using the wired domain for MSIs anyway, and things
>    break badly.
>
> So let's just remove __of_get_msi_domain, and replace it by a direct
> call to irq_find_matching_host, because that's what we really want.
>
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/of/irq.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: MSI: Simplify irqdomain lookup

[Thomas Petazzoni]

Dear Marc Zyngier,

On Tue, 26 Jan 2016 13:52:26 +0000, Marc Zyngier wrote:
> So far, when trying to associate a device with its MSI domain,
> we first lookup the domain using a MSI token, and if this
> doesn't return anything useful, we pick up any domain matching
> the same node.
> 
> This logic is broken for two reasons:
> 1) Only the generic MSI code (PCI or platform) sets this token
>    to PCI/MSI or platform MSI. So we're guaranteed that if there
>    is something to be found, we will find it with the first call.
> 2) If we have a convoluted situation where:
>    - a single node implements both wired and MSI interrupts
>    - MSI support for that HW hasn't been compiled in
>    we'll end up using the wired domain for MSIs anyway, and things
>    break badly.
> 
> So let's just remove __of_get_msi_domain, and replace it by a direct
> call to irq_find_matching_host, because that's what we really want.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

(same testing as for PATCH 1/3)

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/3] base: Export platform_msi_domain_{alloc,free}_irqs

[Marc Zyngier]

From: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The new function platform_msi_domain_{alloc,free}_irqs are meant to be
used in platform drivers, which can be built as modules. Therefore, it
makes sense to export them to be used from kernel modules.

Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/base/platform-msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 5df4575..82f4c89 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -246,6 +246,7 @@ out_free_data:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(platform_msi_domain_alloc_irqs);
 
 /**
  * platform_msi_domain_free_irqs - Free MSI interrupts for @dev
@@ -268,3 +269,4 @@ void platform_msi_domain_free_irqs(struct device *dev)
 	msi_domain_free_irqs(dev->msi_domain, dev);
 	platform_msi_free_descs(dev);
 }
+EXPORT_SYMBOL_GPL(platform_msi_domain_free_irqs);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html