Patchwork next/mmotm unbootable on G5: irqdomain

login
register
mail settings
Submitter Benjamin Herrenschmidt
Date July 23, 2012, 2:45 a.m.
Message ID <1343011543.2957.2.camel@pasglop>
Download mbox | patch
Permalink /patch/172536/
State Not Applicable
Headers show

Comments

Benjamin Herrenschmidt - July 23, 2012, 2:45 a.m.
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>
---
Benjamin Herrenschmidt - July 23, 2012, 6:32 a.m.
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.
Hugh Dickins - July 23, 2012, 7:24 a.m.
On Mon, 23 Jul 2012, Benjamin Herrenschmidt wrote:
> 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

Confirmed: many thanks, Ben.

> (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);
>  
>  	/*
> 
> 
>
Grant Likely - July 23, 2012, 7:59 a.m.
On Sun, Jul 22, 2012 at 8:45 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 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 ?)

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.

>
> 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);
>
>         /*
>
>
Benjamin Herrenschmidt - July 23, 2012, 10:26 p.m.
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.
Grant Likely - July 23, 2012, 10:31 p.m.
On Mon, Jul 23, 2012 at 4:26 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 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...

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.
Grant Likely - July 23, 2012, 10:32 p.m.
On Mon, Jul 23, 2012 at 4:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jul 23, 2012 at 4:26 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> 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...
>
> 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.
Benjamin Herrenschmidt - July 24, 2012, 3:21 a.m.
On Mon, 2012-07-23 at 16:32 -0600, Grant Likely wrote:
> > 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.
Grant Likely - July 24, 2012, 4:37 a.m.
On Mon, Jul 23, 2012 at 9:21 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-07-23 at 16:32 -0600, Grant Likely wrote:
>> > 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.
Grant Likely - July 25, 2012, 5:09 a.m.
On Mon, Jul 23, 2012 at 12:32 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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.

Patch

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);
 
 	/*