cvs commit: src/sys/dev/acpica acpi_pci_link.c

[Nate Lawson]

John Baldwin wrote:
> On Tuesday 30 November 2004 01:55 am, Nate Lawson wrote:
> 
>>njl         2004-11-30 06:55:43 UTC
>>
>>  FreeBSD src repository
>>
>>  Modified files:
>>    sys/dev/acpica       acpi_pci_link.c
>>  Log:
>>  Make sure the link array is big enough to hold both _CRS and _PRS
>>  resource lists.  It used to be sized based only on _CRS, hence _PRS could
>>  perform an out-of-bounds access if it was larger (i.e., when there are
>>  dependent functions).  Add asserts to detect this case.  Note, this is
>>  only a temporary fix and I believe _PRS and _CRS should have separate
>>  arrays.
>>
>>  Also, fix a typo where the wrong irq was being check for the APIC case.
>>
>>  Submitted by:   tegge
> 
> 
> The real fix is not separate arrays per se but real dependent function 
> support.

I agree and that's what the XXX comment says in my commit.  However, 
they may need to be different sizes if _CRS gives us an irq resource and 
_PRS gives an extended irq resource, not just for DPFs.  This could 
occur if the link was boot-configured in PIC mode and then we went to 
APIC mode.  I think it's best to handle each separately for this case.

> We need a resource management concept of resource sets, so that 
> each set of dependent resources with a dependent start/end pair are 
> considered a set, and a driver can query 1) how many sets there are and 2) 
> set the current set of resources.

For now, we should just skip dependent functions (both start and end). 
For PCI irq routing, I've never seen them used for anything.  I've only 
seen them used for multiple resources in the case of _PRS for ACPI 
devices like lpt and sio, like this for sio:

                     Name (_PRS, ResourceTemplate ()
                     {
                         StartDependentFn (0x00, 0x00)
                         {
                             IO (Decode16, 0x03F8, 0x03F8, 0x01, 0x08)
                             IRQNoFlags () {4}
                         }
                         StartDependentFn (0x01, 0x00)
                         {
                             IO (Decode16, 0x02F8, 0x02F8, 0x01, 0x08)
                             IRQNoFlags () {3}
                         }
                         etc.

> Trying to use more resources than are in 
> _CRS is probably a bug and might break _SRS on these machines.  Hmm, also, the 
> code assumes when doing _SRS that the link indices are the resource indices, 
> so you just broke _SRS for any link devices that have a non-IRQ resource. :(

I see.  Sorry, I was concerned about data corruption and so didn't go 
through a longer review/testing process.

We should only set irq or extended irq resources in _SRS, no dependent 
functions.  In every _SRS function I've examined (for both ACPI devices 
and irq links), they examine the resource passed to them at fixed 
offsets so using a DPF would screw this up.

> What I think we should do for now is to only include the first resource set as 
> the current resources for an ACPI device that has multiple resource sets 
> instead of just including all the resources.  I'll try to work on this.

Yes, I think you're saying the same thing as I said above, which is to 
have a check for only one DPF set (if any) and fail if there is more 
than one.  We should handle this for both _PRS and _CRS (consuming DPFs) 
and never generate them for _SRS.

-- 
Nate