next/mmotm unbootable on G5: irqdomain

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

Hugh

commit 94f036a1f242f98cc30700b7676c07270a9c5c27
Author: Grant Likely [off-list ref]
Date:   Sun Jun 3 22:04:39 2012 -0700

irqdomain: eliminate slow-path revmap lookups

With the current state of irq_domain, the reverse map is always updated
when new IRQs get mapped.  This means that the irq_find_mapping() function
can be simplified to execute the revmap lookup functions unconditionally

This patch adds lookup functions for the revmaps that don't yet have one
and removes the slow path lookup code path.

v8: Broke out unrelated changes into separate patches.  Rebased on Paul's irq
    association patches.
v7: Rebased to irqdomain/next for v3.4 and applied before the removal of 'hint'
v6: Remove the slow path entirely.  The only place where the slow path
    could get called is for a linear mapping if the hwirq number is larger
    than the linear revmap size.  There shouldn't be any interrupt
    controllers that do that.
v5: rewrite to not use a ->revmap() callback.  It is simpler, smaller,
    safer and faster to open code each of the revmap lookups directly into
    irq_find_mapping() via a switch statement.
v4: Fix build failure on incorrect variable reference.

Signed-off-by: Grant Likely <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thomas Gleixner <redacted>
Cc: Milton Miller <redacted>
Cc: Paul Mundt <redacted>
Cc: Rob Herring <redacted>

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index c0e638b..a9b810e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -686,16 +686,11 @@ EXPORT_SYMBOL_GPL(irq_dispose_mapping);
  * irq_find_mapping() - Find a linux irq from an hw irq number.
  * @domain: domain owning this hardware interrupt
  * @hwirq: hardware irq number in that domain space
- *
- * This is a slow path, for use by generic code. It's expected that an
- * irq controller implementation directly calls the appropriate low level
- * mapping function.
  */
 unsigned int irq_find_mapping(struct irq_domain *domain,
 			      irq_hw_number_t hwirq)
 {
-	unsigned int i;
-	unsigned int hint = hwirq % nr_irqs;
+	struct irq_data *data;
 
 	/* Look for default domain if nececssary */
 	if (domain == NULL)

@@ -703,22 +698,27 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
 	if (domain == NULL)
 		return 0;
 
-	/* legacy -> bail early */
-	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+	switch (domain->revmap_type) {
+	case IRQ_DOMAIN_MAP_LEGACY:
 		return irq_domain_legacy_revmap(domain, hwirq);
-
-	/* Slow path does a linear search of the map */
-	if (hint == 0)
-		hint = 1;
-	i = hint;
-	do {
-		struct irq_data *data = irq_get_irq_data(i);
+	case IRQ_DOMAIN_MAP_LINEAR:
+		return irq_linear_revmap(domain, hwirq);
+	case IRQ_DOMAIN_MAP_TREE:
+		rcu_read_lock();
+		data = radix_tree_lookup(&domain->revmap_data.tree, hwirq);
+		rcu_read_unlock();
+		if (data)
+			return data->irq;
+		break;
+	case IRQ_DOMAIN_MAP_NOMAP:
+		data = irq_get_irq_data(hwirq);
 		if (data && (data->domain == domain) && (data->hwirq == hwirq))
-			return i;
-		i++;
-		if (i >= nr_irqs)
-			i = 1;
-	} while(i != hint);
+			return hwirq;
+		break;
+	}
+
+	WARN(1, "ERROR: irq revmap went horribly wrong. revmap_type=%i\n",
+		domain->revmap_type);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_find_mapping);

@@ -728,32 +728,19 @@ EXPORT_SYMBOL_GPL(irq_find_mapping);
  * @domain: domain owning this hardware interrupt
  * @hwirq: hardware irq number in that domain space
  *
- * This is a fast path, for use by irq controller code that uses linear
- * revmaps. It does fallback to the slow path if the revmap doesn't exist
- * yet and will create the revmap entry with appropriate locking
+ * This is a fast path that can be called directly by irq controller code to
+ * save a handful of instructions.
  */
 unsigned int irq_linear_revmap(struct irq_domain *domain,
 			       irq_hw_number_t hwirq)
 {
-	unsigned int *revmap;
+	BUG_ON(domain->revmap_type != IRQ_DOMAIN_MAP_LINEAR);
 
-	if (WARN_ON_ONCE(domain->revmap_type != IRQ_DOMAIN_MAP_LINEAR))
-		return irq_find_mapping(domain, hwirq);
-
-	/* Check revmap bounds */
-	if (unlikely(hwirq >= domain->revmap_data.linear.size))
-		return irq_find_mapping(domain, hwirq);
-
-	/* Check if revmap was allocated */
-	revmap = domain->revmap_data.linear.revmap;
-	if (unlikely(revmap == NULL))
-		return irq_find_mapping(domain, hwirq);
-
-	/* Fill up revmap with slow path if no mapping found */
-	if (unlikely(!revmap[hwirq]))
-		revmap[hwirq] = irq_find_mapping(domain, hwirq);
+	/* Check revmap bounds; complain if exceeded */
+	if (WARN_ON(hwirq >= domain->revmap_data.linear.size))
+		return 0;
 
-	return revmap[hwirq];
+	return domain->revmap_data.linear.revmap[hwirq];
 }
 EXPORT_SYMBOL_GPL(irq_linear_revmap);
 

On Sat, 2012-07-21 at 19:47 -0700, Hugh Dickins wrote:

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

Remind me your G5 variant ? (/proc/cpuinfo will do). I'll have a look
tomorrow (and thanks for testing !).

Cheers,
Ben.

On Sun, 22 Jul 2012, Benjamin Herrenschmidt wrote:

On Sat, 2012-07-21 at 19:47 -0700, Hugh Dickins wrote:

quoted

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

Remind me your G5 variant ? (/proc/cpuinfo will do). I'll have a look
tomorrow (and thanks for testing !).

quoted

commit 94f036a1f242f98cc30700b7676c07270a9c5c27
Author: Grant Likely [off-list ref]
Date:   Sun Jun 3 22:04:39 2012 -0700

irqdomain: eliminate slow-path revmap lookups

Thanks, Ben - here's my /proc/cpuinfo:

processor	: 0
cpu		: PPC970MP, altivec supported
clock		: 2500.000000MHz
revision	: 1.1 (pvr 0044 0101)

processor	: 1
cpu		: PPC970MP, altivec supported
clock		: 2500.000000MHz
revision	: 1.1 (pvr 0044 0101)

processor	: 2
cpu		: PPC970MP, altivec supported
clock		: 2500.000000MHz
revision	: 1.1 (pvr 0044 0101)

processor	: 3
cpu		: PPC970MP, altivec supported
clock		: 2500.000000MHz
revision	: 1.1 (pvr 0044 0101)

timebase	: 33333333
platform	: PowerMac
model		: PowerMac11,2
machine		: PowerMac11,2
motherboard	: PowerMac11,2 MacRISC4 Power Macintosh 
detected as	: 337 (PowerMac G5 Dual Core)
pmac flags	: 00000000
L2 cache	: 1024K unified
pmac-generation	: NewWorld

Hugh

On Sat, 2012-07-21 at 19:47 -0700, Hugh Dickins wrote:

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

This fixes it (Grant, how do we avoid bisection breakage here ? I can
put that in -powerpc and we can make sure that gets merged before your
tree ?)

powerpc/mpic: Create a revmap with enough entries for IPIs and timers

The current mpic code creates a linear revmap just big enough for all
the sources, which happens to miss the IPIs and timers on some machines.

This will in turn break when the irqdomain code loses the fallback of
doing a linear search when the revmap fails (and really slows down IPIs
otherwise).

This happens for example on the U4 based Apple machines such as the
dual core PowerMac G5s.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 906f29c..bfc6211 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c

@@ -1376,7 +1376,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
 	mpic->isu_mask = (1 << mpic->isu_shift) - 1;
 
 	mpic->irqhost = irq_domain_add_linear(mpic->node,
-				       last_irq + 1,
+				       intvec_top,
 				       &mpic_host_ops, mpic);
 
 	/*

Allright, another one Grant:

unsigned int irq_find_mapping(struct irq_domain *domain,
			      irq_hw_number_t hwirq)
{
	struct irq_data *data;

	/* Look for default domain if nececssary */
	if (domain == NULL)
		domain = irq_default_domain;
	if (domain == NULL)
		return 0;

	switch (domain->revmap_type) {
	case IRQ_DOMAIN_MAP_LEGACY:
		return irq_domain_legacy_revmap(domain, hwirq);
	case IRQ_DOMAIN_MAP_LINEAR:
		return irq_linear_revmap(domain, hwirq);
	case IRQ_DOMAIN_MAP_TREE:
		rcu_read_lock();
		data = radix_tree_lookup(&domain->revmap_data.tree, hwirq);
		rcu_read_unlock();
		if (data)
			return data->irq;
-               break;
+               return 0;
	case IRQ_DOMAIN_MAP_NOMAP:

Please, stick a proper commit message and my s-o-b and see if you can fix
your tree before you ask Linus to pull because that's not pretty on any
pseries .... irq_find_mapping() does get called for all interrupt the
first time it's mapped to check if there's a pre-existing mapping, so
the case of the thing being unpopulated is absolutely legit.

the NOMAP case has a similar dubious exit case but since I'm not that
familiar with NOMAP I haven't touched it.

Cheers,
Ben.

On Mon, 23 Jul 2012, Benjamin Herrenschmidt wrote:

On Sat, 2012-07-21 at 19:47 -0700, Hugh Dickins wrote:

quoted

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

This fixes it

Confirmed: many thanks, Ben.

On Sun, Jul 22, 2012 at 8:45 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Sat, 2012-07-21 at 19:47 -0700, Hugh Dickins wrote:

quoted

I have to revert the patch below from mmotm 2012-07-20-16-30 or
next-20120720 in order to boot on the PowerPC G5: otherwise it
freezes before switching to the framebuffer console - but I'm
not certain where because that initial console doesn't scroll
(there are mpic messages at bottom and at top of screen, probably
later messages at the top but I don't know the sequence).

This fixes it (Grant, how do we avoid bisection breakage here ? I can
put that in -powerpc and we can make sure that gets merged before your
tree ?)

My tree must be rebased to eliminate bisect breakage. The existing
commits in my tree have the breakage, and fiddling with the merge
order doesn't affect that. I don't want to rebase though. The safest
approach (smallest window of breakage) is to apply that fix onto my
irqdomain tree.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

On Mon, 2012-07-23 at 01:59 -0600, Grant Likely wrote:

My tree must be rebased to eliminate bisect breakage. The existing
commits in my tree have the breakage, and fiddling with the merge
order doesn't affect that. I don't want to rebase though. The safest
approach (smallest window of breakage) is to apply that fix onto my
irqdomain tree.

With your other breakage on pseries I'm thinking rebasing might be the
only option...

Cheers,
Ben.

On Mon, Jul 23, 2012 at 4:26 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Mon, 2012-07-23 at 01:59 -0600, Grant Likely wrote:

quoted

My tree must be rebased to eliminate bisect breakage. The existing
commits in my tree have the breakage, and fiddling with the merge
order doesn't affect that. I don't want to rebase though. The safest
approach (smallest window of breakage) is to apply that fix onto my
irqdomain tree.

With your other breakage on pseries I'm thinking rebasing might be the
only option...

Fair enough. I'm not planning to ask Linus to pull for a few days yet
anyway. I've been pretty useless as a kernel maintainer for the last 3
months so I want to give a bit more time in linux-next to catch
fallout before it gets merged.

As-is I'm backing off from the linear/legacy/tree merge patch as just
too risky. I've already pulled that stuff out of linux-next.

g.

On Mon, Jul 23, 2012 at 4:31 PM, Grant Likely [off-list ref] wrote:

On Mon, Jul 23, 2012 at 4:26 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

On Mon, 2012-07-23 at 01:59 -0600, Grant Likely wrote:

quoted

My tree must be rebased to eliminate bisect breakage. The existing
commits in my tree have the breakage, and fiddling with the merge
order doesn't affect that. I don't want to rebase though. The safest
approach (smallest window of breakage) is to apply that fix onto my
irqdomain tree.

With your other breakage on pseries I'm thinking rebasing might be the
only option...

Fair enough. I'm not planning to ask Linus to pull for a few days yet
anyway. I've been pretty useless as a kernel maintainer for the last 3
months so I want to give a bit more time in linux-next to catch
fallout before it gets merged.

As-is I'm backing off from the linear/legacy/tree merge patch as just
too risky. I've already pulled that stuff out of linux-next.

Can I pull you pseries fix into my tree (my preference), or do I need
to rebase on top of yours?

g.

On Mon, 2012-07-23 at 16:32 -0600, Grant Likely wrote:

quoted

As-is I'm backing off from the linear/legacy/tree merge patch as just
too risky. I've already pulled that stuff out of linux-next.

Can I pull you pseries fix into my tree (my preference), or do I need
to rebase on top of yours? 

The mpic fix for the g5 is in Linus tree already, I added it on top of
powerpc -next before I asked Linus to pull.

For pseries (ie the fix for irq_find_mapping vs. radix), I don't have a
formal patch, just the one I hand typed in my previous email, so do
whatever you want with it.

Cheers,
Ben.

On Mon, Jul 23, 2012 at 9:21 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Mon, 2012-07-23 at 16:32 -0600, Grant Likely wrote:

quoted

quoted

As-is I'm backing off from the linear/legacy/tree merge patch as just
too risky. I've already pulled that stuff out of linux-next.

Can I pull you pseries fix into my tree (my preference), or do I need
to rebase on top of yours?

The mpic fix for the g5 is in Linus tree already, I added it on top of
powerpc -next before I asked Linus to pull.

For pseries (ie the fix for irq_find_mapping vs. radix), I don't have a
formal patch, just the one I hand typed in my previous email, so do
whatever you want with it.

Okay, I'll merge in Linus' tree at the appropriate point to protect
against bisection, and I'll fix up the appropriate patch that touches
irq_find_mapping.

g.

On Mon, Jul 23, 2012 at 12:32 AM, Benjamin Herrenschmidt
[off-list ref] wrote:

Allright, another one Grant:

unsigned int irq_find_mapping(struct irq_domain *domain,
                              irq_hw_number_t hwirq)
{
        struct irq_data *data;

        /* Look for default domain if nececssary */
        if (domain == NULL)
                domain = irq_default_domain;
        if (domain == NULL)
                return 0;

        switch (domain->revmap_type) {
        case IRQ_DOMAIN_MAP_LEGACY:
                return irq_domain_legacy_revmap(domain, hwirq);
        case IRQ_DOMAIN_MAP_LINEAR:
                return irq_linear_revmap(domain, hwirq);
        case IRQ_DOMAIN_MAP_TREE:
                rcu_read_lock();
                data = radix_tree_lookup(&domain->revmap_data.tree, hwirq);
                rcu_read_unlock();
                if (data)
                        return data->irq;
-               break;
+               return 0;
        case IRQ_DOMAIN_MAP_NOMAP:

Please, stick a proper commit message and my s-o-b and see if you can fix
your tree before you ask Linus to pull because that's not pretty on any
pseries .... irq_find_mapping() does get called for all interrupt the
first time it's mapped to check if there's a pre-existing mapping, so
the case of the thing being unpopulated is absolutely legit.

the NOMAP case has a similar dubious exit case but since I'm not that
familiar with NOMAP I haven't touched it.

I've decided to rework the patch to simply omit the WARN statement. It
isn't really needed. I've merged in Linus' tree below the eliminate
slow-path patch and remove the WARN statement. It's been pushed out to
my irqdomain/next branch, so it should show up in tomorrow's
linux-next.

You can find it here if you want to give it a spin:

git://git.secretlab.ca/git/linux-2.6 irqdomain/next

I'll give it a bit more time in linux-next before I ask Linus to pull.

g.