Instead of going into an infinite loop because the list isn't setup yet,
just return NULL if there are no prt entries.
Jesse
diff -Nru a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
--- a/drivers/acpi/pci_irq.c Tue Sep 9 10:24:14 2003
+++ b/drivers/acpi/pci_irq.c Tue Sep 9 10:24:14 2003
@@ -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).
On Tuesday 09 September 2003 21:13, Jesse Barnes wrote:
> 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.
On Tue, Sep 09, 2003 at 09:43:58PM +0100, Andrew de Quincey wrote:
> On Tuesday 09 September 2003 21:13, Jesse Barnes wrote:
> > 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
On Tuesday 09 September 2003 22:17, Jesse Barnes wrote:
> On Tue, Sep 09, 2003 at 09:43:58PM +0100, Andrew de Quincey wrote:
> > On Tuesday 09 September 2003 21:13, Jesse Barnes wrote:
> > > 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?
On Tue, Sep 09, 2003 at 10:38:26PM +0100, Andrew de Quincey wrote:
> On Tuesday 09 September 2003 22:17, Jesse Barnes wrote:
> > On Tue, Sep 09, 2003 at 09:43:58PM +0100, Andrew de Quincey wrote:
> > > On Tuesday 09 September 2003 21:13, Jesse Barnes wrote:
> > > > 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?
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
On Wed, Sep 10, 2003 at 10:30:29PM +0100, Andrew de Quincey wrote:
> 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
On Tuesday 09 Sep 2003 11:01 pm, Jesse Barnes wrote:
> On Tue, Sep 09, 2003 at 10:38:26PM +0100, Andrew de Quincey wrote:
> > On Tuesday 09 September 2003 22:17, Jesse Barnes wrote:
> > > On Tue, Sep 09, 2003 at 09:43:58PM +0100, Andrew de Quincey wrote:
> > > > On Tuesday 09 September 2003 21:13, Jesse Barnes wrote:
> > > > > 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?
>
> 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?
On Wednesday 10 Sep 2003 10:38 pm, Jesse Barnes wrote:
> On Wed, Sep 10, 2003 at 10:30:29PM +0100, Andrew de Quincey wrote:
> > 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.
>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.
On Thu, Sep 11, 2003 at 09:40:08PM +0100, Andrew de Quincey wrote:
> On Wednesday 10 Sep 2003 10:38 pm, Jesse Barnes wrote:
> > On Wed, Sep 10, 2003 at 10:30:29PM +0100, Andrew de Quincey wrote:
> > > 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.
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.
> 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.
> 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
On Thu, Sep 11, 2003 at 10:13:13PM +0100, Andrew de Quincey wrote:
> > 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.
> > > 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
On Thursday 11 Sep 2003 9:53 pm, Jesse Barnes wrote:
> On Thu, Sep 11, 2003 at 09:40:08PM +0100, Andrew de Quincey wrote:
> > On Wednesday 10 Sep 2003 10:38 pm, Jesse Barnes wrote:
> > > On Wed, Sep 10, 2003 at 10:30:29PM +0100, Andrew de Quincey wrote:
> > > > 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.
>
> 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).
> > 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.
On Thursday 11 Sep 2003 10:20 pm, Jesse Barnes wrote:
> On Thu, Sep 11, 2003 at 10:13:13PM +0100, Andrew de Quincey wrote:
> > > 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.
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...
On Thu, Sep 11, 2003 at 11:00:30PM +0100, Andrew de Quincey wrote:
> > 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
> -----Original Message-----
> From: Jesse Barnes [mailto:[email protected]]
> Sent: Thursday, September 11, 2003 6:05 PM
> To: Andrew de Quincey
> Cc: Grover, Andrew; [email protected]
> Subject: Re: [PATCH] deal with lack of acpi prt entries gracefully
>
>
> On Thu, Sep 11, 2003 at 11:00:30PM +0100, Andrew de Quincey wrote:
> > > 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
Even if this exotic box shouldn't be running this flavor of the code,
the inifinite loop part struck me as less than bomb proof;-)
So I pulled the return on count==0 check into the ACPI patch.
Thanks,
-Len