[PATCH] add acpi_interrupt_to_irq

[PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

This patch against 2.6.1 tightens up some language and removes
a couple IA64 #ifdefs:

 drivers/acpi/osl.c        |   26 ++++++++++++++------------
 include/asm-i386/acpi.h   |    6 ++++++
 include/asm-x86_64/acpi.h |    6 ++++++
 include/asm-ia64/acpi.h   |    2 +-
 arch/ia64/kernel/acpi.c   |   16 ++++++++++++----
 5 files changed, 39 insertions(+), 17 deletions(-)

ACPI: Add acpi_interrupt_to_irq() interface.

ACPI interrupts and Linux IRQs need not be identical (though they are
on i386 and x86_64), so introduce acpi_interrupt_to_irq(), clean up
usage of "interrupt" and "irq", and remove IA64 #ifdefs.

=== drivers/acpi/osl.c 1.43 vs edited ==--- 1.43/drivers/acpi/osl.c	Mon Dec 29 14:37:24 2003
+++ edited/drivers/acpi/osl.c	Tue Jan 20 15:36:23 2004
@@ -240,23 +240,22 @@
 }
 
 acpi_status
-acpi_os_install_interrupt_handler(u32 irq, OSD_HANDLER handler, void *context)
+acpi_os_install_interrupt_handler(u32 interrupt, OSD_HANDLER handler, void *context)
 {
+	unsigned int irq;
+
 	/*
-	 * Ignore the irq from the core, and use the value in our copy of the
+	 * Ignore the interrupt from the core, and use the value in our copy of the
 	 * FADT. It may not be the same if an interrupt source override exists
 	 * for the SCI.
 	 */
-	irq = acpi_fadt.sci_int;
+	interrupt = acpi_fadt.sci_int;
 
-#ifdef CONFIG_IA64
-	irq = acpi_irq_to_vector(irq);
-	if (irq < 0) {
+	if (acpi_interrupt_to_irq(interrupt, &irq)) {
 		printk(KERN_ERR PREFIX "SCI (ACPI interrupt %d) not registered\n",
-		       acpi_fadt.sci_int);
+		       interrupt);
 		return AE_OK;
 	}
-#endif
 	acpi_irq_irq = irq;
 	acpi_irq_handler = handler;
 	acpi_irq_context = context;
@@ -269,12 +268,15 @@
 }
 
 acpi_status
-acpi_os_remove_interrupt_handler(u32 irq, OSD_HANDLER handler)
+acpi_os_remove_interrupt_handler(u32 interrupt, OSD_HANDLER handler)
 {
+	unsigned int irq;
+
 	if (acpi_irq_handler) {
-#ifdef CONFIG_IA64
-		irq = acpi_irq_to_vector(irq);
-#endif
+		if (acpi_interrupt_to_irq(interrupt, &irq)) {
+			printk(KERN_ERR PREFIX "Can't remove ACPI interrupt handler\n");
+			return AE_ERROR;
+		}
 		free_irq(irq, acpi_irq);
 		acpi_irq_handler = NULL;
 	}
=== include/asm-i386/acpi.h 1.9 vs edited ==--- 1.9/include/asm-i386/acpi.h	Tue Sep 16 11:21:55 2003
+++ edited/include/asm-i386/acpi.h	Tue Jan 20 15:33:24 2004
@@ -139,6 +139,12 @@
 
 #endif
 
+static inline int acpi_interrupt_to_irq(u32 interrupt, unsigned int *irq)
+{
+	*irq = interrupt;
+	return 0;
+}
+
 #ifdef CONFIG_ACPI_SLEEP
 
 /* routines for saving/restoring kernel state */
=== include/asm-x86_64/acpi.h 1.3 vs edited ==--- 1.3/include/asm-x86_64/acpi.h	Wed Dec 31 22:32:36 2003
+++ edited/include/asm-x86_64/acpi.h	Tue Jan 20 15:33:54 2004
@@ -120,6 +120,12 @@
 
 #endif /*CONFIG_ACPI_BOOT*/
 
+static inline int acpi_interrupt_to_irq(u32 interrupt, unsigned int *irq)
+{
+	*irq = interrupt;
+	return 0;
+}
+
 #ifdef CONFIG_ACPI_SLEEP
 
 /* routines for saving/restoring kernel state */
=== include/asm-ia64/acpi.h 1.13 vs edited ==--- 1.13/include/asm-ia64/acpi.h	Mon Jan 12 00:20:13 2004
+++ edited/include/asm-ia64/acpi.h	Tue Jan 20 15:35:01 2004
@@ -91,7 +91,7 @@
 const char *acpi_get_sysname (void);
 int acpi_request_vector (u32 int_type);
 int acpi_register_irq (u32 gsi, u32 polarity, u32 trigger);
-int acpi_irq_to_vector (u32 irq);
+int acpi_interrupt_to_irq (u32 interrupt, unsigned int *irq);
 
 #ifdef CONFIG_ACPI_NUMA
 /* Proximity bitmap length; _PXM is at most 255 (8 bit)*/
=== arch/ia64/kernel/acpi.c 1.59 vs edited ==--- 1.59/arch/ia64/kernel/acpi.c	Wed Jan 14 12:09:40 2004
+++ edited/arch/ia64/kernel/acpi.c	Tue Jan 20 15:32:41 2004
@@ -613,12 +613,20 @@
 }
 
 int
-acpi_irq_to_vector (u32 gsi)
+acpi_interrupt_to_irq (u32 interrupt, unsigned int *irq)
 {
-	if (has_8259 && gsi < 16)
-		return isa_irq_to_vector(gsi);
+	int vector;
 
-	return gsi_to_vector(gsi);
+	if (has_8259 && interrupt < 16)
+		vector = isa_irq_to_vector(interrupt);
+	else
+		vector = gsi_to_vector(interrupt);
+
+	if (vector < 0)
+		return -EINVAL;
+
+	*irq = vector;
+	return 0;
 }
 
 int

Re: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

On Tuesday 20 January 2004 10:04 pm, Brown, Len wrote:
> FYI, jun says this will conflict with recent vector interrupt updates in
> -mm, so we'll have to sort that out.

Yes, indeed.  Thanks for pointing this out.  Those updates make the
call look like this:

        irq = acpi_fadt.sci_int;
 
#if defined(CONFIG_IA64) || defined(CONFIG_PCI_USE_VECTOR)
        irq = acpi_irq_to_vector(irq);
        if (irq < 0) {
                printk(KERN_ERR PREFIX "SCI (ACPI interrupt %d) not registered\n",
	...
	acpi_irq_irq = irq;

It should be simple to resolve the conflict, but we have to decide

	(a) whether to keep the #ifdefs around the use.  I vote
	    for keeping them out of acpi.  In fact, the #ifdef can
	    be simply removed from the code in 2.6.1-mm5 if we
	    merely add the trivial acpi_irq_to_vector() to x86_64.
	    The ia64 and i386 versions already do the right thing.

	(b) whether "acpi_irq_to_vector" is the correct name.
	    I think it is misleading because it suggests that it
	    takes a Linux IRQ and returns a "vector", whatever
	    that is.  In fact, this function takes an ACPI
	    interrupt (which I think is either an ISA IRQ or
	    a GSI), and returns a Linux IRQ.  Hence my suggestion
	    to name it "acpi_interrupt_to_irq".

There's also something fishy in this area that neither the -mm5
code nor my patch addresses.  In the acpi_os_install_interrupt_handler()
fragment above, we do the "acpi interrupt->irq" conversion and save
the resulting irq in acpi_irq_irq.  The only place acpi_irq_irq is
used is in acpi_os_terminate(), where it is passed to acpi_os_remove_
interupt_handler(), where we apply the "acpi interrupt->irq" conversion
AGAIN.  This seems wrong.


On Tuesday 20 January 2004 4:07 pm, Bjorn Helgaas wrote:
> This patch against 2.6.1 tightens up some language and removes
> a couple IA64 #ifdefs:
> 
>  drivers/acpi/osl.c        |   26 ++++++++++++++------------
>  include/asm-i386/acpi.h   |    6 ++++++
>  include/asm-x86_64/acpi.h |    6 ++++++
>  include/asm-ia64/acpi.h   |    2 +-
>  arch/ia64/kernel/acpi.c   |   16 ++++++++++++----
>  5 files changed, 39 insertions(+), 17 deletions(-)
> 
> ACPI: Add acpi_interrupt_to_irq() interface.
> 
> ACPI interrupts and Linux IRQs need not be identical (though they are
> on i386 and x86_64), so introduce acpi_interrupt_to_irq(), clean up
> usage of "interrupt" and "irq", and remove IA64 #ifdefs.
> 
> === drivers/acpi/osl.c 1.43 vs edited ==> --- 1.43/drivers/acpi/osl.c	Mon Dec 29 14:37:24 2003
> +++ edited/drivers/acpi/osl.c	Tue Jan 20 15:36:23 2004
> @@ -240,23 +240,22 @@
>  }
>  
>  acpi_status
> -acpi_os_install_interrupt_handler(u32 irq, OSD_HANDLER handler, void *context)
> +acpi_os_install_interrupt_handler(u32 interrupt, OSD_HANDLER handler, void *context)
>  {
> +	unsigned int irq;
> +
>  	/*
> -	 * Ignore the irq from the core, and use the value in our copy of the
> +	 * Ignore the interrupt from the core, and use the value in our copy of the
>  	 * FADT. It may not be the same if an interrupt source override exists
>  	 * for the SCI.
>  	 */
> -	irq = acpi_fadt.sci_int;
> +	interrupt = acpi_fadt.sci_int;
>  
> -#ifdef CONFIG_IA64
> -	irq = acpi_irq_to_vector(irq);
> -	if (irq < 0) {
> +	if (acpi_interrupt_to_irq(interrupt, &irq)) {
>  		printk(KERN_ERR PREFIX "SCI (ACPI interrupt %d) not registered\n",
> -		       acpi_fadt.sci_int);
> +		       interrupt);
>  		return AE_OK;
>  	}
> -#endif
>  	acpi_irq_irq = irq;
>  	acpi_irq_handler = handler;
>  	acpi_irq_context = context;
> @@ -269,12 +268,15 @@
>  }
>  
>  acpi_status
> -acpi_os_remove_interrupt_handler(u32 irq, OSD_HANDLER handler)
> +acpi_os_remove_interrupt_handler(u32 interrupt, OSD_HANDLER handler)
>  {
> +	unsigned int irq;
> +
>  	if (acpi_irq_handler) {
> -#ifdef CONFIG_IA64
> -		irq = acpi_irq_to_vector(irq);
> -#endif
> +		if (acpi_interrupt_to_irq(interrupt, &irq)) {
> +			printk(KERN_ERR PREFIX "Can't remove ACPI interrupt handler\n");
> +			return AE_ERROR;
> +		}
>  		free_irq(irq, acpi_irq);
>  		acpi_irq_handler = NULL;
>  	}
> === include/asm-i386/acpi.h 1.9 vs edited ==> --- 1.9/include/asm-i386/acpi.h	Tue Sep 16 11:21:55 2003
> +++ edited/include/asm-i386/acpi.h	Tue Jan 20 15:33:24 2004
> @@ -139,6 +139,12 @@
>  
>  #endif
>  
> +static inline int acpi_interrupt_to_irq(u32 interrupt, unsigned int *irq)
> +{
> +	*irq = interrupt;
> +	return 0;
> +}
> +
>  #ifdef CONFIG_ACPI_SLEEP
>  
>  /* routines for saving/restoring kernel state */
> === include/asm-x86_64/acpi.h 1.3 vs edited ==> --- 1.3/include/asm-x86_64/acpi.h	Wed Dec 31 22:32:36 2003
> +++ edited/include/asm-x86_64/acpi.h	Tue Jan 20 15:33:54 2004
> @@ -120,6 +120,12 @@
>  
>  #endif /*CONFIG_ACPI_BOOT*/
>  
> +static inline int acpi_interrupt_to_irq(u32 interrupt, unsigned int *irq)
> +{
> +	*irq = interrupt;
> +	return 0;
> +}
> +
>  #ifdef CONFIG_ACPI_SLEEP
>  
>  /* routines for saving/restoring kernel state */
> === include/asm-ia64/acpi.h 1.13 vs edited ==> --- 1.13/include/asm-ia64/acpi.h	Mon Jan 12 00:20:13 2004
> +++ edited/include/asm-ia64/acpi.h	Tue Jan 20 15:35:01 2004
> @@ -91,7 +91,7 @@
>  const char *acpi_get_sysname (void);
>  int acpi_request_vector (u32 int_type);
>  int acpi_register_irq (u32 gsi, u32 polarity, u32 trigger);
> -int acpi_irq_to_vector (u32 irq);
> +int acpi_interrupt_to_irq (u32 interrupt, unsigned int *irq);
>  
>  #ifdef CONFIG_ACPI_NUMA
>  /* Proximity bitmap length; _PXM is at most 255 (8 bit)*/
> === arch/ia64/kernel/acpi.c 1.59 vs edited ==> --- 1.59/arch/ia64/kernel/acpi.c	Wed Jan 14 12:09:40 2004
> +++ edited/arch/ia64/kernel/acpi.c	Tue Jan 20 15:32:41 2004
> @@ -613,12 +613,20 @@
>  }
>  
>  int
> -acpi_irq_to_vector (u32 gsi)
> +acpi_interrupt_to_irq (u32 interrupt, unsigned int *irq)
>  {
> -	if (has_8259 && gsi < 16)
> -		return isa_irq_to_vector(gsi);
> +	int vector;
>  
> -	return gsi_to_vector(gsi);
> +	if (has_8259 && interrupt < 16)
> +		vector = isa_irq_to_vector(interrupt);
> +	else
> +		vector = gsi_to_vector(interrupt);
> +
> +	if (vector < 0)
> +		return -EINVAL;
> +
> +	*irq = vector;
> +	return 0;
>  }
>  
>  int
> 
> 
> 
> -------------------------------------------------------
> The SF.Net email is sponsored by EclipseCon 2004
> Premiere Conference on Open Tools Development and Integration
> See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
> http://www.eclipsecon.org/osdn
> _______________________________________________
> Acpi-devel mailing list
> Acpi-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/acpi-devel
> 
> 

-- 
Bjorn Helgaas - bjorn.helgaas at hp.com
Linux and Open Source Lab
Hewlett-Packard Company

Re: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

On Wednesday 21 January 2004 9:39 am, Bjorn Helgaas wrote:
> There's also something fishy in this area that neither the -mm5
> code nor my patch addresses.  In the acpi_os_install_interrupt_handler()
> fragment above, we do the "acpi interrupt->irq" conversion and save
> the resulting irq in acpi_irq_irq.  The only place acpi_irq_irq is
> used is in acpi_os_terminate(), where it is passed to acpi_os_remove_
> interupt_handler(), where we apply the "acpi interrupt->irq" conversion
> AGAIN.  This seems wrong.

OK, I think I understand what's wrong there.  acpi_irq_irq needs to
be the PRE-CONVERSION interrupt, like this:

    acpi_status
    acpi_os_install_interrupt_handler(u32 interrupt, OSD_HANDLER handler, void *context)
    {
	unsigned int irq;

	interrupt = acpi_fadt.sci_int;
	irq = acpi_irq_to_vector(interrupt); 
	...
        acpi_irq_irq = interrupt;
	...
	if (request_irq(irq, ...))

Then acpi_os_terminate() will pass the pre-conversion value to
acpi_os_remove_interrupt_handler(), which will apply acpi_irq_to_vector()
and everything will match.

I'll make a note to clean this up after the previous issues in the
area are straightened out.

Bjorn

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Nakajima, Jun]

One notes. In x86, although you assumed interrupt = IRQ, that's not the
case with so called "vector-based PCI interrupt handling" in support of
MSI (which is 2.6.1). With this, do_IRQ(irq, ...) will get the vector
number instead of IRQ, which is same as IPF. 

I think we can use the common code for that configuration at least in
ACPI. If you look at 2.6.1-mm5, for example, we also have
acpi_irq_to_vector(), which is doing the same thing as IPF does.

Thanks,
Jun
> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: Wednesday, January 21, 2004 12:18 PM
> To: acpi-devel@lists.sourceforge.net; linux-ia64@vger.kernel.org
> Cc: Brown, Len; Nakajima, Jun
> Subject: Re: [ACPI] [PATCH] add acpi_interrupt_to_irq
> 
> On Wednesday 21 January 2004 9:39 am, Bjorn Helgaas wrote:
> > There's also something fishy in this area that neither the -mm5
> > code nor my patch addresses.  In the
acpi_os_install_interrupt_handler()
> > fragment above, we do the "acpi interrupt->irq" conversion and save
> > the resulting irq in acpi_irq_irq.  The only place acpi_irq_irq is
> > used is in acpi_os_terminate(), where it is passed to
acpi_os_remove_
> > interupt_handler(), where we apply the "acpi interrupt->irq"
conversion
> > AGAIN.  This seems wrong.
> 
> OK, I think I understand what's wrong there.  acpi_irq_irq needs to
> be the PRE-CONVERSION interrupt, like this:
> 
>     acpi_status
>     acpi_os_install_interrupt_handler(u32 interrupt, OSD_HANDLER
handler,
> void *context)
>     {
> 	unsigned int irq;
> 
> 	interrupt = acpi_fadt.sci_int;
> 	irq = acpi_irq_to_vector(interrupt);
> 	...
>         acpi_irq_irq = interrupt;
> 	...
> 	if (request_irq(irq, ...))
> 
> Then acpi_os_terminate() will pass the pre-conversion value to
> acpi_os_remove_interrupt_handler(), which will apply
acpi_irq_to_vector()
> and everything will match.
> 
> I'll make a note to clean this up after the previous issues in the
> area are straightened out.
> 
> Bjorn
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

On Wednesday 21 January 2004 3:42 pm, Nakajima, Jun wrote:
> One notes. In x86, although you assumed interrupt = IRQ, that's not the
> case with so called "vector-based PCI interrupt handling" in support of
> MSI (which is 2.6.1). With this, do_IRQ(irq, ...) will get the vector
> number instead of IRQ, which is same as IPF. 

My patch was against 2.6.1, which doesn't include the "vector-based
PCI interrupt handling", so I didn't change the behavior on x86.

> I think we can use the common code for that configuration at least in
> ACPI. If you look at 2.6.1-mm5, for example, we also have
> acpi_irq_to_vector(), which is doing the same thing as IPF does.

I agree that in 2.6.1-mm5, which does have the vector-based stuff,
the x86 acpi_irq_to_vector() needs to be smarter.

The whole point of my patch was to make "acpi_irq_to_vector()"
be a standard platform interface, so each platform can do what
it needs, without cluttering ACPI with #ifdefs.

Any feedback on these questions from my previous mail:

	(a) do you want the #ifdefs in ACPI (as in 2.6.1-mm5),
	    or in the platform-specific code (as in my patch)?

	(b) is "acpi_interrupt_to_irq" a better name than
	    "acpi_irq_to_vector"?

	(c) is acpi_os_install_interrupt_handler() saving the
	    wrong value in acpi_irq_irq?

Bjorn

> > -----Original Message-----
> > From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-
> > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> > Sent: Wednesday, January 21, 2004 12:18 PM
> > To: acpi-devel@lists.sourceforge.net; linux-ia64@vger.kernel.org
> > Cc: Brown, Len; Nakajima, Jun
> > Subject: Re: [ACPI] [PATCH] add acpi_interrupt_to_irq
> > 
> > On Wednesday 21 January 2004 9:39 am, Bjorn Helgaas wrote:
> > > There's also something fishy in this area that neither the -mm5
> > > code nor my patch addresses.  In the
> acpi_os_install_interrupt_handler()
> > > fragment above, we do the "acpi interrupt->irq" conversion and save
> > > the resulting irq in acpi_irq_irq.  The only place acpi_irq_irq is
> > > used is in acpi_os_terminate(), where it is passed to
> acpi_os_remove_
> > > interupt_handler(), where we apply the "acpi interrupt->irq"
> conversion
> > > AGAIN.  This seems wrong.
> > 
> > OK, I think I understand what's wrong there.  acpi_irq_irq needs to
> > be the PRE-CONVERSION interrupt, like this:
> > 
> >     acpi_status
> >     acpi_os_install_interrupt_handler(u32 interrupt, OSD_HANDLER
> handler,
> > void *context)
> >     {
> > 	unsigned int irq;
> > 
> > 	interrupt = acpi_fadt.sci_int;
> > 	irq = acpi_irq_to_vector(interrupt);
> > 	...
> >         acpi_irq_irq = interrupt;
> > 	...
> > 	if (request_irq(irq, ...))
> > 
> > Then acpi_os_terminate() will pass the pre-conversion value to
> > acpi_os_remove_interrupt_handler(), which will apply
> acpi_irq_to_vector()
> > and everything will match.
> > 
> > I'll make a note to clean this up after the previous issues in the
> > area are straightened out.
> > 
> > Bjorn
> > 
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-ia64"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Bjorn Helgaas - bjorn.helgaas at hp.com
Linux and Open Source Lab
Hewlett-Packard Company

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Nakajima, Jun]

To be clear, the vector-based PCI interrupt support for MSI is in 2.6.1,
but the particular ACPI code in -mm was a bug fix that we added
recently.

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Wednesday, January 21, 2004 2:54 PM
> To: Nakajima, Jun; acpi-devel@lists.sourceforge.net; linux-
> 
> Any feedback on these questions from my previous mail:
> 
> 	(a) do you want the #ifdefs in ACPI (as in 2.6.1-mm5),
> 	    or in the platform-specific code (as in my patch)?

I would like to avoid #ifdefs even in -mm5.

> 
> 	(b) is "acpi_interrupt_to_irq" a better name than
> 	    "acpi_irq_to_vector"?

I don't know what people imagine by "interrupt", but to me it implies an
"event". 

If we have definitions of acpi_irq_to_vector(irq) for other
architectures (as you basically did), we can remove #ifdef CONFIG_IA64.
For x86, we can switch the definition using CONFIG_PCI_USE_VECTOR. 

> 
> 	(c) is acpi_os_install_interrupt_handler() saving the
> 	    wrong value in acpi_irq_irq?
> 

I think the original IA-64 code was wrong (so CONFIG_PCI_USE_VECTOR will
be wrong there, too).  The value of irq is covered to vector by
acpi_irq_to_vector(irq) in acpi_os_install_interrupt_handler(), then
this value is set to acpi_irq_irq. But
acpi_os_remove_interrupt_handler() is given acpi_irq_irq (i.e. vector),
then it again calls acpi_irq_to_vector() prior to calling
free_irq(irq,,).

Thanks,
Jun

Re: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

On Wednesday 21 January 2004 8:36 pm, Nakajima, Jun wrote:

> > 	(b) is "acpi_interrupt_to_irq" a better name than
> > 	    "acpi_irq_to_vector"?
> 
> I don't know what people imagine by "interrupt", but to me it implies an
> "event". 

Are you saying that you think "acpi_irq_to_vector" is the right name?
What does "vector" mean?  The return value of that function is in
fact a Linux IRQ, and is passed to request_irq() and free_irq().  So
I think the correct name is "acpi_SOMETHING_to_irq".  If you don't
like "interrupt", you can propose something else.  I just think it's
misleading for the name to contain "to_vector", when it's really
doing "to_irq".

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Brown, Len]

Bjorn,

> irq = acpi_irq_to_vector(irq);

Yeah, this name is totally bogus; and your comments are absolutely
right.  I thank you for taking the time to address it.

When I first saw this I said to myself, "hmm, shouldn't it be
acpi_irq_to_irq()?", nah, that seems even more stupid!";-)

Looks like this is only used for the SCI listed in the FADT.  So maybe
we should just call it something like acpi_fadt_sci_to_irq()?

Thanks,
-Len

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com] 
> Sent: Thursday, January 22, 2004 11:38 AM
> To: Nakajima, Jun; acpi-devel@lists.sourceforge.net; 
> linux-ia64@vger.kernel.org
> Cc: Brown, Len
> Subject: Re: [ACPI] [PATCH] add acpi_interrupt_to_irq
> 
> 
> On Wednesday 21 January 2004 8:36 pm, Nakajima, Jun wrote:
> 
> > > 	(b) is "acpi_interrupt_to_irq" a better name than
> > > 	    "acpi_irq_to_vector"?
> > 
> > I don't know what people imagine by "interrupt", but to me 
> it implies an
> > "event". 
> 
> Are you saying that you think "acpi_irq_to_vector" is the right name?
> What does "vector" mean?  The return value of that function is in
> fact a Linux IRQ, and is passed to request_irq() and free_irq().  So
> I think the correct name is "acpi_SOMETHING_to_irq".  If you don't
> like "interrupt", you can propose something else.  I just think it's
> misleading for the name to contain "to_vector", when it's really
> doing "to_irq".
> 
> 
> 

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[David Mosberger]

>>>>> On Thu, 22 Jan 2004 12:41:10 -0500, "Brown, Len" <len.brown@intel.com> said:

  Len> Bjorn,
  >> irq = acpi_irq_to_vector(irq);

  Len> Yeah, this name is totally bogus; and your comments are
  Len> absolutely right.  I thank you for taking the time to address
  Len> it.

  Len> When I first saw this I said to myself, "hmm, shouldn't it be
  Len> acpi_irq_to_irq()?", nah, that seems even more stupid!";-)

  Len> Looks like this is only used for the SCI listed in the FADT.
  Len> So maybe we should just call it something like
  Len> acpi_fadt_sci_to_irq()?

It would be good to come up with a consistent naming scheme.  I/we
tried to do that in the ia64/iosapic.c, but it might need some
updating/revising with recent changes (such as MSIs).  Perhaps "gsi"
is also a contender for the ACPI interrupts.

	--david

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Nakajima, Jun]

> What does "vector" mean?  
For MSI, vector means the (external) interrupt vector, i.e. the index in
the IDT for MSI on x86. So acpi_irq_to_vector() is correct in that case.
ACPI looks at IRQ or GSI (Global system interrupt) vector (yes, it's
confusing). SOMETHING should be irq or gsi, as David suggested.

Since MSI does not require IRQ (but external interrupt vector), the way
we did for x86 was to use the vector to unify IRQ and vector. So
request_irq() actually gets the interrupt vector number, instead of irq.
That's the reason I preferred acpi_irq_to_vector() in that code with MSI
configured.

Jun
> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Thursday, January 22, 2004 8:38 AM
> To: Nakajima, Jun; acpi-devel@lists.sourceforge.net; linux-
> ia64@vger.kernel.org
> Cc: Brown, Len
> Subject: Re: [ACPI] [PATCH] add acpi_interrupt_to_irq
> 
> On Wednesday 21 January 2004 8:36 pm, Nakajima, Jun wrote:
> 
> > > 	(b) is "acpi_interrupt_to_irq" a better name than
> > > 	    "acpi_irq_to_vector"?
> >
> > I don't know what people imagine by "interrupt", but to me it
implies an
> > "event".
> 
> Are you saying that you think "acpi_irq_to_vector" is the right name?
> What does "vector" mean?  The return value of that function is in
> fact a Linux IRQ, and is passed to request_irq() and free_irq().  So
> I think the correct name is "acpi_SOMETHING_to_irq".  If you don't
> like "interrupt", you can propose something else.  I just think it's
> misleading for the name to contain "to_vector", when it's really
> doing "to_irq".
> 

Re: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Bjorn Helgaas]

On Thursday 22 January 2004 8:36 pm, Nakajima, Jun wrote:
> > What does "vector" mean?  
> For MSI, vector means the (external) interrupt vector, i.e. the index in
> the IDT for MSI on x86. So acpi_irq_to_vector() is correct in that case.
> ACPI looks at IRQ or GSI (Global system interrupt) vector (yes, it's
> confusing). SOMETHING should be irq or gsi, as David suggested.
> 
> Since MSI does not require IRQ (but external interrupt vector), the way
> we did for x86 was to use the vector to unify IRQ and vector. So
> request_irq() actually gets the interrupt vector number, instead of irq.
> That's the reason I preferred acpi_irq_to_vector() in that code with MSI
> configured.

Sorry to drag this out even longer...  I promise I'll shut up
after this :-)

But this business about request_irq() getting an MSI interrupt vector
number, not an irq, is just a detail of the MSI and architecture
implementation.  Surely ACPI should just use the abstract Linux
interrupt interface (request_irq(), free_irq(), etc), which uses
the "irq" terminology, and should remain ignorant of whether MSI
is even present.

Bjorn

RE: [ACPI] [PATCH] add acpi_interrupt_to_irq

[Nakajima, Jun]

Not a problem to me :-) So if we remove the #ifdef (i.e. both
CONFIG_IA64 and CONFIG_PCI_USE_VECTOR) there, I think acpi_gsi_to_irq is
reasonable there. For x86 we'll switch to (the old) acpi_irq_to_vector
if CONFIG_PCI_USE_VECTOR is configured, and don't do anything for the
usual case. But I don't expect Len to replace every irq in ACPI with
gsi, though.

BTW, we need to think how we support MSI (and PCI Express) for IPF; we
already posted a PCI Express patch for x86. Today's IRQ abstraction on
IPF might have some advantage for supporting MSI, which is required for
PCI Express.

Jun

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Friday, January 23, 2004 8:36 AM
> To: Nakajima, Jun; acpi-devel@lists.sourceforge.net; linux-
> ia64@vger.kernel.org
> Cc: Brown, Len
> Subject: Re: [ACPI] [PATCH] add acpi_interrupt_to_irq
> 
> On Thursday 22 January 2004 8:36 pm, Nakajima, Jun wrote:
> > > What does "vector" mean?
> > For MSI, vector means the (external) interrupt vector, i.e. the
index in
> > the IDT for MSI on x86. So acpi_irq_to_vector() is correct in that
case.
> > ACPI looks at IRQ or GSI (Global system interrupt) vector (yes, it's
> > confusing). SOMETHING should be irq or gsi, as David suggested.
> >
> > Since MSI does not require IRQ (but external interrupt vector), the
way
> > we did for x86 was to use the vector to unify IRQ and vector. So
> > request_irq() actually gets the interrupt vector number, instead of
irq.
> > That's the reason I preferred acpi_irq_to_vector() in that code with
MSI
> > configured.
> 
> Sorry to drag this out even longer...  I promise I'll shut up
> after this :-)
> 
> But this business about request_irq() getting an MSI interrupt vector
> number, not an irq, is just a detail of the MSI and architecture
> implementation.  Surely ACPI should just use the abstract Linux
> interrupt interface (request_irq(), free_irq(), etc), which uses
> the "irq" terminology, and should remain ignorant of whether MSI
> is even present.
> 
> Bjorn