[PATCH] voyager: fix up for moving the generic smp call func to pointers

[PATCH] voyager: fix up for moving the generic smp call func to pointers

[James Bottomley]

commit e7986739a76cde5079da08809d8bbc6878387ae0
Author: Mike Travis <travis@sgi.com>
Date:   Tue Dec 16 17:33:52 2008 -0800

    x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers

Changed all the IPI ops to take pointers, but didn't update voyager,
which is now broken.  Make the changes in voyager as well.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/x86/mach-voyager/voyager_smp.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/mach-voyager/voyager_smp.c b/arch/x86/mach-voyager/voyager_smp.c
index b1b1bd3..7ffcdee 100644
--- a/arch/x86/mach-voyager/voyager_smp.c
+++ b/arch/x86/mach-voyager/voyager_smp.c
@@ -81,7 +81,7 @@ static void enable_local_vic_irq(unsigned int irq);
 static void disable_local_vic_irq(unsigned int irq);
 static void before_handle_vic_irq(unsigned int irq);
 static void after_handle_vic_irq(unsigned int irq);
-static void set_vic_irq_affinity(unsigned int irq, cpumask_t mask);
+static void set_vic_irq_affinity(unsigned int irq, const struct cpumask *mask);
 static void ack_vic_irq(unsigned int irq);
 static void vic_enable_cpi(void);
 static void do_boot_cpu(__u8 cpuid);
@@ -1597,16 +1597,16 @@ static void after_handle_vic_irq(unsigned int irq)
  * change the mask and then do an interrupt enable CPI to re-enable on
  * the selected processors */
 
-void set_vic_irq_affinity(unsigned int irq, cpumask_t mask)
+void set_vic_irq_affinity(unsigned int irq, const struct cpumask *mask)
 {
 	/* Only extended processors handle interrupts */
 	unsigned long real_mask;
 	unsigned long irq_mask = 1 << irq;
 	int cpu;
 
-	real_mask = cpus_addr(mask)[0] & voyager_extended_vic_processors;
+	real_mask = cpus_addr(*mask)[0] & voyager_extended_vic_processors;
 
-	if (cpus_addr(mask)[0] == 0)
+	if (cpus_addr(*mask)[0] == 0)
 		/* can't have no CPUs to accept the interrupt -- extremely
 		 * bad things will happen */
 		return;
@@ -1782,9 +1782,9 @@ void __init smp_setup_processor_id(void)
 	x86_write_percpu(cpu_number, hard_smp_processor_id());
 }
 
-static void voyager_send_call_func(cpumask_t callmask)
+static void voyager_send_call_func(const struct cpumask *callmask)
 {
-	__u32 mask = cpus_addr(callmask)[0] & ~(1 << smp_processor_id());
+	__u32 mask = cpus_addr(*callmask)[0] & ~(1 << smp_processor_id());
 	send_CPI(mask, VIC_CALL_FUNCTION_CPI);
 }
 
-- 
1.5.6.6

Re: [PATCH] voyager: fix up for moving the generic smp call func to pointers

[Ingo Molnar]

James,

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> commit e7986739a76cde5079da08809d8bbc6878387ae0
> Author: Mike Travis <travis@sgi.com>
> Date:   Tue Dec 16 17:33:52 2008 -0800
> 
>     x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers
> 
> Changed all the IPI ops to take pointers, but didn't update voyager,
> which is now broken.  Make the changes in voyager as well.

Note: earlier this week i've rearchitectured the x86 subarchitecture and 
APIC code - so there's a whole lot more to be done on the Voyager side 
than just the cpumask conversion.

The x86/Voyager subarch is not fully ported yet (i cleaned up its Kconfig 
impact already) and hence it is disabled for the time being. It ought to 
be relatively straightforward to port it to the new code - if you are 
interested in doing that.

You can find the latest code in tip/master at:

   http://people.redhat.com/mingo/tip.git/README

it's tentatively aimed for v2.6.30.

If you have any questions/suggestions about this code, feel free ...

	Ingo

Re: [PATCH] voyager: fix up for moving the generic smp call func to pointers

[James Bottomley]

On Fri, 2009-01-30 at 19:54 +0100, Ingo Molnar wrote:
> James,
> 
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > commit e7986739a76cde5079da08809d8bbc6878387ae0
> > Author: Mike Travis <travis@sgi.com>
> > Date:   Tue Dec 16 17:33:52 2008 -0800
> > 
> >     x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers
> > 
> > Changed all the IPI ops to take pointers, but didn't update voyager,
> > which is now broken.  Make the changes in voyager as well.
> 
> Note: earlier this week i've rearchitectured the x86 subarchitecture and 
> APIC code - so there's a whole lot more to be done on the Voyager side 
> than just the cpumask conversion.

Not for 2.6.29 there isn't.

> The x86/Voyager subarch is not fully ported yet (i cleaned up its Kconfig 
> impact already) and hence it is disabled for the time being. It ought to 
> be relatively straightforward to port it to the new code - if you are 
> interested in doing that.
> 
> You can find the latest code in tip/master at:
> 
>    http://people.redhat.com/mingo/tip.git/README
> 
> it's tentatively aimed for v2.6.30.

Right, and I'll fix the rest for 2.6.30.  However, this patch is needed
now to get 2.6.29-rc3 to boot.

James

Re: [PATCH] voyager: fix up for moving the generic smp call func to pointers

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Fri, 2009-01-30 at 19:54 +0100, Ingo Molnar wrote:
> > James,
> > 
> > * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > commit e7986739a76cde5079da08809d8bbc6878387ae0
> > > Author: Mike Travis <travis@sgi.com>
> > > Date:   Tue Dec 16 17:33:52 2008 -0800
> > > 
> > >     x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers
> > > 
> > > Changed all the IPI ops to take pointers, but didn't update voyager,
> > > which is now broken.  Make the changes in voyager as well.
> > 
> > Note: earlier this week i've rearchitectured the x86 subarchitecture and 
> > APIC code - so there's a whole lot more to be done on the Voyager side 
> > than just the cpumask conversion.
> 
> Not for 2.6.29 there isn't.
> 
> > The x86/Voyager subarch is not fully ported yet (i cleaned up its Kconfig 
> > impact already) and hence it is disabled for the time being. It ought to 
> > be relatively straightforward to port it to the new code - if you are 
> > interested in doing that.
> > 
> > You can find the latest code in tip/master at:
> > 
> >    http://people.redhat.com/mingo/tip.git/README
> > 
> > it's tentatively aimed for v2.6.30.
> 
> Right, and I'll fix the rest for 2.6.30. [...]

ok.

> [...] However, this patch is needed now to get 2.6.29-rc3 to boot.

and there's also the other ones. We can put them in now that we've got the 
new subarch code. Could you send a pull request with all the voyager fixes 
collected into one tree?

	Ingo

Re: [PATCH] voyager: fix up for moving the generic smp call func to pointers

[James Bottomley]

On Fri, 2009-01-30 at 20:38 +0100, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Fri, 2009-01-30 at 19:54 +0100, Ingo Molnar wrote:
> > > James,
> > > 
> > > * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > 
> > > > commit e7986739a76cde5079da08809d8bbc6878387ae0
> > > > Author: Mike Travis <travis@sgi.com>
> > > > Date:   Tue Dec 16 17:33:52 2008 -0800
> > > > 
> > > >     x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers
> > > > 
> > > > Changed all the IPI ops to take pointers, but didn't update voyager,
> > > > which is now broken.  Make the changes in voyager as well.
> > > 
> > > Note: earlier this week i've rearchitectured the x86 subarchitecture and 
> > > APIC code - so there's a whole lot more to be done on the Voyager side 
> > > than just the cpumask conversion.
> > 
> > Not for 2.6.29 there isn't.
> > 
> > > The x86/Voyager subarch is not fully ported yet (i cleaned up its Kconfig 
> > > impact already) and hence it is disabled for the time being. It ought to 
> > > be relatively straightforward to port it to the new code - if you are 
> > > interested in doing that.
> > > 
> > > You can find the latest code in tip/master at:
> > > 
> > >    http://people.redhat.com/mingo/tip.git/README
> > > 
> > > it's tentatively aimed for v2.6.30.
> > 
> > Right, and I'll fix the rest for 2.6.30. [...]
> 
> ok.
> 
> > [...] However, this patch is needed now to get 2.6.29-rc3 to boot.
> 
> and there's also the other ones. We can put them in now that we've got the 
> new subarch code. Could you send a pull request with all the voyager fixes 
> collected into one tree?

Sure, they're currently all here:

master.kernel.org:/pub/scm/linux/kernel/git/jejb/voyager-2.6.git

James

[PATCH] x86/Voyager: make it build and boot

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > > [...] However, this patch is needed now to get 2.6.29-rc3 to boot.
> > 
> > and there's also the other ones. We can put them in now that we've got 
> > the new subarch code. Could you send a pull request with all the 
> > voyager fixes collected into one tree?
> 
> Sure, they're currently all here:
> 
> master.kernel.org:/pub/scm/linux/kernel/git/jejb/voyager-2.6.git

I couldnt pull this tree as it had nearly as many problems as commits:

1) The tree was rebased freshly, losing history.

2) Commit febf1fe ("[VOYAGER] x86: fix voyager QIC interrupt controller") 
   is bogus as it adds only comment but claims to be an ioremap_cache() 
   fix. Probably a rebasing artifact.

4) Commit 569a588 is bogus as it introduces a duplicate Kconfig entry.
   Probably a rebasing/merging artifact.

3) The standard x86 commit log format is not:

       [VOYAGER] fix cpu bootmaps

   But something like:

       x86/Voyager: fix cpu bootmaps

   You even mixed them inconsistently:

       [VOYAGER] fix cpu bootmaps
       [VOYAGER] x86: fix voyager QIC interrupt controller

5) Commit 78543b1 ("[VOYAGER] x86: add ability to test for boot CPU") is 
   ugly and possibly breaks the standard build:

    +#ifdef CONFIG_ACPI
     #include <asm/acpi.h>
    +#endif

   If then such conditionals should be added to the acpi.h header itself 
   and any knock-on build breakages addressed. The construct you used are 
   magnets for standard kernel build breakages.

6) Neither did i like the commit logs berating x86 developers for breaking 
   voyager - while in reality there's only a single Linux developer on the 
   planet who _can_ test Voyager and its assumptions (and that is you).

   Unless you take part in the x86 development process fully (which you
   havent for a long time - you always came one stuff hit upstream and 
   sometimes in late rcs jumping months of x86 development), you cannot 
   expect special treatment nor can you expect people to even be aware of 
   the quirks of Voyager. Development is complex already without a 
   constant distraction from a subarch that is not used by any developer
   but you.

Anyway, to move forward i consolidated the acceptable bits into a single 
low-impact commit and queued it up in x86/urgent - see it below. The 
acpi.h bit you probably still need to build Voyager but it needs 
reworking. Please submit it separately.

Thanks,

	Ingo

-------------------->
>From 92ab78315c638515d0e81b0c70b2082f713582d9 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sat, 31 Jan 2009 17:24:43 +0100
Subject: [PATCH] x86/Voyager: make it build and boot

[
  mingo@elte.hu: these fixes are a subset of changes cherry-picked from:

     git://git.kernel.org:/pub/scm/linux/kernel/git/jejb/voyager-2.6.git

  They fix various problems that recent x86 changes caused in the Voyager
  subarchitecture: both APIC changes and cpumask changes and certain
  cleanups caused subarch assumptions to break.

  Most of these changes are obsolete as the subarch code has been removed
  from the x86 development tree - but we merge them upstream to make Voyager
  build and boot.
]

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/irqinit_32.c        |   12 ------------
 arch/x86/mach-default/setup.c       |   12 ++++++++++++
 arch/x86/mach-voyager/setup.c       |   12 +++++++++++-
 arch/x86/mach-voyager/voyager_smp.c |   25 ++++++++++++-------------
 4 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/irqinit_32.c b/arch/x86/kernel/irqinit_32.c
index 1507ad4..10a09c2 100644
--- a/arch/x86/kernel/irqinit_32.c
+++ b/arch/x86/kernel/irqinit_32.c
@@ -78,15 +78,6 @@ void __init init_ISA_irqs(void)
 	}
 }
 
-/*
- * IRQ2 is cascade interrupt to second interrupt controller
- */
-static struct irqaction irq2 = {
-	.handler = no_action,
-	.mask = CPU_MASK_NONE,
-	.name = "cascade",
-};
-
 DEFINE_PER_CPU(vector_irq_t, vector_irq) = {
 	[0 ... IRQ0_VECTOR - 1] = -1,
 	[IRQ0_VECTOR] = 0,
@@ -178,9 +169,6 @@ void __init native_init_IRQ(void)
 	alloc_intr_gate(THERMAL_APIC_VECTOR, thermal_interrupt);
 #endif
 
-	if (!acpi_ioapic)
-		setup_irq(2, &irq2);
-
 	/* setup after call gates are initialised (usually add in
 	 * the architecture specific gates)
 	 */
diff --git a/arch/x86/mach-default/setup.c b/arch/x86/mach-default/setup.c
index df167f2..a265a7c 100644
--- a/arch/x86/mach-default/setup.c
+++ b/arch/x86/mach-default/setup.c
@@ -38,6 +38,15 @@ void __init pre_intr_init_hook(void)
 	init_ISA_irqs();
 }
 
+/*
+ * IRQ2 is cascade interrupt to second interrupt controller
+ */
+static struct irqaction irq2 = {
+	.handler = no_action,
+	.mask = CPU_MASK_NONE,
+	.name = "cascade",
+};
+
 /**
  * intr_init_hook - post gate setup interrupt initialisation
  *
@@ -53,6 +62,9 @@ void __init intr_init_hook(void)
 		if (x86_quirks->arch_intr_init())
 			return;
 	}
+	if (!acpi_ioapic)
+		setup_irq(2, &irq2);
+
 }
 
 /**
diff --git a/arch/x86/mach-voyager/setup.c b/arch/x86/mach-voyager/setup.c
index a580b95..d914a79 100644
--- a/arch/x86/mach-voyager/setup.c
+++ b/arch/x86/mach-voyager/setup.c
@@ -33,13 +33,23 @@ void __init intr_init_hook(void)
 	setup_irq(2, &irq2);
 }
 
-void __init pre_setup_arch_hook(void)
+static void voyager_disable_tsc(void)
 {
 	/* Voyagers run their CPUs from independent clocks, so disable
 	 * the TSC code because we can't sync them */
 	setup_clear_cpu_cap(X86_FEATURE_TSC);
 }
 
+void __init pre_setup_arch_hook(void)
+{
+	voyager_disable_tsc();
+}
+
+void __init pre_time_init_hook(void)
+{
+	voyager_disable_tsc();
+}
+
 void __init trap_init_hook(void)
 {
 }
diff --git a/arch/x86/mach-voyager/voyager_smp.c b/arch/x86/mach-voyager/voyager_smp.c
index 9840b7e..7ffcdee 100644
--- a/arch/x86/mach-voyager/voyager_smp.c
+++ b/arch/x86/mach-voyager/voyager_smp.c
@@ -81,7 +81,7 @@ static void enable_local_vic_irq(unsigned int irq);
 static void disable_local_vic_irq(unsigned int irq);
 static void before_handle_vic_irq(unsigned int irq);
 static void after_handle_vic_irq(unsigned int irq);
-static void set_vic_irq_affinity(unsigned int irq, cpumask_t mask);
+static void set_vic_irq_affinity(unsigned int irq, const struct cpumask *mask);
 static void ack_vic_irq(unsigned int irq);
 static void vic_enable_cpi(void);
 static void do_boot_cpu(__u8 cpuid);
@@ -211,8 +211,6 @@ static __u32 cpu_booted_map;
 static cpumask_t smp_commenced_mask = CPU_MASK_NONE;
 
 /* This is for the new dynamic CPU boot code */
-cpumask_t cpu_callin_map = CPU_MASK_NONE;
-cpumask_t cpu_callout_map = CPU_MASK_NONE;
 
 /* The per processor IRQ masks (these are usually kept in sync) */
 static __u16 vic_irq_mask[NR_CPUS] __cacheline_aligned;
@@ -378,7 +376,7 @@ void __init find_smp_config(void)
 	cpus_addr(phys_cpu_present_map)[0] |=
 	    voyager_extended_cmos_read(VOYAGER_PROCESSOR_PRESENT_MASK +
 				       3) << 24;
-	cpu_possible_map = phys_cpu_present_map;
+	init_cpu_possible(&phys_cpu_present_map);
 	printk("VOYAGER SMP: phys_cpu_present_map = 0x%lx\n",
 	       cpus_addr(phys_cpu_present_map)[0]);
 	/* Here we set up the VIC to enable SMP */
@@ -1599,16 +1597,16 @@ static void after_handle_vic_irq(unsigned int irq)
  * change the mask and then do an interrupt enable CPI to re-enable on
  * the selected processors */
 
-void set_vic_irq_affinity(unsigned int irq, cpumask_t mask)
+void set_vic_irq_affinity(unsigned int irq, const struct cpumask *mask)
 {
 	/* Only extended processors handle interrupts */
 	unsigned long real_mask;
 	unsigned long irq_mask = 1 << irq;
 	int cpu;
 
-	real_mask = cpus_addr(mask)[0] & voyager_extended_vic_processors;
+	real_mask = cpus_addr(*mask)[0] & voyager_extended_vic_processors;
 
-	if (cpus_addr(mask)[0] == 0)
+	if (cpus_addr(*mask)[0] == 0)
 		/* can't have no CPUs to accept the interrupt -- extremely
 		 * bad things will happen */
 		return;
@@ -1750,10 +1748,11 @@ static void __cpuinit voyager_smp_prepare_boot_cpu(void)
 	init_gdt(smp_processor_id());
 	switch_to_new_gdt();
 
-	cpu_set(smp_processor_id(), cpu_online_map);
-	cpu_set(smp_processor_id(), cpu_callout_map);
-	cpu_set(smp_processor_id(), cpu_possible_map);
-	cpu_set(smp_processor_id(), cpu_present_map);
+	cpu_online_map = cpumask_of_cpu(smp_processor_id());
+	cpu_callout_map = cpumask_of_cpu(smp_processor_id());
+	cpu_callin_map = CPU_MASK_NONE;
+	cpu_present_map = cpumask_of_cpu(smp_processor_id());
+
 }
 
 static int __cpuinit voyager_cpu_up(unsigned int cpu)
@@ -1783,9 +1782,9 @@ void __init smp_setup_processor_id(void)
 	x86_write_percpu(cpu_number, hard_smp_processor_id());
 }
 
-static void voyager_send_call_func(cpumask_t callmask)
+static void voyager_send_call_func(const struct cpumask *callmask)
 {
-	__u32 mask = cpus_addr(callmask)[0] & ~(1 << smp_processor_id());
+	__u32 mask = cpus_addr(*callmask)[0] & ~(1 << smp_processor_id());
 	send_CPI(mask, VIC_CALL_FUNCTION_CPI);
 }
 

Re: [PATCH] x86/Voyager: make it build and boot

[James Bottomley]

On Sat, 2009-01-31 at 20:05 +0100, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > > [...] However, this patch is needed now to get 2.6.29-rc3 to boot.
> > > 
> > > and there's also the other ones. We can put them in now that we've got 
> > > the new subarch code. Could you send a pull request with all the 
> > > voyager fixes collected into one tree?
> > 
> > Sure, they're currently all here:
> > 
> > master.kernel.org:/pub/scm/linux/kernel/git/jejb/voyager-2.6.git
> 
> I couldnt pull this tree as it had nearly as many problems as commits:

> 1) The tree was rebased freshly, losing history.

?  Rebasing doesn't lose any history ... just read the manual page.  It
also tends to be required operating procedure constructing a patch set
against a moving target

> 2) Commit febf1fe ("[VOYAGER] x86: fix voyager QIC interrupt controller") 
>    is bogus as it adds only comment but claims to be an ioremap_cache() 
>    fix. Probably a rebasing artifact.

No it's still required.  That was the problem that occurred when ioremap
was switched from cached to uncached.  You were just trigger happy when
I explained the problem and how to fix it.  The ioremap_cached needs a
comment explaining why it has to be cached.

> 4) Commit 569a588 is bogus as it introduces a duplicate Kconfig entry.
>    Probably a rebasing/merging artifact.

Yes, good catch ... when they go in in different places rebasing doesn't
always catch the updates.

> 3) The standard x86 commit log format is not:
> 
>        [VOYAGER] fix cpu bootmaps
> 
>    But something like:
> 
>        x86/Voyager: fix cpu bootmaps

Well, you actually have to say what you want.  A simple git log will
tell you that [VOYAGER] has been the standard form for years.

>    You even mixed them inconsistently:
> 
>        [VOYAGER] fix cpu bootmaps
>        [VOYAGER] x86: fix voyager QIC interrupt controller

So if I understand, you want x86 and voyager?

> 5) Commit 78543b1 ("[VOYAGER] x86: add ability to test for boot CPU") is 
>    ugly and possibly breaks the standard build:
> 
>     +#ifdef CONFIG_ACPI
>      #include <asm/acpi.h>
>     +#endif

Really nothing should be including asm/acpi.h ... it should be including
linux/acpi.h, which would resolve the issue ... but I didn't understand
why it wanted the asm version in the first place, so I was reluctant to
try fixing it properly in case there was large breakage in
configurations I don't check.  If fixmap.h wants to continue using the
asm version, the ifdef hedging is about the best way: it duplicates what
linux/acpi.h does, if you look (asm/acpi is gated by the same ifdef).

>    If then such conditionals should be added to the acpi.h header itself 
>    and any knock-on build breakages addressed. The construct you used are 
>    magnets for standard kernel build breakages.

Because you're planning to put non-ACPI symbols in asm/acpi.h?  Since
linux/acpi.h works in the same way, you'll break a lot of other things
if you do that.

> 6) Neither did i like the commit logs berating x86 developers for breaking 
>    voyager - while in reality there's only a single Linux developer on the 
>    planet who _can_ test Voyager and its assumptions (and that is you).

They're pretty neutral to my point of view.  They identify the commit
causing the problem and describe the fix.  This is standard changelog
behaviour.  If you identify problem language, I can fix it, but I think
just saying this commit broke and it was fixed by doing that is about as
neutral as you can get.

As I keep explaining, there are two other voyager users, one in Italy
and one in Argentina.  Both institutions with virtually no IT budget
that happened to be stuck with an inventory of the systems.  It might
not be massive, but I bet it's bigger than some other architectures user
bases.

>    Unless you take part in the x86 development process fully (which you
>    havent for a long time - you always came one stuff hit upstream and 
>    sometimes in late rcs jumping months of x86 development), you cannot 
>    expect special treatment nor can you expect people to even be aware of 
>    the quirks of Voyager. Development is complex already without a 
>    constant distraction from a subarch that is not used by any developer
>    but you.

OK, How?

For example: If you want to be involved in SCSI development, you follow
linux-scsi:  every patch that ever makes it into any tree goes over that
list for comment (this includes every patch I do).  If I look at your
x86/apic branch for instance, most of the patches appear to have gone
straight from your fancy into your git tree without ever traversing a
mailing list (or at least not one indexed by google).

If you develop by banging stuff into a git tree as soon as you think of
it, it's a bit hard to participate in that process (I gave up telepathy
in favour of contact sports a while ago).  The best method is, in fact,
to wait until your stuff gets into mainline and then fix it all up,
which, by co-incidence is the way the voyager tree currently operates.

> Anyway, to move forward i consolidated the acceptable bits into a single 
> low-impact commit and queued it up in x86/urgent - see it below. The 
> acpi.h bit you probably still need to build Voyager but it needs 
> reworking. Please submit it separately.

Thanks.

I really would recommend the fix I proposed:  all it does, as I said, is
precisely what linux/acpi.h does.  The problem with trying to include
linux/acpi.h in asm/fixmap.h is that fixmap is an early include and gets
us into all sorts of nasty circular tangles that will be very invasive
to sort out.

James

Re: [PATCH] x86/Voyager: make it build and boot

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Sat, 2009-01-31 at 20:05 +0100, Ingo Molnar wrote:
> > * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > > > [...] However, this patch is needed now to get 2.6.29-rc3 to boot.
> > > > 
> > > > and there's also the other ones. We can put them in now that we've got 
> > > > the new subarch code. Could you send a pull request with all the 
> > > > voyager fixes collected into one tree?
> > > 
> > > Sure, they're currently all here:
> > > 
> > > master.kernel.org:/pub/scm/linux/kernel/git/jejb/voyager-2.6.git
> > 
> > I couldnt pull this tree as it had nearly as many problems as commits:
> 
> > 1) The tree was rebased freshly, losing history.
> 
> ?  Rebasing doesn't lose any history ... just read the manual page.  It 
> also tends to be required operating procedure constructing a patch set 
> against a moving target

That is nonsense - rebasing loses a lot of history: it loses the precise 
place of the commit and re-anchors the commit to another place. It hides the 
cases where you did conflict resolution in a rebase, and it also hides the 
cases where Git auto-merged something. It makes it hard to trust the dates 
in the tree because literally, according to the timestamps, the tree you 
sent me was just a few hours hold.

We dont ever rebase maintainer Git trees - only in the most exceptional 
circumstances. Rebasing is fine for certain situations during development, 
but it is considered evil as a maintenance model.

> > 2) Commit febf1fe ("[VOYAGER] x86: fix voyager QIC interrupt controller") 
> >    is bogus as it adds only comment but claims to be an ioremap_cache() 
> >    fix. Probably a rebasing artifact.
> 
> No it's still required.  That was the problem that occurred when ioremap 
> was switched from cached to uncached.  You were just trigger happy when I 
> explained the problem and how to fix it.  The ioremap_cached needs a 
> comment explaining why it has to be cached.

Have you considered to _take a look_ at the commit i pointed out to you? The 
commit does this:

| From febf1fee10022544674aa01f518a7603b2231191 Mon Sep 17 00:00:00 2001
| From: James Bottomley <James.Bottomley@HansenPartnership.com>
| Date: Fri, 25 Apr 2008 09:02:51 -0500
|
|
| [...] 
| +		/* The QIC address is really special.  QIC does cross
| +		 * CPU interrupts via cache line states, so if there's
| +		 * no caching on these addresses the interrupts stop
| +		 * working.  If ever the QIC stops booting but VIC is
| +		 * fine, suspect that the ioremap here is no longer
| +		 * producing cached memory */
| 		qic_addr = (unsigned long)ioremap_cache(qic_addr, 0x400);

This commit _only adds a comment_. We fixed the ioremap bug upstream eons 
ago (when you pointed it out), and you probably did a conflict resolution in 
a rebase and then perhaps forgot about this.

> > 4) Commit 569a588 is bogus as it introduces a duplicate Kconfig entry.
> >    Probably a rebasing/merging artifact.
> 
> Yes, good catch ... when they go in in different places rebasing doesn't 
> always catch the updates.

Yes, this is one of the typical dangers of rebasing. Besides losing true 
history, you also risk mishaps like that.

That is why the kernel maintainer 101 is: "never rebase". I'm surprised you 
never even heard about that... Does this mean that you frequently rebase the 
SCSI tree?

> > 3) The standard x86 commit log format is not:
> > 
> >        [VOYAGER] fix cpu bootmaps
> > 
> >    But something like:
> > 
> >        x86/Voyager: fix cpu bootmaps
> 
> Well, you actually have to say what you want.  A simple git log will
> tell you that [VOYAGER] has been the standard form for years.
>
> >    You even mixed them inconsistently:
> > 
> >        [VOYAGER] fix cpu bootmaps
> >        [VOYAGER] x86: fix voyager QIC interrupt controller
> 
> So if I understand, you want x86 and voyager?

The format i suggested above would be fine.

> > 5) Commit 78543b1 ("[VOYAGER] x86: add ability to test for boot CPU") is 
> >    ugly and possibly breaks the standard build:
> > 
> >     +#ifdef CONFIG_ACPI
> >      #include <asm/acpi.h>
> >     +#endif
> 
> Really nothing should be including asm/acpi.h ... it should be including
> linux/acpi.h, which would resolve the issue ... but I didn't understand
> why it wanted the asm version in the first place, so I was reluctant to
> try fixing it properly in case there was large breakage in
> configurations I don't check.  If fixmap.h wants to continue using the
> asm version, the ifdef hedging is about the best way: it duplicates what
> linux/acpi.h does, if you look (asm/acpi is gated by the same ifdef).
> 
> >    If then such conditionals should be added to the acpi.h header itself 
> >    and any knock-on build breakages addressed. The construct you used are 
> >    magnets for standard kernel build breakages.
> 
> Because you're planning to put non-ACPI symbols in asm/acpi.h?  Since 
> linux/acpi.h works in the same way, you'll break a lot of other things if 
> you do that.
> 
> > 6) Neither did i like the commit logs berating x86 developers for breaking 
> >    voyager - while in reality there's only a single Linux developer on the 
> >    planet who _can_ test Voyager and its assumptions (and that is you).
> 
> They're pretty neutral to my point of view.  They identify the commit
> causing the problem and describe the fix.  This is standard changelog
> behaviour.  If you identify problem language, I can fix it, but I think
> just saying this commit broke and it was fixed by doing that is about as
> neutral as you can get.
> 
> As I keep explaining, there are two other voyager users, one in Italy and 
> one in Argentina.  Both institutions with virtually no IT budget that 
> happened to be stuck with an inventory of the systems.  It might not be 
> massive, but I bet it's bigger than some other architectures user bases.

What development kernels are they using? I never heard any bugreport from 
them.

> >    Unless you take part in the x86 development process fully (which you
> >    havent for a long time - you always came one stuff hit upstream and 
> >    sometimes in late rcs jumping months of x86 development), you cannot 
> >    expect special treatment nor can you expect people to even be aware of 
> >    the quirks of Voyager. Development is complex already without a 
> >    constant distraction from a subarch that is not used by any developer
> >    but you.
> 
> OK, How?
> 
> For example: If you want to be involved in SCSI development, you follow 
> linux-scsi: every patch that ever makes it into any tree goes over that 
> list for comment (this includes every patch I do).  If I look at your 
> x86/apic branch for instance, most of the patches appear to have gone 
> straight from your fancy into your git tree without ever traversing a 
> mailing list (or at least not one indexed by google).

Regarding the changes in the x86/apic branch, see this thread in your lkml 
folder:

  x86: unify genapic code, unify subarchitectures, remove old subarchitecture code

  http://lwn.net/Articles/317050/
  http://thread.gmane.org/gmane.linux.kernel/786890

With 114 patches submitted to lkml. (there's ~10 more in the works that i'll 
submit to lkml once i'm happy with them)

All patches to the x86 tree are submitted to lkml (including my patches and 
that of other maintainers) and all development is done there.

So i dont get it how you can claim it credibly that you cannot take part in 
x86 development.

> If you develop by banging stuff into a git tree as soon as you think of 
> it, it's a bit hard to participate in that process (I gave up telepathy in 
> favour of contact sports a while ago).  The best method is, in fact, to 
> wait until your stuff gets into mainline and then fix it all up, which, by 
> co-incidence is the way the voyager tree currently operates.

Nice rant which ignores the fact that i submitted those patches to lkml.

> > Anyway, to move forward i consolidated the acceptable bits into a single 
> > low-impact commit and queued it up in x86/urgent - see it below. The 
> > acpi.h bit you probably still need to build Voyager but it needs 
> > reworking. Please submit it separately.
> 
> Thanks.
> 
> I really would recommend the fix I proposed: all it does, as I said, is 
> precisely what linux/acpi.h does.  The problem with trying to include 
> linux/acpi.h in asm/fixmap.h is that fixmap is an early include and gets 
> us into all sorts of nasty circular tangles that will be very invasive to 
> sort out.

Well, the #ifdef hack you did is out of question. The clean approach would 
be to remove the acpi.h include from asm/fixmap.h and fix the dependency 
fallout. The hack you did deepens the spaghetti and makes it 
config-conditional as well - which is even worse than plain header 
dependency mess and which is a step backwards.

	Ingo