it wil be overwriten later if _CRS is 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
===================================================================
--- linux-2.6.orig/arch/x86/pci/amd_bus.c
+++ linux-2.6/arch/x86/pci/amd_bus.c
@@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
int 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;
On Mon, 20 Apr 2009 18:35:40 -0700
Yinghai Lu <[email protected]> wrote:
>
> it wil be overwriten later if _CRS is 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,
--
Jesse Barnes, Intel Open Source Technology Center
On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote:
> it wil be overwriten later if _CRS is 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
> ===================================================================
> --- linux-2.6.orig/arch/x86/pci/amd_bus.c
> +++ linux-2.6/arch/x86/pci/amd_bus.c
> @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
> int 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.
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
On Mon, 27 Apr 2009 13:44:01 -0600
Bjorn Helgaas <[email protected]> wrote:
> On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote:
> > it wil be overwriten later if _CRS is 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
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/pci/amd_bus.c
> > +++ linux-2.6/arch/x86/pci/amd_bus.c
> > @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
> > int 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.
>
> 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?
--
Jesse Barnes, Intel Open Source Technology Center
Bjorn Helgaas wrote:
> On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote:
>> it wil be overwriten later if _CRS is 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
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/pci/amd_bus.c
>> +++ linux-2.6/arch/x86/pci/amd_bus.c
>> @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
>> int 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.
>
> 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
On Monday 27 April 2009 02:15:33 pm Yinghai Lu wrote:
> Bjorn Helgaas wrote:
> > On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote:
> >> it wil be overwriten later if _CRS is 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
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/pci/amd_bus.c
> >> +++ linux-2.6/arch/x86/pci/amd_bus.c
> >> @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
> >> int 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.
> >
> > 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?
> 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
On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <[email protected]> wrote:
>>
>> 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]
>
>> 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
On Monday 27 April 2009 03:00:16 pm Yinghai Lu wrote:
> On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <[email protected]> wrote:
> >> 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]
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
diff --git a/drivers/acpi/acpica/rsxface.c b/drivers/acpi/acpica/rsxface.c
index 69a2aa5..dc2c5cb 100644
--- a/drivers/acpi/acpica/rsxface.c
+++ b/drivers/acpi/acpica/rsxface.c
@@ -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);
}
On Mon, Apr 27, 2009 at 3:24 PM, Bjorn Helgaas <[email protected]> wrote:
> On Monday 27 April 2009 03:00:16 pm Yinghai Lu wrote:
>> On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <[email protected]> wrote:
>> >> 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]
>
> 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.
will check it.
>
> 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
On Monday 27 April 2009 08:07:01 pm Yinghai Lu wrote:
> On Mon, Apr 27, 2009 at 3:24 PM, Bjorn Helgaas <[email protected]> wrote:
> > On Monday 27 April 2009 03:00:16 pm Yinghai Lu wrote:
> >> On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <[email protected]> wrote:
> >> >> 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]
> >
> > 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.
> 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.
> > 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
>
On Mon, Apr 27, 2009 at 01:44:01PM -0600, Bjorn Helgaas wrote:
> On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote:
> > it wil be overwriten later if _CRS is 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
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/pci/amd_bus.c
> > +++ linux-2.6/arch/x86/pci/amd_bus.c
> > @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct
> > int 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.
>
> 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
--
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
On Wednesday 29 April 2009 05:08:51 pm Bjorn Helgaas wrote:
> On Monday 27 April 2009 08:07:01 pm Yinghai Lu wrote:
> > On Mon, Apr 27, 2009 at 3:24 PM, Bjorn Helgaas <[email protected]> wrote:
> > > On Monday 27 April 2009 03:00:16 pm Yinghai Lu wrote:
> > >> On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <[email protected]> wrote:
> > >> >> 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]
> > >
> > > 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.
> > 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.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pnp/pnpacpi/rsparser.c | 45 ++++++++++++++++++++++++++++++++++++++--
1 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index adf1785..0864242 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -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:
On Wed, 27 May 2009 12:41:44 -0700
Gary Hade <[email protected]> wrote:
> On Thu, May 21, 2009 at 09:37:37AM -0700, Gary Hade wrote:
> > On Thu, May 21, 2009 at 08:46:17AM -0600, Bjorn Helgaas wrote:
> > > On Wednesday 20 May 2009 5:49:10 pm Jesse Barnes wrote:
> > > > 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.
> >
> > 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.
>
> 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.
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.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 27 May 2009 12:41:44 -0700
Gary Hade <[email protected]> wrote:
> On Thu, May 21, 2009 at 09:37:37AM -0700, Gary Hade wrote:
> > On Thu, May 21, 2009 at 08:46:17AM -0600, Bjorn Helgaas wrote:
> > > On Wednesday 20 May 2009 5:49:10 pm Jesse Barnes wrote:
> > > > 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.
> >
> > 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.
>
> 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.
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,
--
Jesse Barnes, Intel Open Source Technology Center