[PATCH] bug w/ shared interrupts

[PATCH] bug w/ shared interrupts

[Alex Williamson]

   I just ran into a bug introduced by cset 1.1371.519.37.  The scenario
is a builtin driver is up and running happily.  A module loads for a
devices that happens to share the same interrupt vector, in this case a
network driver.  The module calls pci_enable_device() as it should,
which eventually lands in iosapic_enable_intr().  We then proceed to
mask the interrupt and kill the device that's already running.  As a
bonus, request_interrupt() doesn't fix the problem because we only call
the startup for the interrupt handler on the first action attached to
the interrupt.

   I think the best way out of this is simply to detect when an action
is already attached to a vector and leave it alone.  This also prevents
interrupts from moving to other cpus (on boxes w/o irq redirection) for
no good reason.  Thanks,

	Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab

=== arch/ia64/kernel/iosapic.c 1.39 vs edited ==--- 1.39/arch/ia64/kernel/iosapic.c	Wed Apr 21 15:26:09 2004
+++ edited/arch/ia64/kernel/iosapic.c	Sun Apr 25 21:13:33 2004
@@ -648,6 +648,16 @@
 iosapic_enable_intr (unsigned int vector)
 {
 	unsigned int dest;
+	irq_desc_t *desc;
+
+	/*
+	 * In the case of a shared interrupt, do not re-route the vector, and
+	 * especially do not mask a running interrupt (startup will not get
+	 * called for a shared interrupt).
+	 */
+	desc = irq_descp(vector);
+	if (desc->action)
+		return;
 
 #ifdef CONFIG_SMP
 	/*

Re: [PATCH] bug w/ shared interrupts

[David Mosberger]

>>>>> On Mon, 26 Apr 2004 08:29:00 -0600, Alex Williamson <alex.williamson@hp.com> said:

  Alex>    I just ran into a bug introduced by cset 1.1371.519.37.

Note that cset numbers are not globally unique.  Since you didn't specify
which repository you used, I'm not sure which cset you're referring to.
To get a globally unique cset id, you can use:

  $ bk changes -k -r1.1371.519.37

I believe you're referring to:

  kaneshige.kenji@jp.fujitsu.com|ChangeSet|20040412205820|04778

Correct?

  Alex> The scenario is a builtin driver is up and running happily.  A
  Alex> module loads for a devices that happens to share the same
  Alex> interrupt vector, in this case a network driver.  The module
  Alex> calls pci_enable_device() as it should, which eventually lands
  Alex> in iosapic_enable_intr().  We then proceed to mask the
  Alex> interrupt and kill the device that's already running.  As a
  Alex> bonus, request_interrupt() doesn't fix the problem because we
  Alex> only call the startup for the interrupt handler on the first
  Alex> action attached to the interrupt.

  Alex>    I think the best way out of this is simply to detect when
  Alex> an action is already attached to a vector and leave it alone.
  Alex> This also prevents interrupts from moving to other cpus (on
  Alex> boxes w/o irq redirection) for no good reason.

Sounds reasonable to me.  I do think that iosapic_enable_intr() is
misnamed now that it actually leaves the interrupt _masked_.  What
it's really doing now is that it asserts that the I/O SAPIC routine is
programmed to a correct/working value.  It would be good to fix that
when changing the corresponding code in drivers/acpi/pci_irq.c.  IIRC,
Bjorn already has patches which touch on that file.  If so, could you
do a rename at the same time, Bjorn?

	--david

RE: [PATCH] bug w/ shared interrupts

[Kenji Kaneshige]

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

Hi,

I don't know whether you are referring to my patch. But if so,
please try a following patch. The patch for sharing vector I sent
before contains a bug and I think this might cause a similar problem.

Thanks,
Kenji Kaneshige


This is a patch to fix the following bug:

Register_intr() changes pin->low32 even when it overrides the
interrupt info. This destroys the value of pin->low32 which is already
set by set_rte() or other staffs.

This patch is against linux-2.6.6-rc2 with share_vector.patch applied.


---

 linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c |    1 -
 1 files changed, 1 deletion(-)

diff -puN arch/ia64/kernel/iosapic.c~share_vector_bugfix
arch/ia64/kernel/iosapic.c
--- linux-2.6.6-rc2/arch/ia64/kernel/iosapic.c~share_vector_bugfix
2004-04-27 18:12:56.000000000 +0900
+++ linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c	2004-04-27
18:12:56.000000000 +0900
@@ -605,7 +605,6 @@ register_intr (unsigned int gsi, int vec
 		pin->polarity = polarity;
 		pin->addr     = iosapic_address;
 		pin->gsi_base = gsi_base;
-		pin->low32    = IOSAPIC_MASK;
 		iosapic_intr_info[vector].dmode   = delivery;
 		iosapic_intr_info[vector].trigger = trigger;
 		iosapic_intr_info[vector].type    = type;

_

[-- Attachment #2: share_vector_bugfix.patch --]
[-- Type: application/octet-stream, Size: 1025 bytes --]


This is a patch to fix the following bug:

Register_intr() changes pin->low32 even when it overrides the
interrupt info. This destroys the value of pin->low32 which is already
set by set_rte() or other staffs.

This patch is against linux-2.6.6-rc2 with share_vector.patch applied.


---

 linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c |    1 -
 1 files changed, 1 deletion(-)

diff -puN arch/ia64/kernel/iosapic.c~share_vector_bugfix arch/ia64/kernel/iosapic.c
--- linux-2.6.6-rc2/arch/ia64/kernel/iosapic.c~share_vector_bugfix	2004-04-27 18:12:56.000000000 +0900
+++ linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c	2004-04-27 18:12:56.000000000 +0900
@@ -605,7 +605,6 @@ register_intr (unsigned int gsi, int vec
 		pin->polarity = polarity;
 		pin->addr     = iosapic_address;
 		pin->gsi_base = gsi_base;
-		pin->low32    = IOSAPIC_MASK;
 		iosapic_intr_info[vector].dmode   = delivery;
 		iosapic_intr_info[vector].trigger = trigger;
 		iosapic_intr_info[vector].type    = type;

_

RE: [PATCH] bug w/ shared interrupts

[Kenji Kaneshige]

Hi,

I misunderstood what you were saying.
Please ignore my previous mail.

Thanks,
Kenji Kaneshige


> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of Kenji Kaneshige
> Sent: Tuesday, April 27, 2004 8:57 PM
> To: Alex Williamson; linux-ia64
> Subject: RE: [PATCH] bug w/ shared interrupts
> 
> 
> Hi,
> 
> I don't know whether you are referring to my patch. But if so,
> please try a following patch. The patch for sharing vector I sent
> before contains a bug and I think this might cause a similar problem.
> 
> Thanks,
> Kenji Kaneshige
> 
> 
> This is a patch to fix the following bug:
> 
> Register_intr() changes pin->low32 even when it overrides the
> interrupt info. This destroys the value of pin->low32 which is already
> set by set_rte() or other staffs.
> 
> This patch is against linux-2.6.6-rc2 with share_vector.patch applied.
> 
> 
> ---
> 
>  linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c |    1 -
>  1 files changed, 1 deletion(-)
> 
> diff -puN arch/ia64/kernel/iosapic.c~share_vector_bugfix
> arch/ia64/kernel/iosapic.c
> --- linux-2.6.6-rc2/arch/ia64/kernel/iosapic.c~share_vector_bugfix
> 2004-04-27 18:12:56.000000000 +0900
> +++ linux-2.6.6-rc2-kanesige/arch/ia64/kernel/iosapic.c	2004-04-27
> 18:12:56.000000000 +0900
> @@ -605,7 +605,6 @@ register_intr (unsigned int gsi, int vec
>  		pin->polarity = polarity;
>  		pin->addr     = iosapic_address;
>  		pin->gsi_base = gsi_base;
> -		pin->low32    = IOSAPIC_MASK;
>  		iosapic_intr_info[vector].dmode   = delivery;
>  		iosapic_intr_info[vector].trigger = trigger;
>  		iosapic_intr_info[vector].type    = type;
> 
> _
> 

Re: [PATCH] bug w/ shared interrupts

[Bjorn Helgaas]

On Monday 26 April 2004 3:12 pm, David Mosberger wrote:
> I do think that iosapic_enable_intr() is
> misnamed now that it actually leaves the interrupt _masked_.  What
> it's really doing now is that it asserts that the I/O SAPIC routine is
> programmed to a correct/working value.  It would be good to fix that
> when changing the corresponding code in drivers/acpi/pci_irq.c.  IIRC,
> Bjorn already has patches which touch on that file.  If so, could you
> do a rename at the same time, Bjorn?

Sure, I'll take a look at that.  I think the only interface between
acpi/pci_irq.c and the architecture code will be acpi_register_gsi(),
though, so renaming iosapic_enable_intr() should be independent
of those changes.

Bjorn

Re: [PATCH] bug w/ shared interrupts

[Alex Williamson]

On Mon, 2004-04-26 at 15:12, David Mosberger wrote:
> >>>>> On Mon, 26 Apr 2004 08:29:00 -0600, Alex Williamson said:
> 
>   Alex>    I just ran into a bug introduced by cset 1.1371.519.37.
> 
> Note that cset numbers are not globally unique.  Since you didn't specify
> which repository you used, I'm not sure which cset you're referring to.
> To get a globally unique cset id, you can use:
> 
>   $ bk changes -k -r1.1371.519.37
> 


   Sorry, seems like I've made this mistake before.  I'm using the
linux-2.5 repository:

$ bk changes -k -r1.1371.519.37:

(email munged)
kaneshige.kenji at jp dot fujitsu dot com|ChangeSet|20040311072739|26139

http://linux.bkbits.net:8080/linux-2.5/cset@405014ebFyD871H8goGX9hQG7Un99g?nav=index.html|src/|src/arch|src/arch/ia64|src/arch/ia64/kernel|related/arch/ia64/kernel/iosapic.c

Thanks,

	Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab