Skip to contentSkip to conversation

deal with lack of acpi prt entries gracefully

PATCH In review Jesse Barnes 3 participants 13 messages Sep 9, 2003 – Sep 11, 2003

Subsystems (2)
ACPI PCI
lore
Share & tools
lore

Jesse Barnes

author
#0 lore raw

PATCH 1/1 deal with lack of acpi prt entries gracefully

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Jesse

drivers/acpi/pci_irq.c +3 -0
diff -Nru a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
@@ -71,6 +71,9 @@
 
 	ACPI_FUNCTION_TRACE("acpi_pci_irq_find_prt_entry");
 
+	if (!acpi_prt.count)
+		return_PTR(NULL);
+
 	/*
 	 * Parse through all PRT entries looking for a match on the specified
 	 * PCI device's segment, bus, device, and pin (don't care about func).
#1 lore raw
Quoted 2 lines · Jesse Barnes

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Ah, this is a patch against the vanilla kernel.. This is unfortunately incompatible with my recent ACPI patches.

author
#2 lore raw
Quoted 6 lines · Andrew de Quincey
Quoted 2 lines · Jesse Barnes

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Ah, this is a patch against the vanilla kernel.. This is unfortunately incompatible with my recent ACPI patches.

Maybe you could include it in your patch then? I noticed that your patch changes some of the IRQ routing code...

Thanks,
Jesse

#3 lore raw
Quoted 10 lines · Jesse Barnes
Quoted 6 lines · Andrew de Quincey
Quoted 2 lines · Jesse Barnes

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Ah, this is a patch against the vanilla kernel.. This is unfortunately incompatible with my recent ACPI patches.

Maybe you could include it in your patch then? I noticed that your patch changes some of the IRQ routing code...

I'm not sure it is still needed or not. The patch makes a lot of changes as to how the acpi_prt list is generated. What triggers the problem exactly, so I can test?

author
#4 lore raw
Quoted 9 lines · Andrew de Quincey
Quoted 8 lines · Jesse Barnes
Quoted 6 lines · Andrew de Quincey
Quoted 2 lines · Jesse Barnes

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Ah, this is a patch against the vanilla kernel.. This is unfortunately incompatible with my recent ACPI patches.

Quoted 6 lines
Quoted 2 lines

Maybe you could include it in your patch then? I noticed that your patch changes some of the IRQ routing code...

I'm not sure it is still needed or not. The patch makes a lot of changes as to how the acpi_prt list is generated. What triggers the problem exactly, so I can test?

An SGI Altix system that only supports part of the ACPI spec :). Our PROM currently doesn't generate any MADT entries other than CPUs (I'm not even sure how we could add them since we use our own interrupt controller, not an IOAPIC or IOSAPIC) and lacks an ACPI namespace, so we're missing all sorts of stuff.

Jesse

#5 lore raw
Quoted 17 lines · Jesse Barnes
Quoted 9 lines · Andrew de Quincey
Quoted 8 lines · Jesse Barnes
Quoted 6 lines · Andrew de Quincey
Quoted 2 lines · Jesse Barnes

Instead of going into an infinite loop because the list isn't setup yet, just return NULL if there are no prt entries.

Ah, this is a patch against the vanilla kernel.. This is unfortunately incompatible with my recent ACPI patches.

Quoted 6 lines
Quoted 2 lines

Maybe you could include it in your patch then? I noticed that your patch changes some of the IRQ routing code...

I'm not sure it is still needed or not. The patch makes a lot of changes as to how the acpi_prt list is generated. What triggers the problem exactly, so I can test?

Quoted 5 lines

An SGI Altix system that only supports part of the ACPI spec :). Our PROM currently doesn't generate any MADT entries other than CPUs (I'm not even sure how we could add them since we use our own interrupt controller, not an IOAPIC or IOSAPIC) and lacks an ACPI namespace, so we're missing all sorts of stuff.

So, exactly as your patch did, you just want it to drop back if there were no PCI routing entries found by ACPI... sounds sensible enough.

Can you confirm I have this right?

author
#6 lore raw
Quoted 4 lines · Andrew de Quincey

So, exactly as your patch did, you just want it to drop back if there were no PCI routing entries found by ACPI... sounds sensible enough.

Can you confirm I have this right?

Yep, that's it. The code should do that, but we get there before the list has been initialized, so we just hang.

Thanks,
Jesse

#7 lore raw
Quoted 8 lines · Jesse Barnes
Quoted 4 lines · Andrew de Quincey

So, exactly as your patch did, you just want it to drop back if there were no PCI routing entries found by ACPI... sounds sensible enough.

Can you confirm I have this right?

Yep, that's it. The code should do that, but we get there before the list has been initialized, so we just hang.

I'm not sure if this is automatically fixed or not yet.

With the new patch:

1) If ACPI fails to parse a table, it disables ACPI, and so disables any attempt to use ACPI for PRT routing.

2) If ACPI is enabled, and enters the function you patched, code further in checks if the routing tables have any entries. If not, it rejects the attempt.

Quoted 1 line

From your patch, I get the impression (1) is what you were patching for.. am I

right? In that case, there shouldn't be a problem.

author
#8 lore raw
Quoted 10 lines · Andrew de Quincey
Quoted 8 lines · Jesse Barnes
Quoted 4 lines · Andrew de Quincey

So, exactly as your patch did, you just want it to drop back if there were no PCI routing entries found by ACPI... sounds sensible enough.

Can you confirm I have this right?

Yep, that's it. The code should do that, but we get there before the list has been initialized, so we just hang.

Quoted 6 lines

I'm not sure if this is automatically fixed or not yet.

With the new patch:

1) If ACPI fails to parse a table, it disables ACPI, and so disables any attempt to use ACPI for PRT routing.

That might work, though I'll be using the ACPI namespace to drive PCI discovery soon (hacking the PROM now). Maybe I should add some MADT and _PRT entries while I'm at it? The problem is that we don't support IOAPIC or IOSAPIC interrupt models/hw registers.

Quoted 3 lines

2) If ACPI is enabled, and enters the function you patched, code further in checks if the routing tables have any entries. If not, it rejects the attempt.

That would work I guess.

Quoted 2 lines

From your patch, I get the impression (1) is what you were patching for.. am I right? In that case, there shouldn't be a problem.

We also use ACPI tables SLIT and SRAT for memory/proximity detection, and fill in the proper FADT fields.

Thanks,
Jesse

#9 lore raw
Quoted 15 lines · Jesse Barnes
Quoted 14 lines · Andrew de Quincey
Quoted 8 lines · Jesse Barnes
Quoted 4 lines · Andrew de Quincey

So, exactly as your patch did, you just want it to drop back if there were no PCI routing entries found by ACPI... sounds sensible enough.

Can you confirm I have this right?

Yep, that's it. The code should do that, but we get there before the list has been initialized, so we just hang.

I'm not sure if this is automatically fixed or not yet.

With the new patch:

Quoted 7 lines
Quoted 2 lines

1) If ACPI fails to parse a table, it disables ACPI, and so disables any attempt to use ACPI for PRT routing.

That might work, though I'll be using the ACPI namespace to drive PCI discovery soon (hacking the PROM now). Maybe I should add some MADT and _PRT entries while I'm at it? The problem is that we don't support IOAPIC or IOSAPIC interrupt models/hw registers.

Which base architecture do you use? x86 and x86_64 ACPI now both support PIC based interrupt models.. as thats the only other option AFAIK (It tries IOAPIC first, then if that fails, it drops back to trying PIC mode).

Quoted 5 lines
Quoted 3 lines

2) If ACPI is enabled, and enters the function you patched, code further in checks if the routing tables have any entries. If not, it rejects the attempt.

That would work I guess.

Cool, well if it doesn't work, at least we know exactly what to fix.

author
#10 lore raw
Quoted 8 lines · Andrew de Quincey
Quoted 4 lines

That might work, though I'll be using the ACPI namespace to drive PCI discovery soon (hacking the PROM now). Maybe I should add some MADT and _PRT entries while I'm at it? The problem is that we don't support IOAPIC or IOSAPIC interrupt models/hw registers.

Which base architecture do you use? x86 and x86_64 ACPI now both support PIC based interrupt models.. as thats the only other option AFAIK (It tries IOAPIC first, then if that fails, it drops back to trying PIC mode).

None of the above. We have our own NUMAlink based interrupt protocol model.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

2) If ACPI is enabled, and enters the function you patched, code further in checks if the routing tables have any entries. If not, it rejects the attempt.

That would work I guess.

Cool, well if it doesn't work, at least we know exactly what to fix.

Yeah, I found the problem pretty quickly last time, so I'm ok with retesting once your patch goes in.

Thanks,
Jesse

#11 lore raw
Quoted 6 lines · Jesse Barnes
Quoted 5 lines · Andrew de Quincey
Quoted 4 lines

That might work, though I'll be using the ACPI namespace to drive PCI discovery soon (hacking the PROM now). Maybe I should add some MADT and _PRT entries while I'm at it? The problem is that we don't support IOAPIC or IOSAPIC interrupt models/hw registers.

Quoted 7 lines
Quoted 4 lines

Which base architecture do you use? x86 and x86_64 ACPI now both support PIC based interrupt models.. as thats the only other option AFAIK (It tries IOAPIC first, then if that fails, it drops back to trying PIC mode).

None of the above. We have our own NUMAlink based interrupt protocol model.

Oooer! Hmm, the existing code would probably NOT like having _PRT entries for a model it doesn't know about.... you could add support for it fairly easily though I suppose...

author
#12 lore raw
Quoted 6 lines · Andrew de Quincey
Quoted 2 lines

None of the above. We have our own NUMAlink based interrupt protocol model.

Oooer! Hmm, the existing code would probably NOT like having _PRT entries for a model it doesn't know about.... you could add support for it fairly easily though I suppose...

Yeah, that's what Andy suggested too. I guess I have to use one of the reserved fields and try to get the ACPI spec updated.

Thanks,
Jesse