[PATCH] fix platform_rename_gsi related ia32 build breakage

[PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 72 bytes --]

Hi Linus,

	This is needed to build current BK tree on IA32.

- Arnaldo

[-- Attachment #2: platform_rename_gsi.patch --]
[-- Type: text/plain, Size: 367 bytes --]

===== arch/i386/kernel/io_apic.c 1.116 vs edited =====
--- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
+++ edited/arch/i386/kernel/io_apic.c	2004-11-10 21:39:57 -02:00
@@ -1039,6 +1039,8 @@
 	return MPBIOS_trigger(idx);
 }
 
+extern int (*platform_rename_gsi)(int ioapic, int gsi);
+
 static int pin_2_irq(int idx, int apic, int pin)
 {
 	int irq, i;

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

arch/i386/kernel/built-in.o(.text+0xeade): In function `pin_2_irq':
: undefined reference to `platform_rename_gsi'
make: ** [.tmp_vmlinux1] Erro 1

Sorry, the patch is not enough...

- Arnaldo

Arnaldo Carvalho de Melo wrote:
> Hi Linus,
> 
>     This is needed to build current BK tree on IA32.
> 
> - Arnaldo
> 
> 
> ------------------------------------------------------------------------
> 
> ===== arch/i386/kernel/io_apic.c 1.116 vs edited =====
> --- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
> +++ edited/arch/i386/kernel/io_apic.c	2004-11-10 21:39:57 -02:00
> @@ -1039,6 +1039,8 @@
>  	return MPBIOS_trigger(idx);
>  }
>  
> +extern int (*platform_rename_gsi)(int ioapic, int gsi);
> +
>  static int pin_2_irq(int idx, int apic, int pin)
>  {
>  	int irq, i;

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

This one compiles and links OK.

- Arnaldo

Arnaldo Carvalho de Melo wrote:
> arch/i386/kernel/built-in.o(.text+0xeade): In function `pin_2_irq':
> : undefined reference to `platform_rename_gsi'
> make: ** [.tmp_vmlinux1] Erro 1
> 
> Sorry, the patch is not enough...
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo wrote:
> 
>> Hi Linus,
>>
>>     This is needed to build current BK tree on IA32.

[-- Attachment #2: a.patch --]
[-- Type: text/plain, Size: 493 bytes --]

===== arch/i386/kernel/io_apic.c 1.116 vs edited =====
--- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
+++ edited/arch/i386/kernel/io_apic.c	2004-11-10 21:58:57 -02:00
@@ -1069,12 +1069,14 @@
 			while (i < apic)
 				irq += nr_ioapic_registers[i++];
 			irq += pin;
+#ifdef CONFIG_ACPI_BOOT
 			/*
 			 * For MPS mode, so far only used by ES7000 platform
 			 */
 			if (platform_rename_gsi)
 				irq = platform_rename_gsi(apic, irq);
 			break;
+#endif
 		}
 		default:
 		{

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

OK, last try, of course the patch is busted because the #endif should
be _before_ the break statement.

/me goes looking for a brown paper bag :-(

- Arnaldo

Arnaldo Carvalho de Melo wrote:
> This one compiles and links OK.
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo wrote:
> 
>> arch/i386/kernel/built-in.o(.text+0xeade): In function `pin_2_irq':
>> : undefined reference to `platform_rename_gsi'
>> make: ** [.tmp_vmlinux1] Erro 1
>>
>> Sorry, the patch is not enough...
>>
>> - Arnaldo
>>
>> Arnaldo Carvalho de Melo wrote:
>>
>>> Hi Linus,
>>>
>>>     This is needed to build current BK tree on IA32.
> 
> 
> ------------------------------------------------------------------------
> 
> ===== arch/i386/kernel/io_apic.c 1.116 vs edited =====
> --- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
> +++ edited/arch/i386/kernel/io_apic.c	2004-11-10 21:58:57 -02:00
> @@ -1069,12 +1069,14 @@
>  			while (i < apic)
>  				irq += nr_ioapic_registers[i++];
>  			irq += pin;
> +#ifdef CONFIG_ACPI_BOOT
>  			/*
>  			 * For MPS mode, so far only used by ES7000 platform
>  			 */
>  			if (platform_rename_gsi)
>  				irq = platform_rename_gsi(apic, irq);
>  			break;
> +#endif
>  		}
>  		default:
>  		{

[-- Attachment #2: a.patch --]
[-- Type: text/plain, Size: 488 bytes --]

===== arch/i386/kernel/io_apic.c 1.116 vs edited =====
--- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
+++ edited/arch/i386/kernel/io_apic.c	2004-11-10 22:17:37 -02:00
@@ -1069,11 +1069,13 @@
 			while (i < apic)
 				irq += nr_ioapic_registers[i++];
 			irq += pin;
+#ifdef CONFIG_ACPI_BOOT
 			/*
 			 * For MPS mode, so far only used by ES7000 platform
 			 */
 			if (platform_rename_gsi)
 				irq = platform_rename_gsi(apic, irq);
+#endif
 			break;
 		}
 		default:

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Linus Torvalds]

On Wed, 10 Nov 2004, Arnaldo Carvalho de Melo wrote:
>
> This one compiles and links OK.

Including when CONFIG_ACPI_BOOT is set? Does it see the prototype then? 
Looks like <asm/acpi.h> should be included, but I assume it gets included 
some other way?

Quite frankly, I think the whole thing is broken. #ifdef's inside code is 
_evil_, and "platform_rename_gsi()" doesn't make sense as a name. I'll 
apply your patch, but quite frankly, I think the ACPI layer is doing crap.

Len, can you please use a more descriptive name, and have it be always 
defined (make it an inline function that just becomes a no-op or 
something).

		Linus

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

Linus Torvalds wrote:
> 
> On Wed, 10 Nov 2004, Arnaldo Carvalho de Melo wrote:
> 
>>This one compiles and links OK.
> 
> 
> Including when CONFIG_ACPI_BOOT is set? Does it see the prototype then? 
> Looks like <asm/acpi.h> should be included, but I assume it gets included 
> some other way?
> 
> Quite frankly, I think the whole thing is broken. #ifdef's inside code is 
> _evil_, and "platform_rename_gsi()" doesn't make sense as a name. I'll 
> apply your patch, but quite frankly, I think the ACPI layer is doing crap.
> 

Agreed, I leave this now to the ACPI guys, it may well be that the whole
case is to be ifdefed or something different, who knows. Just please
apply the one in the message I was looking for a brown paper bag (i.e.
the one with the #endif _before_ the break statement).

Ouch, time to make a hole for the nose, I need some air :o)

> Len, can you please use a more descriptive name, and have it be always 
> defined (make it an inline function that just becomes a no-op or 
> something).
> 
> 		Linus
> 
> 

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Len Brown]

On Wed, 2004-11-10 at 19:24, Linus Torvalds wrote:

> Quite frankly, I think the whole thing is broken. #ifdef's inside code
> is _evil_, and "platform_rename_gsi()" doesn't make sense as a name.
> I'll apply your patch, but quite frankly, I think the ACPI layer is
> doing crap.
> 
> Len, can you please use a more descriptive name, and have it be always
> defined (make it an inline function that just becomes a no-op or
> something).

This is already fixed in the latest ACPI patch, included in the current
-mm tree.  So I've gone ahead and excluded the two csets you applied
adding the #ifdefs.

I used a function pointer here because the same kernel binary must be
able to run on an ES7000 or a non-ES7000, so the compile-time inline
idiom doesn't work.  Originally we called it es7000-something, but since
the mechanism is general it could be used just as well by any non-ES7000
system, so we settled on a more generic name -- platform_rename_gsi().

What this routine does is accomodate exotic hardware.  On the ES7000 the
hardware designers decided to connect their legacy devices to interrupt
pins > 15.  Getting Linux to handle this was goodness because it forced
me to go through and clean up some old interrupt-source-override code
that basically worked by accident.  But on the ES7000, the problem
remained for what to do about the PCI devices connected to interrupt
pins < 16, including pin0.  Without platform_rename_gsi, these pins are
given identity-mapped IRQ#'s below 16, and Linux lives with lots of
legacy code that assumes that IRQ's below 16 are special.  So
platform_rename_gsi() on the ES7000 simply gives these pins new IRQ#'s
starting above what would normally be the highest GSI in the system. 
These number are not subject to any legacy special cases and MSI etc.
work fine.

Finally, there were two places this technicaue is used.  ACPI-mode has
been in for a while.  The one that broke the !ACPI MPS+IOAPIC build
yesterday was new -- it replaced a hack in the MPS code that that was
downright scary because it could potentially break any non-ES7000
system.

If you read this far and have suggestions for a more descriptive name
than platform_rename_gsi(), just let me know.

thanks,
-Len

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Linus Torvalds]

On Thu, 11 Nov 2004, Len Brown wrote:
> 
> I used a function pointer here because the same kernel binary must be
> able to run on an ES7000 or a non-ES7000, so the compile-time inline
> idiom doesn't work. 

Sure it does. Do something like this in a header file

	static inline int translate_irq_number(...)
	{
		#ifdef CONFIG_ACPI_BOOT
			return fn_ptr_xxx();
		#else
			return irq;
		#endif
	}

which means that yes, it uses the function pointer when it is meaningful, 
but if there is no point, the code just goes away.

> If you read this far and have suggestions for a more descriptive name
> than platform_rename_gsi(), just let me know.

At _least_ write out what the hell "gsi" is.

TLA's are bad. "gsi" apparently isn't the Geological Survey of Ireland, 
but that's all I can tell from google.

		Linus

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Len Brown]

On Thu, 2004-11-11 at 17:30, Linus Torvalds wrote:
> 
> On Thu, 11 Nov 2004, Len Brown wrote:
> >
> > I used a function pointer here because the same kernel binary must
> be
> > able to run on an ES7000 or a non-ES7000, so the compile-time inline
> > idiom doesn't work.
> 
> Sure it does. Do something like this in a header file
> 
>         static inline int translate_irq_number(...)
>         {
>                 #ifdef CONFIG_ACPI_BOOT
>                         return fn_ptr_xxx();
>                 #else
>                         return irq;
>                 #endif
>         }

sure, we could add a wrapper for the wrapper,
but the ifdefs are already gone without doing this --
platform_rename_gsi is present with or without ACPI.

I suppose I could shrink the kernel by 4-bytes by compiling out the
function pointer when IO_APIC is not defined.  I'll be happy to optimize
for that case if you think it justifies the code; though if I were
optimizing for that case, this probably isn't where I'd start.

> which means that yes, it uses the function pointer when it is
> meaningful, but if there is no point, the code just goes away.
> 
> > If you read this far and have suggestions for a more descriptive
> name than platform_rename_gsi(), just let me know.
> 
> At _least_ write out what the hell "gsi" is.
> 
> TLA's are bad. "gsi" apparently isn't the Geological Survey of
> Ireland, but that's all I can tell from google.

The _gsi in platform_rename_gsi was consistent with the surrounding use
in the ACPI case.  I decided to re-use the same funtion for the MPS case
for simplicity, even though io_apic.c uses _irq.  If you like, I can add
a synonym using an inline for _irq, but I thought we were moving away
from using _irq, not towards it.

GSI = Global System Interrupt
IRQ = overused so much it means nothing at all

While this fact may not win you over as a fan, GSI is actually
consistent with the language in the ACPI spec.  I don't know if that is
where Bjorn came up with the name, but I do think it was a positive
change.  The reason is that "global" actually means something -- and it
still has the same semantics no matter if your machine passes around cpu
interrupt vectors or if it passes around pin numbers.

cheers,
-Len

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Linus Torvalds]

On Thu, 11 Nov 2004, Len Brown wrote:
> 
> The _gsi in platform_rename_gsi was consistent with the surrounding use
> in the ACPI case.  I decided to re-use the same funtion for the MPS case
> for simplicity, even though io_apic.c uses _irq.  If you like, I can add
> a synonym using an inline for _irq, but I thought we were moving away
> from using _irq, not towards it.

We _definitely_ prefer "irq" over something else that means the same 
thing.

If GSI means some _specific_ interrupt, and thus has additional meaning 
over "irq", then by all means, use it, but spell it out. "Global System 
Interrupt" means _nothing_ to me. What makes it "global"? What makes it 
"system"?

The _only_ thing that uses "gsi" is the MP table stuff, and that's 
apparently just from the documentation.

In other words, if it's a normal interrupt, it's "irq" or "interrupt". The 
same way a "disk" is a "disk" - it's not a DASD. 

Stupid acronyms that don't actually mean anything more than the standard 
name are nothing but stupid.

Interrupts are interrupts. We call them something else only if they are 
_specific_ interrupts (ie a "NMI" clearly has a very _specific_ meaning, 
as has "SCI", although the latter is already obscure enough that it's 
probably a good idea to spell it out a bit if it is ever used outside of a 
context where use is obvious).

		Linus

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Len Brown]

On Thu, 2004-11-11 at 19:01, Linus Torvalds wrote:
> 
> On Thu, 11 Nov 2004, Len Brown wrote:

> We _definitely_ prefer "irq" over something else that means the same
> thing.

Hopeless, I know, to debate the "Royal We" on points of style,
especially when underscores have been invoked;-)

But humor me -- offer a concise definition of the term IRQ, and then
examine usage of the term to see if actually fits within the
definition.  The term started off to mean the interrupt pin numbers on
the 8259A PIC, then it started to mean PCI interrupts (PIRQ) pins too,
and sort of IO-APIC pin numbers, except of course when the pin numbers
don't actually linearly map to the IRQ numbers, and it can also be an
interrupt vector, you see some system never had 8259's, and it can also
be the name of the routine that handles an interrupt, and there are
softirqs, which are different from hardirqs, and there are fixup-irqs
etc. etc.

It isn't possible to offer a term that means the same thing as IRQ,
because IRQ is so over-used that it no longer means anything at all.

The term GSI is simple, it is an interrupt number that applies to the
entire system, as if the system had a completely flat interrupt model. 
They even drew a little picture to make this clear in the ACPI spec at
http://www.acpi.info Who would think such a simple thing would merit a
diagram?

> If GSI means some _specific_ interrupt, and thus has additional
> meaning over "irq", then by all means, use it, but spell it out.
> "Global System Interrupt" means _nothing_ to me. What makes it
> "global"? What makes it "system"?
>
> The _only_ thing that uses "gsi" is the MP table stuff, and that's
> apparently just from the documentation.

Don't those silly tech-writers know that software can be written
directly from the silicon, and that source-code and the terms used in it
is all the documentation that anybody could ever need?:-)

GSI actually did not appear in the MP spec, that referred only to 8259A
ISA/EISA IRQ's, PCI PIRQs, and IOAPIC INTINs.  If they were not PCI
interrupts, the "source bus IRQs" are the same as 8259A Interrupt
Request pins.

> In other words, if it's a normal interrupt, it's "irq" or "interrupt".
> The same way a "disk" is a "disk" - it's not a DASD.
> 
> Stupid acronyms that don't actually mean anything more than the
> standard name are nothing but stupid.

I agree 100%, and submit that the term "IRQ" fits this definition of a
"stupid acronym".

> Interrupts are interrupts. We call them something else only if they
> are _specific_ interrupts (ie a "NMI" clearly has a very _specific_
> meaning, as has "SCI", although the latter is already obscure enough
> that it's probably a good idea to spell it out a bit if it is ever
> used outside of a context where use is obvious).

It is true that a GSI and 8259 ISA IRQ are synonyms when their values
are < 16., but the term IRQ is being mis-used whenever it is not in that
context.

-Len

ps. note that x86 built with MSI or other architectures such as ia64
translate the simple GSI number into a vector and pass it around in that
form.  I don't know if this is a bug or a feature, but doing so means
that all the interrupt code didn't have to be re-written to accomodate
these varied architectures, and the semantics of a GSI as a unique
global integer identifying an interrupt source remain intact.

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Linus Torvalds]

On Thu, 11 Nov 2004, Len Brown wrote:
> 
> I agree 100%, and submit that the term "IRQ" fits this definition of a
> "stupid acronym".

There's a huge difference between an acronyn that is well-established, and 
one that is _totally_ made up, has no history, and is only used on one 
platform.

"irq" is a very traditional shorthand for "interrupt request", and anybody 
who has _ever_ worked with any OS on _any_ platform knows _exactly_ what 
it is.

In contrast, gsi has _zero_ meaning outside some small ACPI group. 

Trust me. Do a poll. 

The same way we don't call disks DASD devices do we not call interrupts 
gsi's.

And that "we" is not a "royal we".  It's a f*cking established _fact_. 
Type "irq" into google, and what's the first hit? In fact, EVERY SINGLE 
hit on the first page is relevant.

In contrast, type "gsi" into google, and NOT A SINGLE ONE has _anything_ 
to do with interrupts. Not the first one, not the tenth one.

So stop being idiotic.

		Linus

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Linus Torvalds]

On Wed, 10 Nov 2004, Arnaldo Carvalho de Melo wrote:
> 
> 	This is needed to build current BK tree on IA32.

Can you please put it into some sane header file, so that if the 
definition of this thing ever changes, we'll get an error instead of a 
wrong type silently being used.

In fact, it _is_ in a hader file: <asm/acpi.h>.

Why not just include it?

		Linus

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Arnaldo Carvalho de Melo]

Linus Torvalds wrote:
> 
> On Wed, 10 Nov 2004, Arnaldo Carvalho de Melo wrote:
> 
>>	This is needed to build current BK tree on IA32.
> 
> 
> Can you please put it into some sane header file, so that if the 
> definition of this thing ever changes, we'll get an error instead of a 
> wrong type silently being used.
> 
> In fact, it _is_ in a hader file: <asm/acpi.h>.
> 
> Why not just include it?

Look at the other messages in this brown paper bag saga... It is already
in asm/acpi.h, but depends on some config options, etc.

- Arnaldo

Re: [PATCH] fix platform_rename_gsi related ia32 build breakage

[Andrew Morton]

Arnaldo Carvalho de Melo <acme@conectiva.com.br> wrote:
>
> --- 1.116/arch/i386/kernel/io_apic.c	2004-10-28 05:35:33 -03:00
> +++ edited/arch/i386/kernel/io_apic.c	2004-11-10 21:39:57 -02:00
> @@ -1039,6 +1039,8 @@
>  	return MPBIOS_trigger(idx);
>  }
>  
> +extern int (*platform_rename_gsi)(int ioapic, int gsi);

This defeats compiler typechecking.  Please place extern declarations in
header files where they are visible to the definition and to all users.