* [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
@ 2015-03-30 2:40 Jiang Liu
2015-04-03 11:49 ` Rafael J. Wysocki
2015-04-04 3:04 ` Bjorn Helgaas
0 siblings, 2 replies; 17+ messages in thread
From: Jiang Liu @ 2015-03-30 2:40 UTC (permalink / raw)
To: Rafael J . Wysocki, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown
Cc: Jiang Liu, Lv Zheng, LKML, linux-pci, linux-acpi
Before commit 593669c2ac0f("Use common ACPI resource interfaces to
simplify implementation"), arch/x86/pci/acpi.c applies following
rules when parsing ACPI resources for PCI host bridge:
1) Ignore IO port resources defined by acpi_resource_io and
acpi_resource_fixed_io, which should be used to define resource
for PCI device instead of PCI bridge.
2) Accept IOMEM resource defined by acpi_resource_memory24,
acpi_resource_memory32 and acpi_resource_fixed_memory32.
These IOMEM resources are accepted to workaround some BIOS issue,
though they should be ignored. For example, PC Engines APU.1C
platform defines PCI host bridge IOMEM resources as:
Memory32Fixed (ReadOnly,
0x000A0000, // Address Base
0x00020000, // Address Length
)
Memory32Fixed (ReadOnly,
0x00000000, // Address Base
0x00000000, // Address Length
_Y00)
3) Accept all IO port and IOMEM resources defined by
acpi_resource_address{16,32,64,extended64}, no matter it's marked as
ACPI_CONSUMER or ACPI_PRODUCER.
Commit 593669c2ac0f("Use common ACPI resource interfaces to
simplify implementation") accept all IO port and IOMEM resources
defined by acpi_resource_io, acpi_resource_fixed_io,
acpi_resource_memory24, acpi_resource_memory32,
acpi_resource_fixed_memory32 and
acpi_resource_address{16,32,64,extended64}, which causes IO port
resources consumed by host bridge itself are listed in to host bridge
resource list.
Then commit 63f1789ec716("Ignore resources consumed by host bridge
itself") ignores resources consumed by host bridge itself by checking
IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
above for BIOS bug .
It's really costed us much time to figure out this whole picture.
So we refine interface acpi_dev_filter_resource_type as below,
which should be easier for maintence:
1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
for bridge(PRODUCER), otherwise it's querying resource for
device(CONSUMER).
2) Ignore IO port resources defined by acpi_resource_io and
acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
3) Accpet IOMEM resource defined by acpi_resource_memory24,
acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS
bugs, with comment to state it's workaround for BIOS bug.
4) Accept IO port and IOMEM defined by acpi_resource_addressxx if
a) IORESOURCE_WINDOW is specified and ACPI_PRODUCER is true
b) IORESOURCE_WINDOW is not specified and ACPI_PRODUCER is false
Currently acpi_dev_filter_resource_type() is only used by ACPI pci
host bridge and IOAPIC driver, so it shouldn't affect other drivers.
Another possible fix is to only ignore IO resource consumed by host
bridge and keep IOMEM resource consumed by host bridge, please refer to:
http://www.spinics.net/lists/linux-pci/msg39706.html
Sample ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221
V2->V3:
Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
arch/x86/pci/acpi.c | 5 ++---
drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..8c4b1201f340 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
info->bridge = device;
ret = acpi_dev_get_resources(device, list,
acpi_dev_filter_resource_type_cb,
- (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+ (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
if (ret < 0)
dev_warn(&device->dev,
"failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
"no IO and memory resources present in _CRS\n");
else
resource_list_for_each_entry_safe(entry, tmp, list) {
- if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
- (entry->res->flags & IORESOURCE_DISABLED))
+ if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
entry->res->name = info->name;
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 5589a6e2a023..e761a868bdba 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
}
EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
+static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
+{
+ return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
+ ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
+}
+
/**
* acpi_dev_filter_resource_type - Filter ACPI resource according to resource
* types
@@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
case ACPI_RESOURCE_TYPE_MEMORY24:
case ACPI_RESOURCE_TYPE_MEMORY32:
case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
+ /*
+ * These types of resource descriptor should be used to
+ * describe resource consumption instead of resource provision.
+ * But some platforms, such as PC Engines APU.1C, reports
+ * resource provision by Memory32Fixed(). Please refer to:
+ * https://bugzilla.kernel.org/show_bug.cgi?id=94221
+ * So accept it no matter IORESOURCE_WINDOW is specified or not.
+ */
type = IORESOURCE_MEM;
break;
case ACPI_RESOURCE_TYPE_IO:
case ACPI_RESOURCE_TYPE_FIXED_IO:
- type = IORESOURCE_IO;
+ if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
+ type = IORESOURCE_IO;
break;
case ACPI_RESOURCE_TYPE_IRQ:
+ if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
+ type = IORESOURCE_IRQ;
+ break;
case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
- type = IORESOURCE_IRQ;
+ if (acpi_dev_match_producer_consumer(types,
+ ares->data.extended_irq.producer_consumer))
+ type = IORESOURCE_IRQ;
break;
case ACPI_RESOURCE_TYPE_DMA:
case ACPI_RESOURCE_TYPE_FIXED_DMA:
- type = IORESOURCE_DMA;
+ if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
+ type = IORESOURCE_DMA;
break;
case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
- type = IORESOURCE_REG;
+ if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
+ type = IORESOURCE_REG;
break;
case ACPI_RESOURCE_TYPE_ADDRESS16:
case ACPI_RESOURCE_TYPE_ADDRESS32:
case ACPI_RESOURCE_TYPE_ADDRESS64:
case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
+ if (!acpi_dev_match_producer_consumer(types,
+ ares->data.address.producer_consumer))
+ break;
if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
type = IORESOURCE_MEM;
else if (ares->data.address.resource_type == ACPI_IO_RANGE)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-03-30 2:40 [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716 Jiang Liu
@ 2015-04-03 11:49 ` Rafael J. Wysocki
2015-04-04 3:04 ` Bjorn Helgaas
1 sibling, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-03 11:49 UTC (permalink / raw)
To: Jiang Liu, Bjorn Helgaas
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Len Brown,
Lv Zheng, LKML, linux-pci, linux-acpi
On Monday, March 30, 2015 10:40:43 AM Jiang Liu wrote:
> Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation"), arch/x86/pci/acpi.c applies following
> rules when parsing ACPI resources for PCI host bridge:
> 1) Ignore IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io, which should be used to define resource
> for PCI device instead of PCI bridge.
> 2) Accept IOMEM resource defined by acpi_resource_memory24,
> acpi_resource_memory32 and acpi_resource_fixed_memory32.
> These IOMEM resources are accepted to workaround some BIOS issue,
> though they should be ignored. For example, PC Engines APU.1C
> platform defines PCI host bridge IOMEM resources as:
> Memory32Fixed (ReadOnly,
> 0x000A0000, // Address Base
> 0x00020000, // Address Length
> )
> Memory32Fixed (ReadOnly,
> 0x00000000, // Address Base
> 0x00000000, // Address Length
> _Y00)
> 3) Accept all IO port and IOMEM resources defined by
> acpi_resource_address{16,32,64,extended64}, no matter it's marked as
> ACPI_CONSUMER or ACPI_PRODUCER.
>
> Commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation") accept all IO port and IOMEM resources
> defined by acpi_resource_io, acpi_resource_fixed_io,
> acpi_resource_memory24, acpi_resource_memory32,
> acpi_resource_fixed_memory32 and
> acpi_resource_address{16,32,64,extended64}, which causes IO port
> resources consumed by host bridge itself are listed in to host bridge
> resource list.
>
> Then commit 63f1789ec716("Ignore resources consumed by host bridge
> itself") ignores resources consumed by host bridge itself by checking
> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> above for BIOS bug .
>
> It's really costed us much time to figure out this whole picture.
> So we refine interface acpi_dev_filter_resource_type as below,
> which should be easier for maintence:
> 1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
> for bridge(PRODUCER), otherwise it's querying resource for
> device(CONSUMER).
> 2) Ignore IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
> 3) Accpet IOMEM resource defined by acpi_resource_memory24,
> acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS
> bugs, with comment to state it's workaround for BIOS bug.
> 4) Accept IO port and IOMEM defined by acpi_resource_addressxx if
> a) IORESOURCE_WINDOW is specified and ACPI_PRODUCER is true
> b) IORESOURCE_WINDOW is not specified and ACPI_PRODUCER is false
>
> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>
> Another possible fix is to only ignore IO resource consumed by host
> bridge and keep IOMEM resource consumed by host bridge, please refer to:
> http://www.spinics.net/lists/linux-pci/msg39706.html
>
> Sample ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>
> V2->V3:
> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
>
> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Bjorn, any comments here? We still have this regression in 4.0-rc ...
> ---
> arch/x86/pci/acpi.c | 5 ++---
> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..8c4b1201f340 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> info->bridge = device;
> ret = acpi_dev_get_resources(device, list,
> acpi_dev_filter_resource_type_cb,
> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> if (ret < 0)
> dev_warn(&device->dev,
> "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> "no IO and memory resources present in _CRS\n");
> else
> resource_list_for_each_entry_safe(entry, tmp, list) {
> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> - (entry->res->flags & IORESOURCE_DISABLED))
> + if (entry->res->flags & IORESOURCE_DISABLED)
> resource_list_destroy_entry(entry);
> else
> entry->res->name = info->name;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 5589a6e2a023..e761a868bdba 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> }
> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>
> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
> +{
> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
> +}
> +
> /**
> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> * types
> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> case ACPI_RESOURCE_TYPE_MEMORY24:
> case ACPI_RESOURCE_TYPE_MEMORY32:
> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> + /*
> + * These types of resource descriptor should be used to
> + * describe resource consumption instead of resource provision.
> + * But some platforms, such as PC Engines APU.1C, reports
> + * resource provision by Memory32Fixed(). Please refer to:
> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
> + */
> type = IORESOURCE_MEM;
> break;
> case ACPI_RESOURCE_TYPE_IO:
> case ACPI_RESOURCE_TYPE_FIXED_IO:
> - type = IORESOURCE_IO;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_IO;
> break;
> case ACPI_RESOURCE_TYPE_IRQ:
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_IRQ;
> + break;
> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> - type = IORESOURCE_IRQ;
> + if (acpi_dev_match_producer_consumer(types,
> + ares->data.extended_irq.producer_consumer))
> + type = IORESOURCE_IRQ;
> break;
> case ACPI_RESOURCE_TYPE_DMA:
> case ACPI_RESOURCE_TYPE_FIXED_DMA:
> - type = IORESOURCE_DMA;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_DMA;
> break;
> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
> - type = IORESOURCE_REG;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_REG;
> break;
> case ACPI_RESOURCE_TYPE_ADDRESS16:
> case ACPI_RESOURCE_TYPE_ADDRESS32:
> case ACPI_RESOURCE_TYPE_ADDRESS64:
> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
> + if (!acpi_dev_match_producer_consumer(types,
> + ares->data.address.producer_consumer))
> + break;
> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
> type = IORESOURCE_MEM;
> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-03-30 2:40 [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716 Jiang Liu
2015-04-03 11:49 ` Rafael J. Wysocki
@ 2015-04-04 3:04 ` Bjorn Helgaas
2015-04-07 0:28 ` Rafael J. Wysocki
1 sibling, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2015-04-04 3:04 UTC (permalink / raw)
To: Jiang Liu
Cc: Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
Hi Jiang,
Sorry for my delayed response. I've been on vacation for a week and am
still trying to catch up.
On Mon, Mar 30, 2015 at 10:40:43AM +0800, Jiang Liu wrote:
> Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation"), arch/x86/pci/acpi.c applies following
> rules when parsing ACPI resources for PCI host bridge:
> 1) Ignore IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io, which should be used to define resource
> for PCI device instead of PCI bridge.
> 2) Accept IOMEM resource defined by acpi_resource_memory24,
> acpi_resource_memory32 and acpi_resource_fixed_memory32.
> These IOMEM resources are accepted to workaround some BIOS issue,
> though they should be ignored. For example, PC Engines APU.1C
> platform defines PCI host bridge IOMEM resources as:
> Memory32Fixed (ReadOnly,
> 0x000A0000, // Address Base
> 0x00020000, // Address Length
> )
> Memory32Fixed (ReadOnly,
> 0x00000000, // Address Base
> 0x00000000, // Address Length
> _Y00)
> 3) Accept all IO port and IOMEM resources defined by
> acpi_resource_address{16,32,64,extended64}, no matter it's marked as
> ACPI_CONSUMER or ACPI_PRODUCER.
>
> Commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation") accept all IO port and IOMEM resources
> defined by acpi_resource_io, acpi_resource_fixed_io,
> acpi_resource_memory24, acpi_resource_memory32,
> acpi_resource_fixed_memory32 and
> acpi_resource_address{16,32,64,extended64}, which causes IO port
> resources consumed by host bridge itself are listed in to host bridge
> resource list.
>
> Then commit 63f1789ec716("Ignore resources consumed by host bridge
> itself") ignores resources consumed by host bridge itself by checking
> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> above for BIOS bug .
This is probably partly my fault.
I think the ACPI spec intention is that every _CRS resource descriptor
should be interpreted as "Consumer," i.e., as resources consumed by the
device itself, unless it's marked otherwise. Only the following types can
be marked as "Producer":
- Word/DWord/QWord/Extended address space descriptors,
- Extended interrupt descriptors,
- GPIO interrupt and I/O connections,
- I2C/SPI/UART serial bus resource descriptors
With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource
types"), I made Linux treat Memory24, Memory32, and Memory32Fixed
descriptors in PCI host bridge _CRS as Producers. I did it because Windows
apparently does that (there are details in
https://bugzilla.kernel.org/show_bug.cgi?id=15817), but I wasn't aware of
any machines that required it. That was probably a mistake because it
didn't fix anything and it covered up ASL usage errors like what PC Engines
did.
> It's really costed us much time to figure out this whole picture.
> So we refine interface acpi_dev_filter_resource_type as below,
> which should be easier for maintence:
> 1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
> for bridge(PRODUCER), otherwise it's querying resource for
> device(CONSUMER).
Sounds good to me.
> 2) Ignore IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
Sounds good to me.
> 3) Accpet IOMEM resource defined by acpi_resource_memory24,
> acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS
> bugs, with comment to state it's workaround for BIOS bug.
I don't like the fact that this is the behavior for all ACPI devices.
Prior to 593669c2ac0f, we had this behavior for PCI host bridges only.
I don't think this is what the spec envisioned, so I don't really like
doing it for all devices.
> 4) Accept IO port and IOMEM defined by acpi_resource_addressxx if
> a) IORESOURCE_WINDOW is specified and ACPI_PRODUCER is true
> b) IORESOURCE_WINDOW is not specified and ACPI_PRODUCER is false
Sounds good to me.
> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
We should assume it will eventually be used for all ACPI devices,
shouldn't we?
> Another possible fix is to only ignore IO resource consumed by host
> bridge and keep IOMEM resource consumed by host bridge, please refer to:
> http://www.spinics.net/lists/linux-pci/msg39706.html
>
> Sample ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>
> V2->V3:
> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
>
> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> arch/x86/pci/acpi.c | 5 ++---
> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..8c4b1201f340 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> info->bridge = device;
> ret = acpi_dev_get_resources(device, list,
> acpi_dev_filter_resource_type_cb,
> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> if (ret < 0)
> dev_warn(&device->dev,
> "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> "no IO and memory resources present in _CRS\n");
> else
> resource_list_for_each_entry_safe(entry, tmp, list) {
> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> - (entry->res->flags & IORESOURCE_DISABLED))
> + if (entry->res->flags & IORESOURCE_DISABLED)
> resource_list_destroy_entry(entry);
> else
> entry->res->name = info->name;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 5589a6e2a023..e761a868bdba 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> }
> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>
> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
> +{
> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
> +}
> +
> /**
> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> * types
> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> case ACPI_RESOURCE_TYPE_MEMORY24:
> case ACPI_RESOURCE_TYPE_MEMORY32:
> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> + /*
> + * These types of resource descriptor should be used to
> + * describe resource consumption instead of resource provision.
> + * But some platforms, such as PC Engines APU.1C, reports
> + * resource provision by Memory32Fixed(). Please refer to:
> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
> + */
> type = IORESOURCE_MEM;
I think this means these resources will be accepted regardless of whether
the caller is looking for Consumer or Producer resources. To preserve the
behavior I added with 66528fdd45b0, we might be forced to do that for PCI
host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
But I don't think it matches the ACPI spec intent, so I'm not sure it's
right to do it for all devices.
> break;
> case ACPI_RESOURCE_TYPE_IO:
> case ACPI_RESOURCE_TYPE_FIXED_IO:
> - type = IORESOURCE_IO;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_IO;
> break;
> case ACPI_RESOURCE_TYPE_IRQ:
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_IRQ;
> + break;
> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> - type = IORESOURCE_IRQ;
> + if (acpi_dev_match_producer_consumer(types,
> + ares->data.extended_irq.producer_consumer))
> + type = IORESOURCE_IRQ;
> break;
> case ACPI_RESOURCE_TYPE_DMA:
> case ACPI_RESOURCE_TYPE_FIXED_DMA:
> - type = IORESOURCE_DMA;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_DMA;
> break;
> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
> - type = IORESOURCE_REG;
> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> + type = IORESOURCE_REG;
> break;
> case ACPI_RESOURCE_TYPE_ADDRESS16:
> case ACPI_RESOURCE_TYPE_ADDRESS32:
> case ACPI_RESOURCE_TYPE_ADDRESS64:
> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
> + if (!acpi_dev_match_producer_consumer(types,
> + ares->data.address.producer_consumer))
> + break;
> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
> type = IORESOURCE_MEM;
> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-04 3:04 ` Bjorn Helgaas
@ 2015-04-07 0:28 ` Rafael J. Wysocki
2015-04-07 13:30 ` Jiang Liu
2015-04-08 5:48 ` Jiang Liu
0 siblings, 2 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-07 0:28 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jiang Liu, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
> Hi Jiang,
>
> Sorry for my delayed response. I've been on vacation for a week and am
> still trying to catch up.
>
> On Mon, Mar 30, 2015 at 10:40:43AM +0800, Jiang Liu wrote:
> > Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation"), arch/x86/pci/acpi.c applies following
> > rules when parsing ACPI resources for PCI host bridge:
> > 1) Ignore IO port resources defined by acpi_resource_io and
> > acpi_resource_fixed_io, which should be used to define resource
> > for PCI device instead of PCI bridge.
> > 2) Accept IOMEM resource defined by acpi_resource_memory24,
> > acpi_resource_memory32 and acpi_resource_fixed_memory32.
> > These IOMEM resources are accepted to workaround some BIOS issue,
> > though they should be ignored. For example, PC Engines APU.1C
> > platform defines PCI host bridge IOMEM resources as:
> > Memory32Fixed (ReadOnly,
> > 0x000A0000, // Address Base
> > 0x00020000, // Address Length
> > )
> > Memory32Fixed (ReadOnly,
> > 0x00000000, // Address Base
> > 0x00000000, // Address Length
> > _Y00)
> > 3) Accept all IO port and IOMEM resources defined by
> > acpi_resource_address{16,32,64,extended64}, no matter it's marked as
> > ACPI_CONSUMER or ACPI_PRODUCER.
> >
> > Commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation") accept all IO port and IOMEM resources
> > defined by acpi_resource_io, acpi_resource_fixed_io,
> > acpi_resource_memory24, acpi_resource_memory32,
> > acpi_resource_fixed_memory32 and
> > acpi_resource_address{16,32,64,extended64}, which causes IO port
> > resources consumed by host bridge itself are listed in to host bridge
> > resource list.
> >
> > Then commit 63f1789ec716("Ignore resources consumed by host bridge
> > itself") ignores resources consumed by host bridge itself by checking
> > IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> > above for BIOS bug .
>
> This is probably partly my fault.
>
> I think the ACPI spec intention is that every _CRS resource descriptor
> should be interpreted as "Consumer," i.e., as resources consumed by the
> device itself, unless it's marked otherwise. Only the following types can
> be marked as "Producer":
>
> - Word/DWord/QWord/Extended address space descriptors,
> - Extended interrupt descriptors,
> - GPIO interrupt and I/O connections,
> - I2C/SPI/UART serial bus resource descriptors
So we're talking about the consumer/producer flag in those extended resource
type descriptors, right?
My understanding of that flag is that it doesn't say whether or not the device
is a producer of a resource in a general sense. It only says whether the device
consumes a resource provided by someone else (1) or it consumes a resources
provided by itself (0).
> With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource
> types"), I made Linux treat Memory24, Memory32, and Memory32Fixed
> descriptors in PCI host bridge _CRS as Producers. I did it because Windows
> apparently does that (there are details in
> https://bugzilla.kernel.org/show_bug.cgi?id=15817),
It looks like it does that to indicate that those resources are provided
by the host bridge itself (which is consistent with the consumer/producer
flag interpretation above)
> but I wasn't aware of any machines that required it. That was probably a
> mistake because it didn't fix anything and it covered up ASL usage errors
> like what PC Engines did.
I don't think it is required. It only seems to allow Windows to consolidate
the handling of host bridge resources.
> > It's really costed us much time to figure out this whole picture.
> > So we refine interface acpi_dev_filter_resource_type as below,
> > which should be easier for maintence:
> > 1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
> > for bridge(PRODUCER), otherwise it's querying resource for
> > device(CONSUMER).
>
> Sounds good to me.
>
> > 2) Ignore IO port resources defined by acpi_resource_io and
> > acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
>
> Sounds good to me.
>
> > 3) Accpet IOMEM resource defined by acpi_resource_memory24,
> > acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS
> > bugs, with comment to state it's workaround for BIOS bug.
>
> I don't like the fact that this is the behavior for all ACPI devices.
> Prior to 593669c2ac0f, we had this behavior for PCI host bridges only.
> I don't think this is what the spec envisioned, so I don't really like
> doing it for all devices.
Agreed.
> > 4) Accept IO port and IOMEM defined by acpi_resource_addressxx if
> > a) IORESOURCE_WINDOW is specified and ACPI_PRODUCER is true
> > b) IORESOURCE_WINDOW is not specified and ACPI_PRODUCER is false
>
> Sounds good to me.
>
> > Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> > host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>
> We should assume it will eventually be used for all ACPI devices,
> shouldn't we?
I'm not sure about that, really. In fact, I'd restrict its use to devices
types that actually can "produce" resources (ie. do not require the resources
to be provided by their ancestors or to be available from a global pool).
Otherwise we're pretty much guaranteed to get into trouble.
And all of the above rules need to be documented in the kernel source tree
or people will get confused.
> > Another possible fix is to only ignore IO resource consumed by host
> > bridge and keep IOMEM resource consumed by host bridge, please refer to:
> > http://www.spinics.net/lists/linux-pci/msg39706.html
> >
> > Sample ACPI table are archived at:
> > https://bugzilla.kernel.org/show_bug.cgi?id=94221
> >
> > V2->V3:
> > Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
> >
> > Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> > Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > ---
> > arch/x86/pci/acpi.c | 5 ++---
> > drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
> > 2 files changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index e4695985f9de..8c4b1201f340 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> > info->bridge = device;
> > ret = acpi_dev_get_resources(device, list,
> > acpi_dev_filter_resource_type_cb,
> > - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> > + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> > if (ret < 0)
> > dev_warn(&device->dev,
> > "failed to parse _CRS method, error code %d\n", ret);
> > @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> > "no IO and memory resources present in _CRS\n");
> > else
> > resource_list_for_each_entry_safe(entry, tmp, list) {
> > - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> > - (entry->res->flags & IORESOURCE_DISABLED))
> > + if (entry->res->flags & IORESOURCE_DISABLED)
> > resource_list_destroy_entry(entry);
> > else
> > entry->res->name = info->name;
> > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > index 5589a6e2a023..e761a868bdba 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> > }
> > EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
> >
> > +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
> > +{
> > + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
> > + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
> > +}
> > +
> > /**
> > * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> > * types
> > @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> > case ACPI_RESOURCE_TYPE_MEMORY24:
> > case ACPI_RESOURCE_TYPE_MEMORY32:
> > case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> > + /*
> > + * These types of resource descriptor should be used to
> > + * describe resource consumption instead of resource provision.
> > + * But some platforms, such as PC Engines APU.1C, reports
> > + * resource provision by Memory32Fixed(). Please refer to:
> > + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
> > + * So accept it no matter IORESOURCE_WINDOW is specified or not.
> > + */
> > type = IORESOURCE_MEM;
>
> I think this means these resources will be accepted regardless of whether
> the caller is looking for Consumer or Producer resources. To preserve the
> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
>
> But I don't think it matches the ACPI spec intent, so I'm not sure it's
> right to do it for all devices.
No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
for all devices.
> > break;
> > case ACPI_RESOURCE_TYPE_IO:
> > case ACPI_RESOURCE_TYPE_FIXED_IO:
> > - type = IORESOURCE_IO;
> > + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> > + type = IORESOURCE_IO;
> > break;
> > case ACPI_RESOURCE_TYPE_IRQ:
> > + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> > + type = IORESOURCE_IRQ;
> > + break;
> > case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> > - type = IORESOURCE_IRQ;
> > + if (acpi_dev_match_producer_consumer(types,
> > + ares->data.extended_irq.producer_consumer))
> > + type = IORESOURCE_IRQ;
> > break;
> > case ACPI_RESOURCE_TYPE_DMA:
> > case ACPI_RESOURCE_TYPE_FIXED_DMA:
> > - type = IORESOURCE_DMA;
> > + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> > + type = IORESOURCE_DMA;
> > break;
> > case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
> > - type = IORESOURCE_REG;
> > + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> > + type = IORESOURCE_REG;
> > break;
> > case ACPI_RESOURCE_TYPE_ADDRESS16:
> > case ACPI_RESOURCE_TYPE_ADDRESS32:
> > case ACPI_RESOURCE_TYPE_ADDRESS64:
> > case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
> > + if (!acpi_dev_match_producer_consumer(types,
> > + ares->data.address.producer_consumer))
> > + break;
> > if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
> > type = IORESOURCE_MEM;
> > else if (ares->data.address.resource_type == ACPI_IO_RANGE)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-07 0:28 ` Rafael J. Wysocki
@ 2015-04-07 13:30 ` Jiang Liu
2015-04-08 5:48 ` Jiang Liu
1 sibling, 0 replies; 17+ messages in thread
From: Jiang Liu @ 2015-04-07 13:30 UTC (permalink / raw)
To: Rafael J. Wysocki, Bjorn Helgaas
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Len Brown,
Lv Zheng, LKML, linux-pci, linux-acpi
On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>> Hi Jiang,
>>
>> Sorry for my delayed response. I've been on vacation for a week and am
>> still trying to catch up.
>>
>> On Mon, Mar 30, 2015 at 10:40:43AM +0800, Jiang Liu wrote:
<snip>
>>> Then commit 63f1789ec716("Ignore resources consumed by host bridge
>>> itself") ignores resources consumed by host bridge itself by checking
>>> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
>>> above for BIOS bug .
>>
>> This is probably partly my fault.
>>
>> I think the ACPI spec intention is that every _CRS resource descriptor
>> should be interpreted as "Consumer," i.e., as resources consumed by the
>> device itself, unless it's marked otherwise. Only the following types can
>> be marked as "Producer":
>>
>> - Word/DWord/QWord/Extended address space descriptors,
>> - Extended interrupt descriptors,
>> - GPIO interrupt and I/O connections,
>> - I2C/SPI/UART serial bus resource descriptors
>
> So we're talking about the consumer/producer flag in those extended resource
> type descriptors, right?
>
> My understanding of that flag is that it doesn't say whether or not the device
> is a producer of a resource in a general sense. It only says whether the device
> consumes a resource provided by someone else (1) or it consumes a resources
> provided by itself (0).
Hi Rafael,
I have read the ACPI spec again.
You are right, the spec states that:
Consumer/Producer:
1–This device consumes this resource
0–This device produces and consumes this resource
This is different from my previous understanding.
Previously I thought "CONSUMER" means the device consumes the resource
by itself and "PRODUCER" means the device provides resource to other
devices.
So seems PCI host bridge has different interpretation of
CONSUMER/PRODUCER flag from ACPI spec.
>
>> With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource
>> types"), I made Linux treat Memory24, Memory32, and Memory32Fixed
>> descriptors in PCI host bridge _CRS as Producers. I did it because Windows
>> apparently does that (there are details in
>> https://bugzilla.kernel.org/show_bug.cgi?id=15817),
>
> It looks like it does that to indicate that those resources are provided
> by the host bridge itself (which is consistent with the consumer/producer
> flag interpretation above)
>
>> but I wasn't aware of any machines that required it. That was probably a
>> mistake because it didn't fix anything and it covered up ASL usage errors
>> like what PC Engines did.
>
> I don't think it is required. It only seems to allow Windows to consolidate
> the handling of host bridge resources.
>
>>> It's really costed us much time to figure out this whole picture.
>>> So we refine interface acpi_dev_filter_resource_type as below,
>>> which should be easier for maintence:
>>> 1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
>>> for bridge(PRODUCER), otherwise it's querying resource for
>>> device(CONSUMER).
>>
>> Sounds good to me.
>>
>>> 2) Ignore IO port resources defined by acpi_resource_io and
>>> acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
>>
>> Sounds good to me.
>>
>>> 3) Accpet IOMEM resource defined by acpi_resource_memory24,
>>> acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS
>>> bugs, with comment to state it's workaround for BIOS bug.
>>
>> I don't like the fact that this is the behavior for all ACPI devices.
>> Prior to 593669c2ac0f, we had this behavior for PCI host bridges only.
>> I don't think this is what the spec envisioned, so I don't really like
>> doing it for all devices.
>
> Agreed.
>
>>> 4) Accept IO port and IOMEM defined by acpi_resource_addressxx if
>>> a) IORESOURCE_WINDOW is specified and ACPI_PRODUCER is true
>>> b) IORESOURCE_WINDOW is not specified and ACPI_PRODUCER is false
>>
>> Sounds good to me.
>>
>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>
>> We should assume it will eventually be used for all ACPI devices,
>> shouldn't we?
>
> I'm not sure about that, really. In fact, I'd restrict its use to devices
> types that actually can "produce" resources (ie. do not require the resources
> to be provided by their ancestors or to be available from a global pool).
>
> Otherwise we're pretty much guaranteed to get into trouble.
>
> And all of the above rules need to be documented in the kernel source tree
> or people will get confused.
You are right, we should limit acpi_dev_filter_resource_type() usages
to PCI host bridges and IOAPIC only.
>
>>> Another possible fix is to only ignore IO resource consumed by host
<snip>
>>> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>> case ACPI_RESOURCE_TYPE_MEMORY24:
>>> case ACPI_RESOURCE_TYPE_MEMORY32:
>>> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>> + /*
>>> + * These types of resource descriptor should be used to
>>> + * describe resource consumption instead of resource provision.
>>> + * But some platforms, such as PC Engines APU.1C, reports
>>> + * resource provision by Memory32Fixed(). Please refer to:
>>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
>>> + */
>>> type = IORESOURCE_MEM;
>>
>> I think this means these resources will be accepted regardless of whether
>> the caller is looking for Consumer or Producer resources. To preserve the
>> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
>> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
>>
>> But I don't think it matches the ACPI spec intent, so I'm not sure it's
>> right to do it for all devices.
>
> No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
> for all devices.
Got it, will update comments.
Thanks!
Gerry
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-07 0:28 ` Rafael J. Wysocki
2015-04-07 13:30 ` Jiang Liu
@ 2015-04-08 5:48 ` Jiang Liu
2015-04-08 23:44 ` Rafael J. Wysocki
1 sibling, 1 reply; 17+ messages in thread
From: Jiang Liu @ 2015-04-08 5:48 UTC (permalink / raw)
To: Rafael J. Wysocki, Bjorn Helgaas
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Len Brown,
Lv Zheng, LKML, linux-pci, linux-acpi
On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>> Hi Jiang,
<snip>
>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>
>> We should assume it will eventually be used for all ACPI devices,
>> shouldn't we?
>
> I'm not sure about that, really. In fact, I'd restrict its use to devices
> types that actually can "produce" resources (ie. do not require the resources
> to be provided by their ancestors or to be available from a global pool).
>
> Otherwise we're pretty much guaranteed to get into trouble.
>
> And all of the above rules need to be documented in the kernel source tree
> or people will get confused.
Hi Rafael,
How about following comments for acpi_dev_filter_resource_type()?
Thanks!
Gerry
--------------------------------------------------------------------
/**
* According to ACPI specifications, Consumer/Producer flag in ACPI resource
* descriptor means:
* 1(CONSUMER): This device consumes this resource
* 0(PRODUCER): This device produces and consumes this resource
* But for ACPI PCI host bridge, it is interpreted in another way:
* 1(CONSUMER): PCI host bridge itself consumes the resource, such as
* IOPORT 0xCF8-0xCFF to access PCI configuraiton space.
* 0(PRODUCER): PCI host bridge provides this resource to its child
* bus and devices.
*
* So this is a specially designed helper function to support ACPI PCI host
* bridge and ACPI IOAPIC, and its usage should be limited to those specific
* scenarioso only. It filters ACPI resource descriptors as below:
* 1) If flag IORESOURCE_WINDOW is not specified, it's querying resources
* consumed by device. That is to return:
* a) ACPI resources without producer_consumer flag
* b) ACPI resources with producer_consumer flag setting to CONSUMER.
* 2) If flag IORESOURCE_WINDOW is specified, it's querying resources
provided
* by device. That is to return:
* a) ACPI resources with producer_consumer flag setting to PRODUCER.
* 3) But there's an exception. Some platforms, such as PC Engines APU.1C,
* report PCI host bridge resource provision by Memory32Fixed().
* Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=94221
* So a special flag IORESOURCE_MEM_8AND16BIT is used to work around this
* BIOS issue.
*/
>
>>> Another possible fix is to only ignore IO resource consumed by host
>>> bridge and keep IOMEM resource consumed by host bridge, please refer to:
>>> http://www.spinics.net/lists/linux-pci/msg39706.html
>>>
>>> Sample ACPI table are archived at:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>
>>> V2->V3:
>>> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
>>>
>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>> ---
>>> arch/x86/pci/acpi.c | 5 ++---
>>> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>> index e4695985f9de..8c4b1201f340 100644
>>> --- a/arch/x86/pci/acpi.c
>>> +++ b/arch/x86/pci/acpi.c
>>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>> info->bridge = device;
>>> ret = acpi_dev_get_resources(device, list,
>>> acpi_dev_filter_resource_type_cb,
>>> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
>>> if (ret < 0)
>>> dev_warn(&device->dev,
>>> "failed to parse _CRS method, error code %d\n", ret);
>>> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>> "no IO and memory resources present in _CRS\n");
>>> else
>>> resource_list_for_each_entry_safe(entry, tmp, list) {
>>> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
>>> - (entry->res->flags & IORESOURCE_DISABLED))
>>> + if (entry->res->flags & IORESOURCE_DISABLED)
>>> resource_list_destroy_entry(entry);
>>> else
>>> entry->res->name = info->name;
>>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>>> index 5589a6e2a023..e761a868bdba 100644
>>> --- a/drivers/acpi/resource.c
>>> +++ b/drivers/acpi/resource.c
>>> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
>>> }
>>> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>>
>>> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
>>> +{
>>> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
>>> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
>>> +}
>>> +
>>> /**
>>> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
>>> * types
>>> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>> case ACPI_RESOURCE_TYPE_MEMORY24:
>>> case ACPI_RESOURCE_TYPE_MEMORY32:
>>> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>> + /*
>>> + * These types of resource descriptor should be used to
>>> + * describe resource consumption instead of resource provision.
>>> + * But some platforms, such as PC Engines APU.1C, reports
>>> + * resource provision by Memory32Fixed(). Please refer to:
>>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
>>> + */
>>> type = IORESOURCE_MEM;
>>
>> I think this means these resources will be accepted regardless of whether
>> the caller is looking for Consumer or Producer resources. To preserve the
>> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
>> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
>>
>> But I don't think it matches the ACPI spec intent, so I'm not sure it's
>> right to do it for all devices.
>
> No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
> for all devices.
>
>>> break;
>>> case ACPI_RESOURCE_TYPE_IO:
>>> case ACPI_RESOURCE_TYPE_FIXED_IO:
>>> - type = IORESOURCE_IO;
>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>> + type = IORESOURCE_IO;
>>> break;
>>> case ACPI_RESOURCE_TYPE_IRQ:
>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>> + type = IORESOURCE_IRQ;
>>> + break;
>>> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>> - type = IORESOURCE_IRQ;
>>> + if (acpi_dev_match_producer_consumer(types,
>>> + ares->data.extended_irq.producer_consumer))
>>> + type = IORESOURCE_IRQ;
>>> break;
>>> case ACPI_RESOURCE_TYPE_DMA:
>>> case ACPI_RESOURCE_TYPE_FIXED_DMA:
>>> - type = IORESOURCE_DMA;
>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>> + type = IORESOURCE_DMA;
>>> break;
>>> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
>>> - type = IORESOURCE_REG;
>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>> + type = IORESOURCE_REG;
>>> break;
>>> case ACPI_RESOURCE_TYPE_ADDRESS16:
>>> case ACPI_RESOURCE_TYPE_ADDRESS32:
>>> case ACPI_RESOURCE_TYPE_ADDRESS64:
>>> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
>>> + if (!acpi_dev_match_producer_consumer(types,
>>> + ares->data.address.producer_consumer))
>>> + break;
>>> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
>>> type = IORESOURCE_MEM;
>>> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-08 5:48 ` Jiang Liu
@ 2015-04-08 23:44 ` Rafael J. Wysocki
2015-04-09 2:50 ` Jiang Liu
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-08 23:44 UTC (permalink / raw)
To: Jiang Liu
Cc: Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> > On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
> >> Hi Jiang,
> <snip>
> >>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> >>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
> >>
> >> We should assume it will eventually be used for all ACPI devices,
> >> shouldn't we?
> >
> > I'm not sure about that, really. In fact, I'd restrict its use to devices
> > types that actually can "produce" resources (ie. do not require the resources
> > to be provided by their ancestors or to be available from a global pool).
> >
> > Otherwise we're pretty much guaranteed to get into trouble.
> >
> > And all of the above rules need to be documented in the kernel source tree
> > or people will get confused.
> Hi Rafael,
> How about following comments for acpi_dev_filter_resource_type()?
> Thanks!
> Gerry
> --------------------------------------------------------------------
> /**
> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
> * descriptor means:
> * 1(CONSUMER): This device consumes this resource
> * 0(PRODUCER): This device produces and consumes this resource
> * But for ACPI PCI host bridge, it is interpreted in another way:
So first of all, this leads to a question: Why is it interpreted for ACPI PCI
host bridges differently?
Is it something we've figured out from experience, or is there a standard
mandating that?
> * 1(CONSUMER): PCI host bridge itself consumes the resource, such as
> * IOPORT 0xCF8-0xCFF to access PCI configuraiton space.
> * 0(PRODUCER): PCI host bridge provides this resource to its child
> * bus and devices.
> *
> * So this is a specially designed helper function to support ACPI PCI host
> * bridge and ACPI IOAPIC, and its usage should be limited to those specific
And this will make the reader wonder why the IOAPIC should be treated the same
way as a PCI host bridge in that respect.
> * scenarioso only. It filters ACPI resource descriptors as below:
> * 1) If flag IORESOURCE_WINDOW is not specified, it's querying resources
> * consumed by device. That is to return:
> * a) ACPI resources without producer_consumer flag
> * b) ACPI resources with producer_consumer flag setting to CONSUMER.
> * 2) If flag IORESOURCE_WINDOW is specified, it's querying resources
> provided
> * by device. That is to return:
> * a) ACPI resources with producer_consumer flag setting to PRODUCER.
> * 3) But there's an exception. Some platforms, such as PC Engines APU.1C,
> * report PCI host bridge resource provision by Memory32Fixed().
> * Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=94221
> * So a special flag IORESOURCE_MEM_8AND16BIT is used to work around this
> * BIOS issue.
> */
>
> >
> >>> Another possible fix is to only ignore IO resource consumed by host
> >>> bridge and keep IOMEM resource consumed by host bridge, please refer to:
> >>> http://www.spinics.net/lists/linux-pci/msg39706.html
> >>>
> >>> Sample ACPI table are archived at:
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
> >>>
> >>> V2->V3:
> >>> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
> >>>
> >>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> >>> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>> ---
> >>> arch/x86/pci/acpi.c | 5 ++---
> >>> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
> >>> 2 files changed, 31 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> >>> index e4695985f9de..8c4b1201f340 100644
> >>> --- a/arch/x86/pci/acpi.c
> >>> +++ b/arch/x86/pci/acpi.c
> >>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> >>> info->bridge = device;
> >>> ret = acpi_dev_get_resources(device, list,
> >>> acpi_dev_filter_resource_type_cb,
> >>> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> >>> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> >>> if (ret < 0)
> >>> dev_warn(&device->dev,
> >>> "failed to parse _CRS method, error code %d\n", ret);
> >>> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> >>> "no IO and memory resources present in _CRS\n");
> >>> else
> >>> resource_list_for_each_entry_safe(entry, tmp, list) {
> >>> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> >>> - (entry->res->flags & IORESOURCE_DISABLED))
> >>> + if (entry->res->flags & IORESOURCE_DISABLED)
> >>> resource_list_destroy_entry(entry);
> >>> else
> >>> entry->res->name = info->name;
> >>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> >>> index 5589a6e2a023..e761a868bdba 100644
> >>> --- a/drivers/acpi/resource.c
> >>> +++ b/drivers/acpi/resource.c
> >>> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> >>> }
> >>> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
> >>>
> >>> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
> >>> +{
> >>> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
> >>> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
> >>> +}
> >>> +
> >>> /**
> >>> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> >>> * types
> >>> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> >>> case ACPI_RESOURCE_TYPE_MEMORY24:
> >>> case ACPI_RESOURCE_TYPE_MEMORY32:
> >>> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> >>> + /*
> >>> + * These types of resource descriptor should be used to
> >>> + * describe resource consumption instead of resource provision.
> >>> + * But some platforms, such as PC Engines APU.1C, reports
> >>> + * resource provision by Memory32Fixed(). Please refer to:
> >>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
> >>> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
> >>> + */
> >>> type = IORESOURCE_MEM;
> >>
> >> I think this means these resources will be accepted regardless of whether
> >> the caller is looking for Consumer or Producer resources. To preserve the
> >> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
> >> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
> >>
> >> But I don't think it matches the ACPI spec intent, so I'm not sure it's
> >> right to do it for all devices.
> >
> > No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
> > for all devices.
> >
> >>> break;
> >>> case ACPI_RESOURCE_TYPE_IO:
> >>> case ACPI_RESOURCE_TYPE_FIXED_IO:
> >>> - type = IORESOURCE_IO;
> >>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> >>> + type = IORESOURCE_IO;
> >>> break;
> >>> case ACPI_RESOURCE_TYPE_IRQ:
> >>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> >>> + type = IORESOURCE_IRQ;
> >>> + break;
> >>> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> >>> - type = IORESOURCE_IRQ;
> >>> + if (acpi_dev_match_producer_consumer(types,
> >>> + ares->data.extended_irq.producer_consumer))
> >>> + type = IORESOURCE_IRQ;
> >>> break;
> >>> case ACPI_RESOURCE_TYPE_DMA:
> >>> case ACPI_RESOURCE_TYPE_FIXED_DMA:
> >>> - type = IORESOURCE_DMA;
> >>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> >>> + type = IORESOURCE_DMA;
> >>> break;
> >>> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
> >>> - type = IORESOURCE_REG;
> >>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
> >>> + type = IORESOURCE_REG;
> >>> break;
> >>> case ACPI_RESOURCE_TYPE_ADDRESS16:
> >>> case ACPI_RESOURCE_TYPE_ADDRESS32:
> >>> case ACPI_RESOURCE_TYPE_ADDRESS64:
> >>> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
> >>> + if (!acpi_dev_match_producer_consumer(types,
> >>> + ares->data.address.producer_consumer))
> >>> + break;
> >>> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
> >>> type = IORESOURCE_MEM;
> >>> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-08 23:44 ` Rafael J. Wysocki
@ 2015-04-09 2:50 ` Jiang Liu
2015-04-09 21:37 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Jiang Liu @ 2015-04-09 2:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On 2015/4/9 7:44, Rafael J. Wysocki wrote:
> On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>>>> Hi Jiang,
>> <snip>
>>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>>>
>>>> We should assume it will eventually be used for all ACPI devices,
>>>> shouldn't we?
>>>
>>> I'm not sure about that, really. In fact, I'd restrict its use to devices
>>> types that actually can "produce" resources (ie. do not require the resources
>>> to be provided by their ancestors or to be available from a global pool).
>>>
>>> Otherwise we're pretty much guaranteed to get into trouble.
>>>
>>> And all of the above rules need to be documented in the kernel source tree
>>> or people will get confused.
>> Hi Rafael,
>> How about following comments for acpi_dev_filter_resource_type()?
>> Thanks!
>> Gerry
>> --------------------------------------------------------------------
>> /**
>> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>> * descriptor means:
>> * 1(CONSUMER): This device consumes this resource
>> * 0(PRODUCER): This device produces and consumes this resource
>> * But for ACPI PCI host bridge, it is interpreted in another way:
>
> So first of all, this leads to a question: Why is it interpreted for ACPI PCI
> host bridges differently?
>
> Is it something we've figured out from experience, or is there a standard
> mandating that?
Hi Rafael,
I think we got this knowledge by experiences. PCI FW spec only
states _CRS reports resources assigned to the host bridge by firmware.
There's no statement about whether the resource is consumed by host
bridge itself or provided to it's child bus/devices. On x86/IA64 side,
the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
but not sure about ARM64 side. So how about:
PCI Firmware specification states that _CRS reports resources
assigned to the host bridge, but there's no way to tell whether
the resource is consumed by host bridge itself or provided to
its child bus/devices. So according to experiences, PCI host bridge
interprets Consumer/Producer flag as below to tell whether the resource
is consumed by host bridge itself.
>
>> * 1(CONSUMER): PCI host bridge itself consumes the resource, such as
>> * IOPORT 0xCF8-0xCFF to access PCI configuraiton space.
>> * 0(PRODUCER): PCI host bridge provides this resource to its child
>> * bus and devices.
>> *
>> * So this is a specially designed helper function to support ACPI PCI host
>> * bridge and ACPI IOAPIC, and its usage should be limited to those specific
>
> And this will make the reader wonder why the IOAPIC should be treated the same
> way as a PCI host bridge in that respect.
If a hot-pluggable IOAPIC is represented as an ACPI device, its _CRS
reports MMIO address assigned to the IOAPIC. And an IOAPIC device
won't produce MMIO resources by itself. So we could reuse
acpi_dev_filter_resource_type() here.
How about:
* So this is a specially designed helper function to support:
* 1) ACPI PCI host bridge, as explained above
* 2) ACPI IOAPIC, its _CRS reports only one MMIO resource and
* it won't produce MMIO resources by itself.
Thanks!
Gerry
>
>> * scenarioso only. It filters ACPI resource descriptors as below:
>> * 1) If flag IORESOURCE_WINDOW is not specified, it's querying resources
>> * consumed by device. That is to return:
>> * a) ACPI resources without producer_consumer flag
>> * b) ACPI resources with producer_consumer flag setting to CONSUMER.
>> * 2) If flag IORESOURCE_WINDOW is specified, it's querying resources
>> provided
>> * by device. That is to return:
>> * a) ACPI resources with producer_consumer flag setting to PRODUCER.
>> * 3) But there's an exception. Some platforms, such as PC Engines APU.1C,
>> * report PCI host bridge resource provision by Memory32Fixed().
>> * Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=94221
>> * So a special flag IORESOURCE_MEM_8AND16BIT is used to work around this
>> * BIOS issue.
>> */
>>
>>>
>>>>> Another possible fix is to only ignore IO resource consumed by host
>>>>> bridge and keep IOMEM resource consumed by host bridge, please refer to:
>>>>> http://www.spinics.net/lists/linux-pci/msg39706.html
>>>>>
>>>>> Sample ACPI table are archived at:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>>
>>>>> V2->V3:
>>>>> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
>>>>>
>>>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>>> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>>> ---
>>>>> arch/x86/pci/acpi.c | 5 ++---
>>>>> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
>>>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>>>> index e4695985f9de..8c4b1201f340 100644
>>>>> --- a/arch/x86/pci/acpi.c
>>>>> +++ b/arch/x86/pci/acpi.c
>>>>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>> info->bridge = device;
>>>>> ret = acpi_dev_get_resources(device, list,
>>>>> acpi_dev_filter_resource_type_cb,
>>>>> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>>>> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
>>>>> if (ret < 0)
>>>>> dev_warn(&device->dev,
>>>>> "failed to parse _CRS method, error code %d\n", ret);
>>>>> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>> "no IO and memory resources present in _CRS\n");
>>>>> else
>>>>> resource_list_for_each_entry_safe(entry, tmp, list) {
>>>>> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
>>>>> - (entry->res->flags & IORESOURCE_DISABLED))
>>>>> + if (entry->res->flags & IORESOURCE_DISABLED)
>>>>> resource_list_destroy_entry(entry);
>>>>> else
>>>>> entry->res->name = info->name;
>>>>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>>>>> index 5589a6e2a023..e761a868bdba 100644
>>>>> --- a/drivers/acpi/resource.c
>>>>> +++ b/drivers/acpi/resource.c
>>>>> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>>>>
>>>>> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
>>>>> +{
>>>>> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
>>>>> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
>>>>> * types
>>>>> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>>>> case ACPI_RESOURCE_TYPE_MEMORY24:
>>>>> case ACPI_RESOURCE_TYPE_MEMORY32:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>>>> + /*
>>>>> + * These types of resource descriptor should be used to
>>>>> + * describe resource consumption instead of resource provision.
>>>>> + * But some platforms, such as PC Engines APU.1C, reports
>>>>> + * resource provision by Memory32Fixed(). Please refer to:
>>>>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
>>>>> + */
>>>>> type = IORESOURCE_MEM;
>>>>
>>>> I think this means these resources will be accepted regardless of whether
>>>> the caller is looking for Consumer or Producer resources. To preserve the
>>>> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
>>>> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
>>>>
>>>> But I don't think it matches the ACPI spec intent, so I'm not sure it's
>>>> right to do it for all devices.
>>>
>>> No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
>>> for all devices.
>>>
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_IO:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_IO:
>>>>> - type = IORESOURCE_IO;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_IO;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_IRQ:
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_IRQ;
>>>>> + break;
>>>>> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>>>> - type = IORESOURCE_IRQ;
>>>>> + if (acpi_dev_match_producer_consumer(types,
>>>>> + ares->data.extended_irq.producer_consumer))
>>>>> + type = IORESOURCE_IRQ;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_DMA:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_DMA:
>>>>> - type = IORESOURCE_DMA;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_DMA;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
>>>>> - type = IORESOURCE_REG;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_REG;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS16:
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS32:
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS64:
>>>>> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
>>>>> + if (!acpi_dev_match_producer_consumer(types,
>>>>> + ares->data.address.producer_consumer))
>>>>> + break;
>>>>> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
>>>>> type = IORESOURCE_MEM;
>>>>> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-09 2:50 ` Jiang Liu
@ 2015-04-09 21:37 ` Rafael J. Wysocki
2015-04-09 22:00 ` Bjorn Helgaas
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-09 21:37 UTC (permalink / raw)
To: Jiang Liu
Cc: Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
> > On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
> >> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> >>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
> >>>> Hi Jiang,
> >> <snip>
> >>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> >>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
> >>>>
> >>>> We should assume it will eventually be used for all ACPI devices,
> >>>> shouldn't we?
> >>>
> >>> I'm not sure about that, really. In fact, I'd restrict its use to devices
> >>> types that actually can "produce" resources (ie. do not require the resources
> >>> to be provided by their ancestors or to be available from a global pool).
> >>>
> >>> Otherwise we're pretty much guaranteed to get into trouble.
> >>>
> >>> And all of the above rules need to be documented in the kernel source tree
> >>> or people will get confused.
> >> Hi Rafael,
> >> How about following comments for acpi_dev_filter_resource_type()?
> >> Thanks!
> >> Gerry
> >> --------------------------------------------------------------------
> >> /**
> >> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
> >> * descriptor means:
> >> * 1(CONSUMER): This device consumes this resource
> >> * 0(PRODUCER): This device produces and consumes this resource
> >> * But for ACPI PCI host bridge, it is interpreted in another way:
> >
> > So first of all, this leads to a question: Why is it interpreted for ACPI PCI
> > host bridges differently?
> >
> > Is it something we've figured out from experience, or is there a standard
> > mandating that?
> Hi Rafael,
> I think we got this knowledge by experiences. PCI FW spec only
> states _CRS reports resources assigned to the host bridge by firmware.
> There's no statement about whether the resource is consumed by host
> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
> but not sure about ARM64 side. So how about:
This:
> PCI Firmware specification states that _CRS reports resources
> assigned to the host bridge, but there's no way to tell whether
> the resource is consumed by host bridge itself or provided to
> its child bus/devices.
looks OK to me, but I'd replace the below with something like:
"However, experience shows, that in the PCI host bridge case firmware writers
expect the resource to be provided to devices on the bus (below the bridge) for
consumption entirely if its Consumer/Producer flag is clear. That expectation
is reflected by the code in this routine as follows:".
> So according to experiences, PCI host bridge
> interprets Consumer/Producer flag as below to tell whether the resource
> is consumed by host bridge itself.
>
> >
> >> * 1(CONSUMER): PCI host bridge itself consumes the resource, such as
> >> * IOPORT 0xCF8-0xCFF to access PCI configuraiton space.
> >> * 0(PRODUCER): PCI host bridge provides this resource to its child
> >> * bus and devices.
> >> *
> >> * So this is a specially designed helper function to support ACPI PCI host
> >> * bridge and ACPI IOAPIC, and its usage should be limited to those specific
> >
> > And this will make the reader wonder why the IOAPIC should be treated the same
> > way as a PCI host bridge in that respect.
Your responses would be easier to follow if they were clearly separate from the
original message text (ie. add an empty line between the paragraph you're
responding to and the response).
> If a hot-pluggable IOAPIC is represented as an ACPI device, its _CRS
> reports MMIO address assigned to the IOAPIC. And an IOAPIC device
> won't produce MMIO resources by itself. So we could reuse
> acpi_dev_filter_resource_type() here.
> How about:
> * So this is a specially designed helper function to support:
> * 1) ACPI PCI host bridge, as explained above
> * 2) ACPI IOAPIC, its _CRS reports only one MMIO resource and
> * it won't produce MMIO resources by itself.
OK, so I'd like to see how the code would look like if those two cases had their
own "filter" routines, if that's not a problem.
Rafael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-09 21:37 ` Rafael J. Wysocki
@ 2015-04-09 22:00 ` Bjorn Helgaas
2015-04-09 22:37 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2015-04-09 22:00 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jiang Liu, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
>> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
>> > On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>> >> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>> >>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>> >>>> Hi Jiang,
>> >> <snip>
>> >>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>> >>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>> >>>>
>> >>>> We should assume it will eventually be used for all ACPI devices,
>> >>>> shouldn't we?
>> >>>
>> >>> I'm not sure about that, really. In fact, I'd restrict its use to devices
>> >>> types that actually can "produce" resources (ie. do not require the resources
>> >>> to be provided by their ancestors or to be available from a global pool).
>> >>>
>> >>> Otherwise we're pretty much guaranteed to get into trouble.
>> >>>
>> >>> And all of the above rules need to be documented in the kernel source tree
>> >>> or people will get confused.
>> >> Hi Rafael,
>> >> How about following comments for acpi_dev_filter_resource_type()?
>> >> Thanks!
>> >> Gerry
>> >> --------------------------------------------------------------------
>> >> /**
>> >> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>> >> * descriptor means:
>> >> * 1(CONSUMER): This device consumes this resource
>> >> * 0(PRODUCER): This device produces and consumes this resource
>> >> * But for ACPI PCI host bridge, it is interpreted in another way:
>> >
>> > So first of all, this leads to a question: Why is it interpreted for ACPI PCI
>> > host bridges differently?
>> >
>> > Is it something we've figured out from experience, or is there a standard
>> > mandating that?
>> Hi Rafael,
>> I think we got this knowledge by experiences. PCI FW spec only
>> states _CRS reports resources assigned to the host bridge by firmware.
>> There's no statement about whether the resource is consumed by host
>> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
>> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
>> but not sure about ARM64 side. So how about:
>
> This:
>
>> PCI Firmware specification states that _CRS reports resources
>> assigned to the host bridge, but there's no way to tell whether
>> the resource is consumed by host bridge itself or provided to
>> its child bus/devices.
>
> looks OK to me, but I'd replace the below with something like:
>
> "However, experience shows, that in the PCI host bridge case firmware writers
> expect the resource to be provided to devices on the bus (below the bridge) for
> consumption entirely if its Consumer/Producer flag is clear. That expectation
> is reflected by the code in this routine as follows:".
What a mess. The spec is regrettably lacking in Consumer/Producer
specifics. But I don't see what's confusing about the descriptors
that have Consumer/Producer bits.
QWord, DWord, and Word descriptors don't seem to have a
Consumer/Producer bit, but they do contain _TRA, so they must be
intended for bridge windows. Can they also be used for device
registers? I don't know.
The Extended Address descriptor has a Consumer/Producer bit, and I
would interpret the spec to mean that if the flag is clear (the device
produces and consumes this resource), the resource is available on the
bus below the bridge, i.e., the bridge consumes the resource on its
upstream side and produces it on its downstream side. If the bit were
set (the device only consumes the resource), I would expect that to
mean the descriptor is for bridge registers like 0xcf8/0xcfc that
never appear on the downstream side.
Maybe I'm reading the spec too naively, but this doesn't seem a matter
of experience.
>> So according to experiences, PCI host bridge
>> interprets Consumer/Producer flag as below to tell whether the resource
>> is consumed by host bridge itself.
Bjorn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-09 22:00 ` Bjorn Helgaas
@ 2015-04-09 22:37 ` Rafael J. Wysocki
2015-04-10 0:28 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-09 22:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jiang Liu, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On Thursday, April 09, 2015 05:00:08 PM Bjorn Helgaas wrote:
> On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
> >> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
> >> > On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
> >> >> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> >> >>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
> >> >>>> Hi Jiang,
> >> >> <snip>
> >> >>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> >> >>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
> >> >>>>
> >> >>>> We should assume it will eventually be used for all ACPI devices,
> >> >>>> shouldn't we?
> >> >>>
> >> >>> I'm not sure about that, really. In fact, I'd restrict its use to devices
> >> >>> types that actually can "produce" resources (ie. do not require the resources
> >> >>> to be provided by their ancestors or to be available from a global pool).
> >> >>>
> >> >>> Otherwise we're pretty much guaranteed to get into trouble.
> >> >>>
> >> >>> And all of the above rules need to be documented in the kernel source tree
> >> >>> or people will get confused.
> >> >> Hi Rafael,
> >> >> How about following comments for acpi_dev_filter_resource_type()?
> >> >> Thanks!
> >> >> Gerry
> >> >> --------------------------------------------------------------------
> >> >> /**
> >> >> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
> >> >> * descriptor means:
> >> >> * 1(CONSUMER): This device consumes this resource
> >> >> * 0(PRODUCER): This device produces and consumes this resource
> >> >> * But for ACPI PCI host bridge, it is interpreted in another way:
> >> >
> >> > So first of all, this leads to a question: Why is it interpreted for ACPI PCI
> >> > host bridges differently?
> >> >
> >> > Is it something we've figured out from experience, or is there a standard
> >> > mandating that?
> >> Hi Rafael,
> >> I think we got this knowledge by experiences. PCI FW spec only
> >> states _CRS reports resources assigned to the host bridge by firmware.
> >> There's no statement about whether the resource is consumed by host
> >> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
> >> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
> >> but not sure about ARM64 side. So how about:
> >
> > This:
> >
> >> PCI Firmware specification states that _CRS reports resources
> >> assigned to the host bridge, but there's no way to tell whether
> >> the resource is consumed by host bridge itself or provided to
> >> its child bus/devices.
> >
> > looks OK to me, but I'd replace the below with something like:
> >
> > "However, experience shows, that in the PCI host bridge case firmware writers
> > expect the resource to be provided to devices on the bus (below the bridge) for
> > consumption entirely if its Consumer/Producer flag is clear. That expectation
> > is reflected by the code in this routine as follows:".
>
> What a mess. The spec is regrettably lacking in Consumer/Producer
> specifics. But I don't see what's confusing about the descriptors
> that have Consumer/Producer bits.
>
> QWord, DWord, and Word descriptors don't seem to have a
> Consumer/Producer bit, but they do contain _TRA, so they must be
> intended for bridge windows. Can they also be used for device
> registers? I don't know.
>
> The Extended Address descriptor has a Consumer/Producer bit, and I
> would interpret the spec to mean that if the flag is clear (the device
> produces and consumes this resource), the resource is available on the
> bus below the bridge, i.e., the bridge consumes the resource on its
> upstream side and produces it on its downstream side.
OK, that makes sense for bridges.
> If the bit were
> set (the device only consumes the resource), I would expect that to
> mean the descriptor is for bridge registers like 0xcf8/0xcfc that
> never appear on the downstream side.
That part is clear. What is not clear is whether or not we can *always*
expect the resources with Consumer/Producer *clear* to be produced on the
downstram side. That appears to be the case for host bridges if my
understanding of things is correct, but is it the case in general?
> Maybe I'm reading the spec too naively, but this doesn't seem a matter
> of experience.
Well, the specification is not clear enough in that respect, at least as
far as I can say, or maybe it is, but then does firmware always follow that
interpretation?
Rafael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-09 22:37 ` Rafael J. Wysocki
@ 2015-04-10 0:28 ` Rafael J. Wysocki
2015-04-13 4:31 ` Jiang Liu
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-10 0:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, Jiang Liu, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown, Lv Zheng, LKML, linux-pci,
linux-acpi
On Fri, Apr 10, 2015 at 12:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, April 09, 2015 05:00:08 PM Bjorn Helgaas wrote:
>> On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
>> >> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
>> >> > On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>> >> >> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>> >> >>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>> >> >>>> Hi Jiang,
>> >> >> <snip>
>> >> >>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>> >> >>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>> >> >>>>
>> >> >>>> We should assume it will eventually be used for all ACPI devices,
>> >> >>>> shouldn't we?
>> >> >>>
>> >> >>> I'm not sure about that, really. In fact, I'd restrict its use to devices
>> >> >>> types that actually can "produce" resources (ie. do not require the resources
>> >> >>> to be provided by their ancestors or to be available from a global pool).
>> >> >>>
>> >> >>> Otherwise we're pretty much guaranteed to get into trouble.
>> >> >>>
>> >> >>> And all of the above rules need to be documented in the kernel source tree
>> >> >>> or people will get confused.
>> >> >> Hi Rafael,
>> >> >> How about following comments for acpi_dev_filter_resource_type()?
>> >> >> Thanks!
>> >> >> Gerry
>> >> >> --------------------------------------------------------------------
>> >> >> /**
>> >> >> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>> >> >> * descriptor means:
>> >> >> * 1(CONSUMER): This device consumes this resource
>> >> >> * 0(PRODUCER): This device produces and consumes this resource
>> >> >> * But for ACPI PCI host bridge, it is interpreted in another way:
>> >> >
>> >> > So first of all, this leads to a question: Why is it interpreted for ACPI PCI
>> >> > host bridges differently?
>> >> >
>> >> > Is it something we've figured out from experience, or is there a standard
>> >> > mandating that?
>> >> Hi Rafael,
>> >> I think we got this knowledge by experiences. PCI FW spec only
>> >> states _CRS reports resources assigned to the host bridge by firmware.
>> >> There's no statement about whether the resource is consumed by host
>> >> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
>> >> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
>> >> but not sure about ARM64 side. So how about:
>> >
>> > This:
>> >
>> >> PCI Firmware specification states that _CRS reports resources
>> >> assigned to the host bridge, but there's no way to tell whether
>> >> the resource is consumed by host bridge itself or provided to
>> >> its child bus/devices.
>> >
>> > looks OK to me, but I'd replace the below with something like:
>> >
>> > "However, experience shows, that in the PCI host bridge case firmware writers
>> > expect the resource to be provided to devices on the bus (below the bridge) for
>> > consumption entirely if its Consumer/Producer flag is clear. That expectation
>> > is reflected by the code in this routine as follows:".
>>
>> What a mess. The spec is regrettably lacking in Consumer/Producer
>> specifics. But I don't see what's confusing about the descriptors
>> that have Consumer/Producer bits.
>>
>> QWord, DWord, and Word descriptors don't seem to have a
>> Consumer/Producer bit, but they do contain _TRA, so they must be
>> intended for bridge windows. Can they also be used for device
>> registers? I don't know.
>>
>> The Extended Address descriptor has a Consumer/Producer bit, and I
>> would interpret the spec to mean that if the flag is clear (the device
>> produces and consumes this resource), the resource is available on the
>> bus below the bridge, i.e., the bridge consumes the resource on its
>> upstream side and produces it on its downstream side.
>
> OK, that makes sense for bridges.
>
>> If the bit were
>> set (the device only consumes the resource), I would expect that to
>> mean the descriptor is for bridge registers like 0xcf8/0xcfc that
>> never appear on the downstream side.
>
> That part is clear. What is not clear is whether or not we can *always*
> expect the resources with Consumer/Producer *clear* to be produced on the
> downstram side. That appears to be the case for host bridges if my
> understanding of things is correct, but is it the case in general?
>
>> Maybe I'm reading the spec too naively, but this doesn't seem a matter
>> of experience.
>
> Well, the specification is not clear enough in that respect, at least as
> far as I can say, or maybe it is, but then does firmware always follow that
> interpretation?
So I guess I'd like to propose to go back to the 3.19 behavior for PCI host
bridges and then to handle the IOAPIC as a separate case.
We can think about consolidating the two later.
Any objections to doing that?
Rafael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-10 0:28 ` Rafael J. Wysocki
@ 2015-04-13 4:31 ` Jiang Liu
2015-04-13 6:38 ` [Bugfix v5] " Jiang Liu
2015-04-13 12:04 ` [Bugfix v3] " Rafael J. Wysocki
0 siblings, 2 replies; 17+ messages in thread
From: Jiang Liu @ 2015-04-13 4:31 UTC (permalink / raw)
To: Rafael J. Wysocki, Rafael J. Wysocki
Cc: Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Len Brown, Lv Zheng, LKML, linux-pci, linux-acpi
On 2015/4/10 8:28, Rafael J. Wysocki wrote:
> On Fri, Apr 10, 2015 at 12:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, April 09, 2015 05:00:08 PM Bjorn Helgaas wrote:
>>> On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
>>>>> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
>>>>>> On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>>>>>>> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>>>>>>>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>>>>>>>>> Hi Jiang,
>>>>>>> <snip>
>>>>>>>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>>>>>>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>>>>>>>>
>>>>>>>>> We should assume it will eventually be used for all ACPI devices,
>>>>>>>>> shouldn't we?
>>>>>>>>
>>>>>>>> I'm not sure about that, really. In fact, I'd restrict its use to devices
>>>>>>>> types that actually can "produce" resources (ie. do not require the resources
>>>>>>>> to be provided by their ancestors or to be available from a global pool).
>>>>>>>>
>>>>>>>> Otherwise we're pretty much guaranteed to get into trouble.
>>>>>>>>
>>>>>>>> And all of the above rules need to be documented in the kernel source tree
>>>>>>>> or people will get confused.
>>>>>>> Hi Rafael,
>>>>>>> How about following comments for acpi_dev_filter_resource_type()?
>>>>>>> Thanks!
>>>>>>> Gerry
>>>>>>> --------------------------------------------------------------------
>>>>>>> /**
>>>>>>> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>>>>>>> * descriptor means:
>>>>>>> * 1(CONSUMER): This device consumes this resource
>>>>>>> * 0(PRODUCER): This device produces and consumes this resource
>>>>>>> * But for ACPI PCI host bridge, it is interpreted in another way:
>>>>>>
>>>>>> So first of all, this leads to a question: Why is it interpreted for ACPI PCI
>>>>>> host bridges differently?
>>>>>>
>>>>>> Is it something we've figured out from experience, or is there a standard
>>>>>> mandating that?
>>>>> Hi Rafael,
>>>>> I think we got this knowledge by experiences. PCI FW spec only
>>>>> states _CRS reports resources assigned to the host bridge by firmware.
>>>>> There's no statement about whether the resource is consumed by host
>>>>> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
>>>>> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
>>>>> but not sure about ARM64 side. So how about:
>>>>
>>>> This:
>>>>
>>>>> PCI Firmware specification states that _CRS reports resources
>>>>> assigned to the host bridge, but there's no way to tell whether
>>>>> the resource is consumed by host bridge itself or provided to
>>>>> its child bus/devices.
>>>>
>>>> looks OK to me, but I'd replace the below with something like:
>>>>
>>>> "However, experience shows, that in the PCI host bridge case firmware writers
>>>> expect the resource to be provided to devices on the bus (below the bridge) for
>>>> consumption entirely if its Consumer/Producer flag is clear. That expectation
>>>> is reflected by the code in this routine as follows:".
>>>
>>> What a mess. The spec is regrettably lacking in Consumer/Producer
>>> specifics. But I don't see what's confusing about the descriptors
>>> that have Consumer/Producer bits.
>>>
>>> QWord, DWord, and Word descriptors don't seem to have a
>>> Consumer/Producer bit, but they do contain _TRA, so they must be
>>> intended for bridge windows. Can they also be used for device
>>> registers? I don't know.
>>>
>>> The Extended Address descriptor has a Consumer/Producer bit, and I
>>> would interpret the spec to mean that if the flag is clear (the device
>>> produces and consumes this resource), the resource is available on the
>>> bus below the bridge, i.e., the bridge consumes the resource on its
>>> upstream side and produces it on its downstream side.
>>
>> OK, that makes sense for bridges.
>>> If the bit were
>>> set (the device only consumes the resource), I would expect that to
>>> mean the descriptor is for bridge registers like 0xcf8/0xcfc that
>>> never appear on the downstream side.
>>
>> That part is clear. What is not clear is whether or not we can *always*
>> expect the resources with Consumer/Producer *clear* to be produced on the
>> downstram side. That appears to be the case for host bridges if my
>> understanding of things is correct, but is it the case in general?
>>
>>> Maybe I'm reading the spec too naively, but this doesn't seem a matter
>>> of experience.
>>
>> Well, the specification is not clear enough in that respect, at least as
>> far as I can say, or maybe it is, but then does firmware always follow that
>> interpretation?
>
> So I guess I'd like to propose to go back to the 3.19 behavior for PCI host
> bridges and then to handle the IOAPIC as a separate case.
>
> We can think about consolidating the two later.
>
> Any objections to doing that?
Hi Rafael,
When reverting to the behavior before v3.19, I found a simpler
solution. The logic before v3.19 is:
on x86 and IA64, all IO port and MMIO resources assigned to PCI host
bridge are available to child bus/devices, except one special case to
filter out IO port[0xCF8-0xCFF] for PCI configuration space access.
And with pre-v3.19 kernels, all IO port defined by IO and fixed_IO
resource descriptors are ignored to filter out IO port[0xCF8-0xCFF].
So I plan to make following change to revert to the behavior before
v3.19:
1) make acpi_dev_filter_resource_type() filter descriptors based on
descriptor type, and don't check consumer_producer flag anymore.
2) use IORESOURCE_IO_FIXED flag to filter out io and fixed_io resource
descriptors.
3) x86 ACPI pci host bridge driver calls acpi_dev_filter_resource_type()
with flag IORESOURCE_IO_FIXED,
By this way, we could keep acpi_dev_filter_resource_type()
as a generic interface and could be reused. And the change
is small too. Any comments?
Thanks!
Gerry
> Rafael
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-13 4:31 ` Jiang Liu
@ 2015-04-13 6:38 ` Jiang Liu
2015-04-13 6:56 ` Ingo Molnar
2015-04-13 12:04 ` [Bugfix v3] " Rafael J. Wysocki
1 sibling, 1 reply; 17+ messages in thread
From: Jiang Liu @ 2015-04-13 6:38 UTC (permalink / raw)
To: Rafael J . Wysocki, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown
Cc: Jiang Liu, Lv Zheng, LKML, linux-pci, linux-acpi
Before commit 593669c2ac0f("Use common ACPI resource interfaces to
simplify implementation"), arch/x86/pci/acpi.c applies following
rules when parsing ACPI resources for PCI host bridge:
1) Ignore IO port resources defined by acpi_resource_io and
acpi_resource_fixed_io, which should be used to define resource
for PCI device instead of PCI bridge.
2) Accept IOMEM resource defined by acpi_resource_memory24,
acpi_resource_memory32 and acpi_resource_fixed_memory32.
These IOMEM resources are accepted to workaround some BIOS issue,
though they should be ignored. For example, PC Engines APU.1C
platform defines PCI host bridge IOMEM resources as:
Memory32Fixed (ReadOnly,
0x000A0000, // Address Base
0x00020000, // Address Length
)
Memory32Fixed (ReadOnly,
0x00000000, // Address Base
0x00000000, // Address Length
_Y00)
3) Accept all IO port and IOMEM resources defined by
acpi_resource_address{16,32,64,extended64}, no matter it's marked as
ACPI_CONSUMER or ACPI_PRODUCER.
Commit 593669c2ac0f("Use common ACPI resource interfaces to
simplify implementation") accept all IO port and IOMEM resources
defined by acpi_resource_io, acpi_resource_fixed_io,
acpi_resource_memory24, acpi_resource_memory32,
acpi_resource_fixed_memory32 and
acpi_resource_address{16,32,64,extended64}, which causes IO port
resources consumed by host bridge itself are listed in to host bridge
resource list.
Then commit 63f1789ec716("Ignore resources consumed by host bridge
itself") ignores resources consumed by host bridge itself by checking
IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
above for BIOS bug .
So revert to the behavior before v3.19 to fix the regression.
Sample ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221
Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
arch/x86/pci/acpi.c | 25 ++++++++++++++++++++++---
drivers/acpi/resource.c | 6 +++++-
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..fc2da98985c3 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
{
int ret;
struct resource_entry *entry, *tmp;
+ unsigned long res_flags;
sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
info->bridge = device;
+
+ /*
+ * An IO or MMIO resource assigned to PCI host bridge may be consumed
+ * by the host bridge itself or available to its child bus/devices.
+ * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
+ * be available to child bus/devices except one special case:
+ * IO port [0xCF8-0xCFF] is consumed by host bridge itself to
+ * access PCI configuration space.
+ *
+ * Due to lack of specification to define resources consumed by host
+ * bridge itself, all IO port resources defined by acpi_resource_io
+ * and acpi_resource_fixed_io are ignored to filter out IO
+ * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
+ * it's not perfect.
+ *
+ * Another possible solution is to explicitly filter out IO
+ * port[0xCF8-0xCFF].
+ */
+ res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
ret = acpi_dev_get_resources(device, list,
acpi_dev_filter_resource_type_cb,
- (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+ (void *)res_flags);
if (ret < 0)
dev_warn(&device->dev,
"failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +366,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
"no IO and memory resources present in _CRS\n");
else
resource_list_for_each_entry_safe(entry, tmp, list) {
- if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
- (entry->res->flags & IORESOURCE_DISABLED))
+ if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
entry->res->name = info->name;
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 5589a6e2a023..79b6d3b5ffd2 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
*
* This is a hepler function to support acpi_dev_get_resources(), which filters
* ACPI resource objects according to resource types.
+ *
+ * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
+ * descriptors for ACPI host bridges on x86 and IA64 platforms.
*/
int acpi_dev_filter_resource_type(struct acpi_resource *ares,
unsigned long types)
@@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
break;
case ACPI_RESOURCE_TYPE_IO:
case ACPI_RESOURCE_TYPE_FIXED_IO:
- type = IORESOURCE_IO;
+ if ((types & IORESOURCE_IO_FIXED) == 0)
+ type = IORESOURCE_IO;
break;
case ACPI_RESOURCE_TYPE_IRQ:
case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
--
1.7.10.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-13 6:38 ` [Bugfix v5] " Jiang Liu
@ 2015-04-13 6:56 ` Ingo Molnar
2015-04-13 12:05 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Ingo Molnar @ 2015-04-13 6:56 UTC (permalink / raw)
To: Jiang Liu
Cc: Rafael J . Wysocki, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown, Lv Zheng, LKML, linux-pci,
linux-acpi
* Jiang Liu <jiang.liu@linux.intel.com> wrote:
> Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation"), arch/x86/pci/acpi.c applies following
s/applies following
/applied the following
> rules when parsing ACPI resources for PCI host bridge:
> 1) Ignore IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io, which should be used to define resource
> for PCI device instead of PCI bridge.
> 2) Accept IOMEM resource defined by acpi_resource_memory24,
> acpi_resource_memory32 and acpi_resource_fixed_memory32.
> These IOMEM resources are accepted to workaround some BIOS issue,
s/workaround
/work around
It's a verb, not a noun.
> though they should be ignored. For example, PC Engines APU.1C
Please put the platform name into quotes to make it more clearly stand
apart from the rest of the text.
> platform defines PCI host bridge IOMEM resources as:
> Memory32Fixed (ReadOnly,
> 0x000A0000, // Address Base
> 0x00020000, // Address Length
> )
> Memory32Fixed (ReadOnly,
> 0x00000000, // Address Base
> 0x00000000, // Address Length
> _Y00)
> 3) Accept all IO port and IOMEM resources defined by
> acpi_resource_address{16,32,64,extended64}, no matter it's marked as
> ACPI_CONSUMER or ACPI_PRODUCER.
s/no matter it's marked
/regardless of whether it's marked
> Commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation") accept all IO port and IOMEM resources
s/accept
/started accepting
> defined by acpi_resource_io, acpi_resource_fixed_io,
> acpi_resource_memory24, acpi_resource_memory32,
> acpi_resource_fixed_memory32 and
> acpi_resource_address{16,32,64,extended64}, which causes IO port
> resources consumed by host bridge itself are listed in to host bridge
> resource list.
this latter part of the sentence does not parse. Did you want to
write:
acpi_resource_address{16,32,64,extended64}, which causes IO port
resources consumed by the host bridge itself to be listed in the
host bridge resource list.
?
> Then commit 63f1789ec716("Ignore resources consumed by host bridge
> itself") ignores resources consumed by host bridge itself by checking
s/ignores
/started ignoring
s/by host bridge
/by the host bridge
> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> above for BIOS bug .
s/for BIOS bug .
/for the BIOS bug.
> So revert to the behavior before v3.19 to fix the regression.
>
> Sample ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>
> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> arch/x86/pci/acpi.c | 25 ++++++++++++++++++++++---
> drivers/acpi/resource.c | 6 +++++-
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..fc2da98985c3 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
> {
> int ret;
> struct resource_entry *entry, *tmp;
> + unsigned long res_flags;
>
> sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
> info->bridge = device;
> +
> + /*
> + * An IO or MMIO resource assigned to PCI host bridge may be consumed
s/to PCI host bridge
/to a PCI host bridge
> + * by the host bridge itself or available to its child bus/devices.
> + * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> + * be available to child bus/devices except one special case:
> + * IO port [0xCF8-0xCFF] is consumed by host bridge itself to
s/by host bridge
/by the host bridge
> + * access PCI configuration space.
> + *
> + * Due to lack of specification to define resources consumed by host
> + * bridge itself, all IO port resources defined by acpi_resource_io
s/by host bridge
/by the host bridge
> + * and acpi_resource_fixed_io are ignored to filter out IO
> + * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> + * it's not perfect.
> + *
> + * Another possible solution is to explicitly filter out IO
s/is to
/would be to
> + * port[0xCF8-0xCFF].
> + */
> + res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
> ret = acpi_dev_get_resources(device, list,
> acpi_dev_filter_resource_type_cb,
> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> + (void *)res_flags);
> if (ret < 0)
> dev_warn(&device->dev,
> "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +366,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> "no IO and memory resources present in _CRS\n");
> else
> resource_list_for_each_entry_safe(entry, tmp, list) {
> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> - (entry->res->flags & IORESOURCE_DISABLED))
> + if (entry->res->flags & IORESOURCE_DISABLED)
> resource_list_destroy_entry(entry);
> else
> entry->res->name = info->name;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 5589a6e2a023..79b6d3b5ffd2 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
> *
> * This is a hepler function to support acpi_dev_get_resources(), which filters
> * ACPI resource objects according to resource types.
> + *
> + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
s/Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
The IORESOURCE_IO_FIXED flag is used to opt out IO and FIXED_IO resource
> + * descriptors for ACPI host bridges on x86 and IA64 platforms.
> */
> int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> unsigned long types)
> @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> break;
> case ACPI_RESOURCE_TYPE_IO:
> case ACPI_RESOURCE_TYPE_FIXED_IO:
> - type = IORESOURCE_IO;
> + if ((types & IORESOURCE_IO_FIXED) == 0)
> + type = IORESOURCE_IO;
> break;
> case ACPI_RESOURCE_TYPE_IRQ:
> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> --
> 1.7.10.4
>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-13 4:31 ` Jiang Liu
2015-04-13 6:38 ` [Bugfix v5] " Jiang Liu
@ 2015-04-13 12:04 ` Rafael J. Wysocki
1 sibling, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-13 12:04 UTC (permalink / raw)
To: Jiang Liu
Cc: Rafael J. Wysocki, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown, Lv Zheng, LKML, linux-pci,
linux-acpi
On Monday, April 13, 2015 12:31:20 PM Jiang Liu wrote:
> On 2015/4/10 8:28, Rafael J. Wysocki wrote:
> > On Fri, Apr 10, 2015 at 12:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> On Thursday, April 09, 2015 05:00:08 PM Bjorn Helgaas wrote:
> >>> On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>>> On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
> >>>>> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
> >>>>>> On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
> >>>>>>> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
> >>>>>>>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
> >>>>>>>>> Hi Jiang,
> >>>>>>> <snip>
> >>>>>>>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
> >>>>>>>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
> >>>>>>>>>
> >>>>>>>>> We should assume it will eventually be used for all ACPI devices,
> >>>>>>>>> shouldn't we?
> >>>>>>>>
> >>>>>>>> I'm not sure about that, really. In fact, I'd restrict its use to devices
> >>>>>>>> types that actually can "produce" resources (ie. do not require the resources
> >>>>>>>> to be provided by their ancestors or to be available from a global pool).
> >>>>>>>>
> >>>>>>>> Otherwise we're pretty much guaranteed to get into trouble.
> >>>>>>>>
> >>>>>>>> And all of the above rules need to be documented in the kernel source tree
> >>>>>>>> or people will get confused.
> >>>>>>> Hi Rafael,
> >>>>>>> How about following comments for acpi_dev_filter_resource_type()?
> >>>>>>> Thanks!
> >>>>>>> Gerry
> >>>>>>> --------------------------------------------------------------------
> >>>>>>> /**
> >>>>>>> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
> >>>>>>> * descriptor means:
> >>>>>>> * 1(CONSUMER): This device consumes this resource
> >>>>>>> * 0(PRODUCER): This device produces and consumes this resource
> >>>>>>> * But for ACPI PCI host bridge, it is interpreted in another way:
> >>>>>>
> >>>>>> So first of all, this leads to a question: Why is it interpreted for ACPI PCI
> >>>>>> host bridges differently?
> >>>>>>
> >>>>>> Is it something we've figured out from experience, or is there a standard
> >>>>>> mandating that?
> >>>>> Hi Rafael,
> >>>>> I think we got this knowledge by experiences. PCI FW spec only
> >>>>> states _CRS reports resources assigned to the host bridge by firmware.
> >>>>> There's no statement about whether the resource is consumed by host
> >>>>> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
> >>>>> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
> >>>>> but not sure about ARM64 side. So how about:
> >>>>
> >>>> This:
> >>>>
> >>>>> PCI Firmware specification states that _CRS reports resources
> >>>>> assigned to the host bridge, but there's no way to tell whether
> >>>>> the resource is consumed by host bridge itself or provided to
> >>>>> its child bus/devices.
> >>>>
> >>>> looks OK to me, but I'd replace the below with something like:
> >>>>
> >>>> "However, experience shows, that in the PCI host bridge case firmware writers
> >>>> expect the resource to be provided to devices on the bus (below the bridge) for
> >>>> consumption entirely if its Consumer/Producer flag is clear. That expectation
> >>>> is reflected by the code in this routine as follows:".
> >>>
> >>> What a mess. The spec is regrettably lacking in Consumer/Producer
> >>> specifics. But I don't see what's confusing about the descriptors
> >>> that have Consumer/Producer bits.
> >>>
> >>> QWord, DWord, and Word descriptors don't seem to have a
> >>> Consumer/Producer bit, but they do contain _TRA, so they must be
> >>> intended for bridge windows. Can they also be used for device
> >>> registers? I don't know.
> >>>
> >>> The Extended Address descriptor has a Consumer/Producer bit, and I
> >>> would interpret the spec to mean that if the flag is clear (the device
> >>> produces and consumes this resource), the resource is available on the
> >>> bus below the bridge, i.e., the bridge consumes the resource on its
> >>> upstream side and produces it on its downstream side.
> >>
> >> OK, that makes sense for bridges.
>
> >>> If the bit were
> >>> set (the device only consumes the resource), I would expect that to
> >>> mean the descriptor is for bridge registers like 0xcf8/0xcfc that
> >>> never appear on the downstream side.
> >>
> >> That part is clear. What is not clear is whether or not we can *always*
> >> expect the resources with Consumer/Producer *clear* to be produced on the
> >> downstram side. That appears to be the case for host bridges if my
> >> understanding of things is correct, but is it the case in general?
> >>
> >>> Maybe I'm reading the spec too naively, but this doesn't seem a matter
> >>> of experience.
> >>
> >> Well, the specification is not clear enough in that respect, at least as
> >> far as I can say, or maybe it is, but then does firmware always follow that
> >> interpretation?
> >
> > So I guess I'd like to propose to go back to the 3.19 behavior for PCI host
> > bridges and then to handle the IOAPIC as a separate case.
> >
> > We can think about consolidating the two later.
> >
> > Any objections to doing that?
> Hi Rafael,
> When reverting to the behavior before v3.19, I found a simpler
> solution. The logic before v3.19 is:
> on x86 and IA64, all IO port and MMIO resources assigned to PCI host
> bridge are available to child bus/devices, except one special case to
> filter out IO port[0xCF8-0xCFF] for PCI configuration space access.
>
> And with pre-v3.19 kernels, all IO port defined by IO and fixed_IO
> resource descriptors are ignored to filter out IO port[0xCF8-0xCFF].
>
> So I plan to make following change to revert to the behavior before
> v3.19:
> 1) make acpi_dev_filter_resource_type() filter descriptors based on
> descriptor type, and don't check consumer_producer flag anymore.
> 2) use IORESOURCE_IO_FIXED flag to filter out io and fixed_io resource
> descriptors.
> 3) x86 ACPI pci host bridge driver calls acpi_dev_filter_resource_type()
> with flag IORESOURCE_IO_FIXED,
>
> By this way, we could keep acpi_dev_filter_resource_type()
> as a generic interface and could be reused. And the change
> is small too. Any comments?
Sounds good to me.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
2015-04-13 6:56 ` Ingo Molnar
@ 2015-04-13 12:05 ` Rafael J. Wysocki
0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2015-04-13 12:05 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jiang Liu, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Len Brown, Lv Zheng, LKML, linux-pci,
linux-acpi
On Monday, April 13, 2015 08:56:17 AM Ingo Molnar wrote:
>
> * Jiang Liu <jiang.liu@linux.intel.com> wrote:
>
> > Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation"), arch/x86/pci/acpi.c applies following
>
> s/applies following
> /applied the following
>
> > rules when parsing ACPI resources for PCI host bridge:
> > 1) Ignore IO port resources defined by acpi_resource_io and
> > acpi_resource_fixed_io, which should be used to define resource
> > for PCI device instead of PCI bridge.
> > 2) Accept IOMEM resource defined by acpi_resource_memory24,
> > acpi_resource_memory32 and acpi_resource_fixed_memory32.
> > These IOMEM resources are accepted to workaround some BIOS issue,
>
> s/workaround
> /work around
>
> It's a verb, not a noun.
Agreed on this one an all of the points below.
> > though they should be ignored. For example, PC Engines APU.1C
>
> Please put the platform name into quotes to make it more clearly stand
> apart from the rest of the text.
>
> > platform defines PCI host bridge IOMEM resources as:
> > Memory32Fixed (ReadOnly,
> > 0x000A0000, // Address Base
> > 0x00020000, // Address Length
> > )
> > Memory32Fixed (ReadOnly,
> > 0x00000000, // Address Base
> > 0x00000000, // Address Length
> > _Y00)
> > 3) Accept all IO port and IOMEM resources defined by
> > acpi_resource_address{16,32,64,extended64}, no matter it's marked as
> > ACPI_CONSUMER or ACPI_PRODUCER.
>
> s/no matter it's marked
> /regardless of whether it's marked
>
> > Commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation") accept all IO port and IOMEM resources
>
> s/accept
> /started accepting
>
> > defined by acpi_resource_io, acpi_resource_fixed_io,
> > acpi_resource_memory24, acpi_resource_memory32,
> > acpi_resource_fixed_memory32 and
> > acpi_resource_address{16,32,64,extended64}, which causes IO port
> > resources consumed by host bridge itself are listed in to host bridge
> > resource list.
>
> this latter part of the sentence does not parse. Did you want to
> write:
>
> acpi_resource_address{16,32,64,extended64}, which causes IO port
> resources consumed by the host bridge itself to be listed in the
> host bridge resource list.
>
> ?
>
> > Then commit 63f1789ec716("Ignore resources consumed by host bridge
> > itself") ignores resources consumed by host bridge itself by checking
>
> s/ignores
> /started ignoring
>
> s/by host bridge
> /by the host bridge
>
> > IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> > above for BIOS bug .
>
> s/for BIOS bug .
> /for the BIOS bug.
>
> > So revert to the behavior before v3.19 to fix the regression.
> >
> > Sample ACPI table are archived at:
> > https://bugzilla.kernel.org/show_bug.cgi?id=94221
> >
> > Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> > Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > ---
> > arch/x86/pci/acpi.c | 25 ++++++++++++++++++++++---
> > drivers/acpi/resource.c | 6 +++++-
> > 2 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index e4695985f9de..fc2da98985c3 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
> > {
> > int ret;
> > struct resource_entry *entry, *tmp;
> > + unsigned long res_flags;
> >
> > sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
> > info->bridge = device;
> > +
> > + /*
> > + * An IO or MMIO resource assigned to PCI host bridge may be consumed
>
> s/to PCI host bridge
> /to a PCI host bridge
>
> > + * by the host bridge itself or available to its child bus/devices.
> > + * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> > + * be available to child bus/devices except one special case:
> > + * IO port [0xCF8-0xCFF] is consumed by host bridge itself to
>
> s/by host bridge
> /by the host bridge
>
> > + * access PCI configuration space.
> > + *
> > + * Due to lack of specification to define resources consumed by host
> > + * bridge itself, all IO port resources defined by acpi_resource_io
>
> s/by host bridge
> /by the host bridge
>
> > + * and acpi_resource_fixed_io are ignored to filter out IO
> > + * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> > + * it's not perfect.
> > + *
> > + * Another possible solution is to explicitly filter out IO
>
> s/is to
> /would be to
>
> > + * port[0xCF8-0xCFF].
> > + */
> > + res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
> > ret = acpi_dev_get_resources(device, list,
> > acpi_dev_filter_resource_type_cb,
> > - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> > + (void *)res_flags);
> > if (ret < 0)
> > dev_warn(&device->dev,
> > "failed to parse _CRS method, error code %d\n", ret);
> > @@ -346,8 +366,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
> > "no IO and memory resources present in _CRS\n");
> > else
> > resource_list_for_each_entry_safe(entry, tmp, list) {
> > - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> > - (entry->res->flags & IORESOURCE_DISABLED))
> > + if (entry->res->flags & IORESOURCE_DISABLED)
> > resource_list_destroy_entry(entry);
> > else
> > entry->res->name = info->name;
> > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > index 5589a6e2a023..79b6d3b5ffd2 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
> > *
> > * This is a hepler function to support acpi_dev_get_resources(), which filters
> > * ACPI resource objects according to resource types.
> > + *
> > + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
>
> s/Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
> The IORESOURCE_IO_FIXED flag is used to opt out IO and FIXED_IO resource
>
> > + * descriptors for ACPI host bridges on x86 and IA64 platforms.
> > */
> > int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> > unsigned long types)
> > @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> > break;
> > case ACPI_RESOURCE_TYPE_IO:
> > case ACPI_RESOURCE_TYPE_FIXED_IO:
> > - type = IORESOURCE_IO;
> > + if ((types & IORESOURCE_IO_FIXED) == 0)
> > + type = IORESOURCE_IO;
> > break;
> > case ACPI_RESOURCE_TYPE_IRQ:
> > case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-04-13 12:04 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-30 2:40 [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716 Jiang Liu
2015-04-03 11:49 ` Rafael J. Wysocki
2015-04-04 3:04 ` Bjorn Helgaas
2015-04-07 0:28 ` Rafael J. Wysocki
2015-04-07 13:30 ` Jiang Liu
2015-04-08 5:48 ` Jiang Liu
2015-04-08 23:44 ` Rafael J. Wysocki
2015-04-09 2:50 ` Jiang Liu
2015-04-09 21:37 ` Rafael J. Wysocki
2015-04-09 22:00 ` Bjorn Helgaas
2015-04-09 22:37 ` Rafael J. Wysocki
2015-04-10 0:28 ` Rafael J. Wysocki
2015-04-13 4:31 ` Jiang Liu
2015-04-13 6:38 ` [Bugfix v5] " Jiang Liu
2015-04-13 6:56 ` Ingo Molnar
2015-04-13 12:05 ` Rafael J. Wysocki
2015-04-13 12:04 ` [Bugfix v3] " Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.