bug w/ shared interrupts

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

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:

  [email protected]|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

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;

_

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
@@ -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;

_

Hi,

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

Thanks,
Kenji Kaneshige

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

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