x86/pci: do assign root bus res if _CRS is used
Subsystems (6)
PATCH 1/1 x86/pci: do assign root bus res if _CRS is used
it wil be overwriten later if _CRS is used, so don't bother to set it.
[ Impact: cleanup ]
--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/x86/pci/amd_bus.c +4 -0
@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;
Jesse Barnes
AppliedQuoted 4 lines · On Mon, 20 Apr 2009 18:35:40 -0700 Yinghai Lu <[email protected]> wrote:
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Applied, thanks.
A general comment on your patches though: please spent a few more minutes coming up with readable & useful summaries & changelogs. Most of the time when applying your patches (which are generally fine technically) I have to delete the whole changelog and come up with a new one based on reading the sources and then your patch. It would be nice if I didn't have to. Grammar and spelling mistakes are fine (I usually catch those) but when the logic of the changelog is all wrong, or it doesn't describe what it's doing and why, it makes things much more difficult.
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 15 lines · Yinghai Lu
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Signed-off-by Yinghai Lu <[email protected]>--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)Index:
linux-2.6/arch/x86/pci/amd_bus.c
===================================================================
arch/x86/pci/amd_bus.c void x86_pci_root_bus_res_quirks(struct · line 109
Quoted 10 lines
arch/x86/pci/amd_bus.c +4
-0@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;
This isn't a comment on this patch per se.
I am concerned about the fact that "pci=use_crs" is not the default.
Quoted 1 line
From the changelog of
62f420f8282, it sounds like you have to boot an
IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards.
We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet.
Bjorn
Quoted 35 lines · On Mon, 27 Apr 2009 13:44:01 -0600 Bjorn Helgaas <[email protected]> wrote:
Quoted 16 lines · Yinghai Lu
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Signed-off-by Yinghai Lu <[email protected]>--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)Index:
linux-2.6/arch/x86/pci/amd_bus.c
===================================================================arch/x86/pci/amd_bus.c void x86_pci_root_bus_res_quirks(struct · line 109
Quoted 10 lines
arch/x86/pci/amd_bus.c +4
-0@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;This isn't a comment on this patch per se.
I am concerned about the fact that "pci=use_crs" is not the default. From the changelog of
62f420f8282, it sounds like you have to boot an IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards.
Quoted 4 lines
We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet.
Well, we could try using _CRS by default, but like many things ACPI we can probably only trust firmwares after a certain date (i.e. the date when Windows started relying on the data being correct in order to boot). Do we have a good cutoff for that? Or should we try generally enabling it early in 2.6.31 to see what happens?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 34 lines · Bjorn Helgaas
Quoted 15 lines · Yinghai Lu
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Signed-off-by Yinghai Lu <[email protected]>--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)Index:
linux-2.6/arch/x86/pci/amd_bus.c
===================================================================arch/x86/pci/amd_bus.c void x86_pci_root_bus_res_quirks(struct · line 109
Quoted 10 lines
arch/x86/pci/amd_bus.c +4
-0@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;This isn't a comment on this patch per se.
I am concerned about the fact that "pci=use_crs" is not the default. From the changelog of
62f420f8282, it sounds like you have to boot an IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards.
Quoted 4 lines
We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet.
other system may have broken _CRS.
maybe we could try to use DMI whitelist them?
YH
Quoted 35 lines · Yinghai Lu
Quoted 34 lines · Bjorn Helgaas
Quoted 15 lines · Yinghai Lu
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Signed-off-by Yinghai Lu <[email protected]>--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)Index:
linux-2.6/arch/x86/pci/amd_bus.c
===================================================================arch/x86/pci/amd_bus.c void x86_pci_root_bus_res_quirks(struct · line 109
Quoted 10 lines
arch/x86/pci/amd_bus.c +4
-0@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;This isn't a comment on this patch per se.
I am concerned about the fact that "pci=use_crs" is not the default. From the changelog of
62f420f8282, it sounds like you have to boot an IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards.
Quoted 6 lines
Quoted 4 lines
We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet.
other system may have broken
_CRS.
Do you have examples of problems here, or are you just worried that there *may* be problems?
Quoted 1 line
maybe we could try to use DMI whitelist them?
I don't like a whitelist because it requires ongoing maintenance for correctly-working machines. A blacklist is nicer because it only requires maintenance for *broken* machines. A date-based solution would be better from that point of view.
Bjorn
Quoted 5 lines · Bjorn Helgaas
Quoted 2 lines
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669] pci_bus 0000:00: resource 0 io: [0x00-0x3af] [ 9.371065] pci_bus 0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551] pci_bus 0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028] pci_bus 0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513] pci_bus 0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077] pci_bus 0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084] pci_bus 0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089] pci_bus 0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991] pci_bus 0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff]
without that: amd_bus.c will read that from pci conf space
[ 9.310965] pci_bus 0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621] pci_bus 0000:00: resource 1 io: [0x00-0xfff] [ 9.322020] pci_bus 0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373] pci_bus 0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378] pci_bus 0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731] pci_bus 0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084] pci_bus 0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099] pci_bus 0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757] pci_bus 0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff]
Quoted 7 lines
Quoted 2 lines
maybe we could try to use DMI whitelist them?
I don't like a whitelist because it requires ongoing maintenance for correctly-working machines. A blacklist is nicer because it only requires maintenance for *broken* machines. A date-based solution would be better from that point of view.
could try apply that in development cycle like -rcX, and disable that formal release.
so could find more broken system.
YH
Quoted 20 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
Quoted 1 line
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669] pci_bus 0000:00: resource 0 io: [0x00-0x3af] [ 9.371065] pci_bus 0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551] pci_bus 0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028] pci_bus 0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513] pci_bus 0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077] pci_bus 0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084] pci_bus 0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089] pci_bus 0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991] pci_bus 0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff]without that:
amd_bus.cwill read that from pci conf space
Quoted 12 lines
[ 9.310965] pci_bus 0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621] pci_bus 0000:00: resource 1 io: [0x00-0xfff] [ 9.322020] pci_bus 0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373] pci_bus 0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378] pci_bus 0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731] pci_bus 0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084] pci_bus 0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099] pci_bus 0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757] pci_bus 0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff]
It's interesting that many of the differences involve the legacy VGA I/O ports in the 0x3b0-0x3df range. My guess is that the AMD chipset has special routing for those ranges. If it didn't, it would be difficult to support VGA devices under the other two root bridges. Maybe that VGA routing doesn't show up in the bridge's PCI config space. Can you tell from the ASL whether the root bridge _SRS/_PRS/_CRS methods handle the VGA ranges specially?
One of the differences is that PCI config space shows a 64-bit region (bus 0000:00 mem 0x840000000-0xfcffffffff) that doesn't show up in the _CRS info. But the _CRS parsing depends on acpi_resource_to_address64(), which doesn't know about the ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 descriptors added in ACPI 3.0. So this difference could be a result of that Linux bug. It'd be interesting to see whether the test patch below makes a difference.
The PCI config space region 0xf0000000-0xffffffff, also on bus 0000:00, looks suspicious to me. I thought that area contained a bunch of BIOS-y things like reset vectors and local APICs.
Bjorn
drivers/acpi/acpica/rsxface.c +9 -2
@@ -62,8 +62,7 @@ ACPI_MODULE_NAME("rsxface")ACPI_COPY_FIELD (out, in, minimum); \ACPI_COPY_FIELD (out, in, maximum); \ACPI_COPY_FIELD (out, in, translation_offset); \ -ACPI_COPY_FIELD (out, in, address_length); \ -ACPI_COPY_FIELD (out, in, resource_source); +ACPI_COPY_FIELD (out, in, address_length);/* Local prototypes */ static acpi_status acpi_rs_match_vendor_resource (struct acpi_resource *resource,void *context);
@@ -328,6 +327,7 @@ acpi_resource_to_address64(struct acpi_resource *resource, {struct acpi_resource_address16 *address16;struct acpi_resource_address32 *address32; +struct acpi_resource_extended_address64 *address64;if (!resource || !out) {return (AE_BAD_PARAMETER );
@@ -356,6 +356,13 @@ acpi_resource_to_address64(struct acpi_resource *resource, sizeof(struct acpi_resource_address64));break ; +case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 : + + address64 = (struct acpi_resource_extended_address64 *) + &resource->data; +ACPI_COPY_ADDRESS (out, address64); +break ; +default :return (AE_BAD_PARAMETER ); }
Quoted 42 lines · Bjorn Helgaas
Quoted 19 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
Quoted 1 line
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669]pci_bus0000:00: resource 0 io: [0x00-0x3af] [ 9.371065]pci_bus0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551]pci_bus0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028]pci_bus0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513]pci_bus0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077]pci_bus0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084]pci_bus0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089]pci_bus0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991]pci_bus0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]Quoted 13 lines
without that:
amd_bus.cwill read that from pci conf space
[ 9.310965]pci_bus0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621]pci_bus0000:00: resource 1 io: [0x00-0xfff]
[ 9.322020]pci_bus0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373]pci_bus0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378]pci_bus0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731]pci_bus0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084]pci_bus0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099]pci_bus0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757]pci_bus0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]It's interesting that many of the differences involve the legacy VGA I/O ports in the 0x3b0-0x3df range. My guess is that the AMD chipset has special routing for those ranges. If it didn't, it would be difficult to support VGA devices under the other two root bridges. Maybe that VGA routing doesn't show up in the bridge's PCI config space. Can you tell from the ASL whether the root bridge _SRS/_PRS/_CRS methods handle the VGA ranges specially?
Quoted 7 lines
One of the differences is that PCI config space shows a 64-bit region (bus 0000:00 mem 0x840000000-0xfcffffffff) that doesn't show up in the
_CRSinfo. But the_CRSparsing depends onacpi_resource_to_address64(), which doesn't know about theACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64descriptors added in ACPI 3.0. So this difference could be a result of that Linux bug. It'd be interesting to see whether the test patch below makes a difference.
will check it.
Quoted 4 lines
The PCI config space region 0xf0000000-0xffffffff, also on bus 0000:00, looks suspicious to me. I thought that area contained a bunch of BIOS-y things like reset vectors and local APICs.
in the amd_bus.c, will put left over resource to def HT chain.
YH
Quoted 43 lines · Yinghai Lu
Quoted 42 lines · Bjorn Helgaas
Quoted 19 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
Quoted 1 line
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669]pci_bus0000:00: resource 0 io: [0x00-0x3af] [ 9.371065]pci_bus0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551]pci_bus0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028]pci_bus0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513]pci_bus0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077]pci_bus0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084]pci_bus0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089]pci_bus0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991]pci_bus0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]Quoted 13 lines
without that:
amd_bus.cwill read that from pci conf space
[ 9.310965]pci_bus0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621]pci_bus0000:00: resource 1 io: [0x00-0xfff]
[ 9.322020]pci_bus0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373]pci_bus0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378]pci_bus0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731]pci_bus0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084]pci_bus0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099]pci_bus0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757]pci_bus0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]It's interesting that many of the differences involve the legacy VGA I/O ports in the 0x3b0-0x3df range. My guess is that the AMD chipset has special routing for those ranges. If it didn't, it would be difficult to support VGA devices under the other two root bridges. Maybe that VGA routing doesn't show up in the bridge's PCI config space. Can you tell from the ASL whether the root bridge _SRS/_PRS/_CRS methods handle the VGA ranges specially?
Quoted 8 lines
Quoted 7 lines
One of the differences is that PCI config space shows a 64-bit region (bus 0000:00 mem 0x840000000-0xfcffffffff) that doesn't show up in the
_CRSinfo. But the_CRSparsing depends onacpi_resource_to_address64(), which doesn't know about theACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64descriptors added in ACPI 3.0. So this difference could be a result of that Linux bug. It'd be interesting to see whether the test patch below makes a difference.will check it.
Did you learn anything about this? I have a PNPACPI patch to parse these new descriptors, but I don't have any machines where I can test it. If your box uses that descriptor, it'd be nice to test the patch there.
Quoted 8 lines
Quoted 3 lines
The PCI config space region 0xf0000000-0xffffffff, also on bus 0000:00, looks suspicious to me. I thought that area contained a bunch of BIOS-y things like reset vectors and local APICs.
in the
amd_bus.c, will put left over resource to def HT chain.YH
Quoted 30 lines · Bjorn Helgaas
Quoted 15 lines · Yinghai Lu
it wil be overwriten later if
_CRSis used, so don't bother to set it.[ Impact: cleanup ]
Signed-off-by Yinghai Lu <[email protected]>--- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+)Index:
linux-2.6/arch/x86/pci/amd_bus.c
===================================================================arch/x86/pci/amd_bus.c void x86_pci_root_bus_res_quirks(struct · line 109
Quoted 10 lines
arch/x86/pci/amd_bus.c +4
-0@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(structint j;struct pci_root_info *info; +/* don't go for it if _CRS is used */ +if (pci_probe &PCI_USE__CRS ) +return ; +/* if only one root bus, don't need to anything */ if (pci_root_num <2 )return ;This isn't a comment on this patch per se.
I am concerned about the fact that "pci=use_crs" is not the default.
Quoted 8 lines
Quoted 1 line
From the changelog of
62f420f8282, it sounds like you have to boot anIBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards.
We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet.
Sorry, I am behind on my email and just noticed this.
When I posted the patches to add "pci=use_crs" it was only needed to enable PCI hotplug on a subset of our systems. At that time it was not apparent that others were interested. I was also concerned that real or anticipated breakage on on other systems might delay or prevent acceptance.
As long as there is an option to disable it I am also in favor of trying to make it the default.
Thanks!
Gary
Signature · 6 lines
Gary Hade System x Enablement IBM Linux Technology Center 503-578-4503 IBM T/L: 775-4503 [email protected] http://www.ibm.com/linux/ltc
Quoted 51 lines · Bjorn Helgaas
Quoted 50 lines · Yinghai Lu
Quoted 49 lines · Bjorn Helgaas
Quoted 19 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
Quoted 1 line
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669]pci_bus0000:00: resource 0 io: [0x00-0x3af] [ 9.371065]pci_bus0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551]pci_bus0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028]pci_bus0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513]pci_bus0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077]pci_bus0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084]pci_bus0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089]pci_bus0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991]pci_bus0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]Quoted 13 lines
without that:
amd_bus.cwill read that from pci conf space
[ 9.310965]pci_bus0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621]pci_bus0000:00: resource 1 io: [0x00-0xfff]
[ 9.322020]pci_bus0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373]pci_bus0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378]pci_bus0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731]pci_bus0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084]pci_bus0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099]pci_bus0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757]pci_bus0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]It's interesting that many of the differences involve the legacy VGA I/O ports in the 0x3b0-0x3df range. My guess is that the AMD chipset has special routing for those ranges. If it didn't, it would be difficult to support VGA devices under the other two root bridges. Maybe that VGA routing doesn't show up in the bridge's PCI config space. Can you tell from the ASL whether the root bridge _SRS/_PRS/_CRS methods handle the VGA ranges specially?
One of the differences is that PCI config space shows a 64-bit region (bus 0000:00 mem 0x840000000-0xfcffffffff) that doesn't show up in the
_CRSinfo. But the_CRSparsing depends onacpi_resource_to_address64(), which doesn't know about theACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64descriptors added in ACPI 3.0. So this difference could be a result of that Linux bug. It'd be interesting to see whether the test patch below makes a difference.
Quoted 6 lines
Quoted 1 line
will check it.
Did you learn anything about this? I have a PNPACPI patch to parse these new descriptors, but I don't have any machines where I can test it. If your box uses that descriptor, it'd be nice to test the patch there.
Oops, I should have just attached the PNPACPI patch in case anybody has a box where it can be tested. One way to test it would be to compare the output of "grep . /sys/devices/pnp*/*/{id,resources,options}" before and after the patch. If a BIOS uses the new descriptors, we should see some new resources after the patch.
PNPACPI: parse Extended Address Space Descriptors
From: Bjorn Helgaas <[email protected]>
Extended Address Space Descriptors are new in ACPI 3.0 and allow the BIOS to communicate device resource cacheability attributes (write-back, write-through, uncacheable, etc) to the OS.
Previously, PNPACPI ignored these descriptors, so if a BIOS used them, a device could be responding at addresses the OS doesn't know about. This patch adds support for these descriptors in _CRS and _PRS. We don't attempt to encode them for _SRS (just like we don't attempt to encode the existing 16-, 32-, and 64-bit Address Space Descriptors).
Unfortunately, I don't have a way to test this.
--- drivers/pnp/pnpacpi/rsparser.c | 45 ++++++++++++++++++++++++++++++++++++++-- 1 files changed, 43 insertions(+), 2 deletions(-)
drivers/pnp/pnpacpi/rsparser.c +43 -2
@@ -287,6 +287,25 @@ static void pnpacpi_parse_allocated_address_space(struct pnp_dev *dev, ACPI_DECODE_16); } +static void pnpacpi_parse_allocated_ext_address_space (struct pnp_dev *dev, +struct acpi_resource *res) +{ +struct acpi_resource_extended_address64 *p = &res->data.ext_address64; + +if (p->producer_consumer ==ACPI_PRODUCER ) +return ; + +if (p->resource_type ==ACPI_MEMORY_RANGE ) +pnpacpi_parse_allocated_memresource (dev, + p->minimum, p->address_length, + p->info.mem.write_protect); +else if (p->resource_type ==ACPI_IO_RANGE ) +pnpacpi_parse_allocated_ioresource (dev, + p->minimum, p->address_length, + p->granularity ==0xfff ?ACPI_DECODE_10 : +ACPI_DECODE_16 ); +} +static acpi_status pnpacpi_allocated_resource (struct acpi_resource *res,void *data) {
@@ -400,8 +419,7 @@ static acpi_status pnpacpi_allocated_resource(struct acpi_resource *res,break ;case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 : -if (res->data.ext_address64.producer_consumer ==ACPI_PRODUCER ) -return AE_OK ; +pnpacpi_parse_allocated_ext_address_space (dev, res);break ;case ACPI_RESOURCE_TYPE_EXTENDED_IRQ :
@@ -630,6 +648,28 @@ static __init void pnpacpi_parse_address_option(struct pnp_dev *dev, IORESOURCE_IO_FIXED); } +static __init void pnpacpi_parse_ext_address_option (struct pnp_dev *dev, +unsigned int option_flags, +struct acpi_resource *r) +{ +struct acpi_resource_extended_address64 *p = &r->data.ext_address64; +unsigned char flags =0 ; + +if (p->address_length ==0 ) +return ; + +if (p->resource_type ==ACPI_MEMORY_RANGE ) { +if (p->info.mem.write_protect ==ACPI_READ_WRITE_MEMORY ) + flags =IORESOURCE_MEM_WRITEABLE ; +pnp_register_mem_resource (dev, option_flags, p->minimum, + p->minimum,0 , p->address_length, + flags); + }else if (p->resource_type ==ACPI_IO_RANGE ) +pnp_register_port_resource (dev, option_flags, p->minimum, + p->minimum,0 , p->address_length, +IORESOURCE_IO_FIXED ); +} +struct acpipnp_parse_option_s {struct pnp_dev *dev;unsigned int option_flags;
@@ -711,6 +751,7 @@ static __init acpi_status pnpacpi_option_resource(struct acpi_resource *res,break ;case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 : +pnpacpi_parse_ext_address_option (dev, option_flags, res);break ;case ACPI_RESOURCE_TYPE_EXTENDED_IRQ :
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 59 lines · Bjorn Helgaas
Quoted 51 lines · Bjorn Helgaas
Quoted 50 lines · Yinghai Lu
Quoted 49 lines · Bjorn Helgaas
Quoted 19 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
Quoted 1 line
other system may have broken
_CRS.Do you have examples of problems here, or are you just worried that there *may* be problems?
one system with three chains... with pci=use_crs
[ 9.365669]pci_bus0000:00: resource 0 io: [0x00-0x3af] [ 9.371065]pci_bus0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551]pci_bus0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028]pci_bus0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513]pci_bus0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077]pci_bus0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084]pci_bus0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089]pci_bus0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991]pci_bus0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]Quoted 13 lines
without that:
amd_bus.cwill read that from pci conf space
[ 9.310965]pci_bus0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621]pci_bus0000:00: resource 1 io: [0x00-0xfff]
[ 9.322020]pci_bus0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373]pci_bus0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378]pci_bus0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731]pci_bus0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084]pci_bus0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440]pci_bus0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099]pci_bus0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757]pci_bus0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118]pci_bus0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777]pci_bus0000:80: resource 1 mem: [0xda000000-0xdaffffff]It's interesting that many of the differences involve the legacy VGA I/O ports in the 0x3b0-0x3df range. My guess is that the AMD chipset has special routing for those ranges. If it didn't, it would be difficult to support VGA devices under the other two root bridges. Maybe that VGA routing doesn't show up in the bridge's PCI config space. Can you tell from the ASL whether the root bridge _SRS/_PRS/_CRS methods handle the VGA ranges specially?
One of the differences is that PCI config space shows a 64-bit region (bus 0000:00 mem 0x840000000-0xfcffffffff) that doesn't show up in the
_CRSinfo. But the_CRSparsing depends onacpi_resource_to_address64(), which doesn't know about theACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64descriptors added in ACPI 3.0. So this difference could be a result of that Linux bug. It'd be interesting to see whether the test patch below makes a difference.Quoted 6 lines
Quoted 1 line
will check it.
Did you learn anything about this? I have a PNPACPI patch to parse these new descriptors, but I don't have any machines where I can test it. If your box uses that descriptor, it'd be nice to test the patch there.
Quoted 5 lines
Oops, I should have just attached the PNPACPI patch in case anybody has a box where it can be tested. One way to test it would be to compare the output of "grep .
/sys/devices/pnp*/*/{id,resources,options}" before and after the patch. If a BIOS uses the new descriptors, we should see some new resources after the patch.
Did anything happen with this?
The longer we wait to make "use_crs" the default, the harder it will be, so I'd like to push ahead.
Bjorn
Quoted 92 lines
PNPACPI: parse Extended Address Space Descriptors
From: Bjorn Helgaas <[email protected]>
Extended Address Space Descriptors are new in ACPI 3.0 and allow the BIOS to communicate device resource cacheability attributes (write-back, write-through, uncacheable, etc) to the OS.
Previously, PNPACPI ignored these descriptors, so if a BIOS used them, a device could be responding at addresses the OS doesn't know about. This patch adds support for these descriptors in
_CRSand_PRS. We don't attempt to encode them for_SRS(just like we don't attempt to encode the existing 16-, 32-, and 64-bit Address Space Descriptors).Unfortunately, I don't have a way to test this.
Signed-off-by Bjorn Helgaas <[email protected]>--- drivers/pnp/pnpacpi/rsparser.c | 45 ++++++++++++++++++++++++++++++++++++++-- 1 files changed, 43 insertions(+), 2 deletions(-)drivers/pnp/pnpacpi/rsparser.c +42
-2@@ -287,6 +287,25 @@ static void pnpacpi_parse_allocated_address_space(struct pnp_dev *dev, ACPI_DECODE_16); } +static void pnpacpi_parse_allocated_ext_address_space (struct pnp_dev *dev, +struct acpi_resource *res) +{ +struct acpi_resource_extended_address64 *p = &res->data.ext_address64; + +if (p->producer_consumer ==ACPI_PRODUCER ) +return ; + +if (p->resource_type ==ACPI_MEMORY_RANGE ) +pnpacpi_parse_allocated_memresource (dev, + p->minimum, p->address_length, + p->info.mem.write_protect); +else if (p->resource_type ==ACPI_IO_RANGE ) +pnpacpi_parse_allocated_ioresource (dev, + p->minimum, p->address_length, + p->granularity ==0xfff ?ACPI_DECODE_10 : +ACPI_DECODE_16 ); +} +static acpi_status pnpacpi_allocated_resource (struct acpi_resource *res,void *data) {@@ -400,8 +419,7 @@ static acpi_status pnpacpi_allocated_resource(struct acpi_resource *res,break ;case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 : -if (res->data.ext_address64.producer_consumer ==ACPI_PRODUCER ) -return AE_OK ; +pnpacpi_parse_allocated_ext_address_space (dev, res);break ;case ACPI_RESOURCE_TYPE_EXTENDED_IRQ :@@ -630,6 +648,28 @@ static __init void pnpacpi_parse_address_option(struct pnp_dev *dev, IORESOURCE_IO_FIXED); } +static __init void pnpacpi_parse_ext_address_option (struct pnp_dev *dev, +unsigned int option_flags, +struct acpi_resource *r) +{ +struct acpi_resource_extended_address64 *p = &r->data.ext_address64; +unsigned char flags =0 ; + +if (p->address_length ==0 ) +return ; + +if (p->resource_type ==ACPI_MEMORY_RANGE ) { +if (p->info.mem.write_protect ==ACPI_READ_WRITE_MEMORY ) + flags =IORESOURCE_MEM_WRITEABLE ; +pnp_register_mem_resource (dev, option_flags, p->minimum, + p->minimum,0 , p->address_length, + flags); + }else if (p->resource_type ==ACPI_IO_RANGE ) +pnp_register_port_resource (dev, option_flags, p->minimum, + p->minimum,0 , p->address_length, +IORESOURCE_IO_FIXED ); +} +struct acpipnp_parse_option_s {struct pnp_dev *dev;unsigned int option_flags;
drivers/pnp/pnpacpi/rsparser.c static __init acpi_status pnpacpi_option_resource(struct acpi_resource *res, · line 757
Quoted 7 lines
drivers/pnp/pnpacpi/rsparser.c +1
-0@@ -711,6 +751,7 @@ static __init acpi_status pnpacpi_option_resource(struct acpi_resource *res,break ;case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 : +pnpacpi_parse_ext_address_option (dev, option_flags, res);break ;case ACPI_RESOURCE_TYPE_EXTENDED_IRQ :
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.
Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Documentation/kernel-parameters.txt +1 -1
@@ -1777,7 +1777,7 @@ and is between 256 and 4096 characters. It is defined in the file IRQ routing is enabled. noacpi [X86] Do not use ACPI for IRQ routing or for PCI scanning. - use_crs [X86] Use _CRS for PCI resource + nocrs [X86] Don't use _CRS for PCI resource allocation. routeirq Do IRQ routing for all PCI devices. This is normally done in pci_enable_device(),
arch/x86/include/asm/pci_x86.h +1 -1
@@ -25,7 +25,7 @@#define PCI_BIOS_IRQ_SCAN 0x2000#define PCI_ASSIGN_ALL_BUSSES 0x4000#define PCI_CAN_SKIP_ISA_ALIGN 0x8000 -#define PCI_USE__CRS 0x10000 +#define PCI_NO_CRS 0x10000#define PCI_CHECK_ENABLE_AMD_MMCONF 0x20000#define PCI_HAS_IO_ECS 0x40000#define PCI_NOASSIGN_ROMS 0x80000
arch/x86/pci/acpi.c +1 -1
@@ -217,7 +217,7 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_device *device, int do#endif } -if (bus && (pci_probe &PCI_USE__CRS )) +if (bus && !(pci_probe &PCI_NO_CRS ))get_current_resources (device, busnum, domain, bus);return bus; }
arch/x86/pci/amd_bus.c +1 -1
@@ -101,7 +101,7 @@ void x86_pci_root_bus_res_quirks(struct pci_bus *b)struct pci_root_info *info;/* don't go for it if _CRS is used */ -if (pci_probe &PCI_USE__CRS ) + if (!(pci_probe & PCI_NO_CRS)return ;/* if only one root bus, don't need to anything */
arch/x86/pci/common.c +2 -2
@@ -515,8 +515,8 @@ char * __devinit pcibios_setup(char *str) }else if (!strcmp (str,"assign-busses" )) { pci_probe |=PCI_ASSIGN_ALL_BUSSES ;return NULL ; - }else if (!strcmp (str,"use_crs" )) { - pci_probe |=PCI_USE__CRS ; + }else if (!strcmp (str,"nocrs" )) { + pci_probe |=PCI_NO_CRS ;return NULL ; }else if (!strcmp (str,"earlydump" )) { pci_early_dump_regs =1 ;
Quoted 32 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
--
Jesse Barnes, Intel Open Source Technology CenterDocumentation/kernel-parameters.txt +0
-0@@ -1777,7 +1777,7 @@ and is between 256 and 4096 characters. It is defined in the fileIRQ routing is enabled.
noacpi [X86] Do not use ACPI for IRQ routing or for PCI scanning.
-use_crs[X86] Use_CRSfor PCI resource
+ nocrs [X86] Don't use_CRSfor PCI resource allocation.
routeirq Do IRQ routing for all PCI devices. This is normally done inpci_enable_device(),arch/x86/include/asm/pci_x86.h +0
-0
arch/x86/include/asm/pci_x86.h line 28
Quoted 5 lines
arch/x86/include/asm/pci_x86.h +1
-1@@ -25,7 +25,7 @@ #define PCI_BIOS_IRQ_SCAN 0x2000 #define PCI_ASSIGN_ALL_BUSSES 0x4000 #define PCI_CAN_SKIP_ISA_ALIGN 0x8000 -#define PCI_USE__CRS 0x10000 +#define PCI_NO_CRS 0x10000
PCI_NO_ROOT_CRS is more accurate ?
YH
Quoted 10 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Thanks for following up on this, Jesse. It was on my to-do list for yesterday, but I didn't get to it.
Yinghai mentioned a specific box where we might have trouble, but we never got enough details to really debug it. So I think we might as well give it a shot and fine-tune it as we need to.
Bjorn
Quoted 12 lines · Bjorn Helgaas
Quoted 10 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Quoted 6 lines
Thanks for following up on this, Jesse. It was on my to-do list for yesterday, but I didn't get to it.
Yinghai mentioned a specific box where we might have trouble, but we never got enough details to really debug it. So I think we might as well give it a shot and fine-tune it as we need to.
I just remembered that Andrew Morton once raised some concerns related to the number of _CRS returned resources exceeding the fixed resource array size -> http://lkml.org/lkml/2007/11/1/49
I believe PCI_BUS_NUM_RESOURCES was later increased but if that increase was insufficient to cover all systems that the code will now be exposed to, it seems like the "And should we really be silently ignoring this problem? Should we at least report it?" comment that was not addressed could become relevent.
Gary
Signature · 6 lines
Gary Hade System x Enablement IBM Linux Technology Center 503-578-4503 IBM T/L: 775-4503 [email protected] http://www.ibm.com/linux/ltc
Quoted 24 lines · Gary Hade
Quoted 12 lines · Bjorn Helgaas
Quoted 10 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Quoted 6 lines
Thanks for following up on this, Jesse. It was on my to-do list for yesterday, but I didn't get to it.
Yinghai mentioned a specific box where we might have trouble, but we never got enough details to really debug it. So I think we might as well give it a shot and fine-tune it as we need to.
I just remembered that Andrew Morton once raised some concerns related to the number of
_CRSreturned resources exceeding the fixed resource array size -> http://lkml.org/lkml/2007/11/1/49
Quoted 5 lines
I believe
PCI_BUS_NUM_RESOURCESwas later increased but if that increase was insufficient to cover all systems that the code will now be exposed to, it seems like the "And should we really be silently ignoring this problem? Should we at least report it?" comment that was not addressed could become relevent.
Not only do we not warn, if we did warn based on the current
'if (info->res_num >= PCI_BUS_NUM_RESOURCES)'
resource array overrun avoidance criteria it would be incorrect for a root bus that is connected to a subordinate bus with a transparent bridge. We need to avoid the last 3 slots of the resource array since they do not get mapped to a transparent bridge connected subordinate bus. I believe the below patch addresses these issues.
With PCI_BUS_NUM_RESOURCES currently at 16 we still have space to accomodate 13 _CRS returned resource descriptors which will hopefully be sufficient.
Gary
Signature · 94 lines
Gary Hade System x Enablement IBM Linux Technology Center 503-578-4503 IBM T/L: 775-4503 [email protected] http://www.ibm.com/linux/ltc From: Gary Hade <[email protected]> Correct maximum allowed _CRS returned resources and warn if exceeded. Issue a warning if _CRS returns too many resource descriptors to be accommodated by the fixed size resource array instances. If there is no transparent bridge on the root bus "too many" is the PCI_BUS_NUM_RESOURCES size of the resource array. Otherwise, the last 3 slots of the resource array must be excluded making the maximum (PCI_BUS_NUM_RESOURCES - 3). The current code: - is silent when _CRS returns too many resource descriptors and - incorrectly allows use of the last 3 slots of the resource array for a root bus with a transparent bridge Signed-off-by: Gary Hade <[email protected]> --- arch/x86/pci/acpi.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) Index: linux-2.6.28-rc7/arch/x86/pci/acpi.c =================================================================== --- linux-2.6.28-rc7.orig/arch/x86/pci/acpi.c 2009-05-27 10:42:10.000000000 -0700 +++ linux-2.6.28-rc7/arch/x86/pci/acpi.c 2009-05-27 10:43:35.000000000 -0700 @@ -38,15 +38,26 @@ count_resource(struct acpi_resource *acp struct acpi_resource_address64 addr; acpi_status status; - if (info->res_num >= PCI_BUS_NUM_RESOURCES) - return AE_OK; - status = resource_to_addr(acpi_res, &addr); if (ACPI_SUCCESS(status)) info->res_num++; return AE_OK; } +static int +bus_has_transparent_bridge(struct pci_bus *bus) +{ + struct pci_dev *dev; + + list_for_each_entry(dev, &bus->devices, bus_list) { + u16 class = dev->class >> 8; + + if (class == PCI_CLASS_BRIDGE_PCI && dev->transparent) + return true; + } + return false; +} + static acpi_status setup_resource(struct acpi_resource *acpi_res, void *data) { @@ -56,9 +67,7 @@ setup_resource(struct acpi_resource *acp acpi_status status; unsigned long flags; struct resource *root; - - if (info->res_num >= PCI_BUS_NUM_RESOURCES) - return AE_OK; + int max_root_bus_resources = PCI_BUS_NUM_RESOURCES; status = resource_to_addr(acpi_res, &addr); if (!ACPI_SUCCESS(status)) @@ -82,6 +91,18 @@ setup_resource(struct acpi_resource *acp res->end = res->start + addr.address_length - 1; res->child = NULL; + if (bus_has_transparent_bridge(info->bus)) + max_root_bus_resources -= 3; + if (info->res_num >= max_root_bus_resources) { + printk(KERN_WARNING "PCI: Failed to allocate 0x%lx-0x%lx " + "from %s for %s due to _CRS returning more than " + "%d resource descriptors\n", (unsigned long) res->start, + (unsigned long) res->end, root->name, info->name, + max_root_bus_resources); + info->res_num++; + return AE_OK; + } + if (insert_resource(root, res)) { printk(KERN_ERR "PCI: Failed to allocate 0x%lx-0x%lx " "from %s for %s\n", (unsigned long) res->start,
Quoted 41 lines · On Wed, 27 May 2009 12:41:44 -0700 Gary Hade <[email protected]> wrote:
Quoted 25 lines · Gary Hade
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Quoted 6 lines
Thanks for following up on this, Jesse. It was on my to-do list for yesterday, but I didn't get to it.
Yinghai mentioned a specific box where we might have trouble, but we never got enough details to really debug it. So I think we might as well give it a shot and fine-tune it as we need to.
I just remembered that Andrew Morton once raised some concerns related to the number of
_CRSreturned resources exceeding the fixed resource array size -> http://lkml.org/lkml/2007/11/1/49Quoted 5 lines
I believe
PCI_BUS_NUM_RESOURCESwas later increased but if that increase was insufficient to cover all systems that the code will now be exposed to, it seems like the "And should we really be silently ignoring this problem? Should we at least report it?" comment that was not addressed could become relevent.Not only do we not warn, if we did warn based on the current
'if (info->res_num >= PCI_BUS_NUM_RESOURCES)'resource array overrun avoidance criteria it would be incorrect for a root bus that is connected to a subordinate bus with a transparent bridge. We need to avoid the last 3 slots of the resource array since they do not get mapped to a transparent bridge connected subordinate bus. I believe the below patch addresses these issues.
Quoted 3 lines
With
PCI_BUS_NUM_RESOURCEScurrently at 16 we still have space to accomodate 13_CRSreturned resource descriptors which will hopefully be sufficient.
Ok, applied the patch (with Yinghai's suggested name change) to my linux-next branch. We'll see now it goes and revert it if there's trouble.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 41 lines · On Wed, 27 May 2009 12:41:44 -0700 Gary Hade <[email protected]> wrote:
Quoted 25 lines · Gary Hade
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · Jesse Barnes
Quoted 4 lines · On Fri, 8 May 2009 16:40:10 -0600 Bjorn Helgaas <[email protected]> wrote:
Did anything happen with this?
The longer we wait to make "
use_crs" the default, the harder it will be, so I'd like to push ahead.Here's a patch to make CRS the default. If it looks ok I can push it into my linux-next branch. I'm all for using reliable data from the BIOS. I guess we'll find out fairly quickly if this stuff isn't...
Quoted 6 lines
Thanks for following up on this, Jesse. It was on my to-do list for yesterday, but I didn't get to it.
Yinghai mentioned a specific box where we might have trouble, but we never got enough details to really debug it. So I think we might as well give it a shot and fine-tune it as we need to.
I just remembered that Andrew Morton once raised some concerns related to the number of
_CRSreturned resources exceeding the fixed resource array size -> http://lkml.org/lkml/2007/11/1/49Quoted 5 lines
I believe
PCI_BUS_NUM_RESOURCESwas later increased but if that increase was insufficient to cover all systems that the code will now be exposed to, it seems like the "And should we really be silently ignoring this problem? Should we at least report it?" comment that was not addressed could become relevent.Not only do we not warn, if we did warn based on the current
'if (info->res_num >= PCI_BUS_NUM_RESOURCES)'resource array overrun avoidance criteria it would be incorrect for a root bus that is connected to a subordinate bus with a transparent bridge. We need to avoid the last 3 slots of the resource array since they do not get mapped to a transparent bridge connected subordinate bus. I believe the below patch addresses these issues.
Quoted 3 lines
With
PCI_BUS_NUM_RESOURCEScurrently at 16 we still have space to accomodate 13_CRSreturned resource descriptors which will hopefully be sufficient.
Ok, just applied this one to my linux-next tree, since we'll want it now that _CRS is used by default.
Len, is this ok with you? It touches x86 ACPI files rather than PCI files, but it's pretty tightly related...
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center