Skip to contentSkip to conversation

x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

PATCH Superseded Jiang Liu 3 participants 10 messages Apr 20, 2015 – Apr 30, 2015

Subsystems (3)
x86 ACPI PCI
lore
Share & tools
lore

Jiang Liu

author
#0 lore raw

PATCH v5 1/1 x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +22 -3
index e4695985f9de..fc2da98985c3 100644
@@ -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;
drivers/acpi/resource.c +5 -1
index 5589a6e2a023..79b6d3b5ffd2 100644
@@ -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:
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 33 lines · Jiang Liu

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Quoted 6 lines

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>

Bjorn, Ingo, is anyone looking at this? We're still having a regression in this area ...

Quoted 78 lines
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +22 -3
index e4695985f9de..fc2da98985c3 100644
@@ -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;
drivers/acpi/resource.c +5 -1
index 5589a6e2a023..79b6d3b5ffd2 100644
@@ -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:
Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
maintainer
#2 lore raw
Quoted 59 lines · Rafael J. Wysocki
Quoted 33 lines · Jiang Liu

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Quoted 6 lines

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>

Bjorn, Ingo, is anyone looking at this? We're still having a regression in this area ...

Quoted 15 lines
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +1 -0
index e4695985f9de..fc2da98985c3 100644
@@ -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;
Quoted 16 lines
Quoted 6 lines
arch/x86/pci/acpi.c +4 -0
      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.

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 352

Quoted 10 lines
arch/x86/pci/acpi.c +10 -0
+      * 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.

1) I think it's misleading to say "the specs haven't provided a method." As far as I can tell, the Producer/Consumer bit is intended precisely to distinguish resources consumed by a bridge from those forwarded to downstream devices. It would be more accurate to say "the spec defines a bit, but firmware hasn't used that bit consistently, so we can't rely on it."

If you want to say "it's not perfect," it would be useful to mention the ways in which it is not perfect. This code is still a candidate for unification with ia64 and arm64, so we should avoid x86-specific things here as much as possible.

Quoted 5 lines

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 357

Quoted 5 lines
arch/x86/pci/acpi.c +5 -0
+      *
+      * Another possible solution is to explicitly filter out IO
+      * port[0xCF8-0xCFF].
+      */
+     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;

2) The usage of IORESOURCE_IO_FIXED here seems like a hack. It's not related to other existing use of IORESOURCE_IO_FIXED, and it's not intuitive that supplying IORESOURCE_MEM means "I want all the memory-type resources," but supplying IORESOURCE_IO_FIXED means "I *don't* want the fixed I/O-type resources."

A struct resource is not as expressive as a struct acpi_resource. We're going through a lot of contortions to pass nuances of acpi_resource through to code that only knows about struct resource. I'm not sure that's a good long-term strategy, but I don't have a better suggestion.

Quoted 17 lines
Quoted 17 lines
arch/x86/pci/acpi.c +2 -3
      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;
Quoted 8 lines

drivers/acpi/resource.c EXPORT_SYMBOL_GPL(acpi_dev_get_resources); · line 577

Quoted 8 lines
drivers/acpi/resource.c +0 -0
index 5589a6e2a023..79b6d3b5ffd2 100644
@@ -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.

3) Since you're touching this comment anyway, can you fix the "hepler" typo above, too?

Quoted 21 lines
Quoted 8 lines
drivers/acpi/resource.c +3 -0
+ *
+ * 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)

drivers/acpi/resource.c int acpi_dev_filter_resource_type(struct acpi_resource *ares, · line 599

Quoted 9 lines
drivers/acpi/resource.c +2 -1
@@ -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.

author
#3 lore raw
Quoted 75 lines · Bjorn Helgaas
Quoted 74 lines · Rafael J. Wysocki
Quoted 33 lines · Jiang Liu

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Quoted 6 lines

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>

Bjorn, Ingo, is anyone looking at this? We're still having a regression in this area ...

Quoted 30 lines
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +14 -0
index e4695985f9de..fc2da98985c3 100644
@@ -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
Quoted 8 lines
Quoted 1 line

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 352

Quoted 1 line
arch/x86/pci/acpi.c +1 -0
+      * it's not perfect.

1) I think it's misleading to say "the specs haven't provided a method." As far as I can tell, the Producer/Consumer bit is intended precisely to distinguish resources consumed by a bridge from those forwarded to downstream devices. It would be more accurate to say "the spec defines a bit, but firmware hasn't used that bit consistently, so we can't rely on it."

Hi Bjorn,
Thanks for review, I will refine the words as suggested by you.

Quoted 4 lines

If you want to say "it's not perfect," it would be useful to mention the ways in which it is not perfect. This code is still a candidate for unification with ia64 and arm64, so we should avoid x86-specific things here as much as possible.

Yes, I have another pending patch set to consolidate IA64 and x86 code for ACPI PCI root.

Quoted 7 lines
Quoted 6 lines

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 357

Quoted 6 lines
arch/x86/pci/acpi.c +5 -0
+      *
+      * Another possible solution is to explicitly filter out IO
+      * port[0xCF8-0xCFF].
+      */
+     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
Quoted 5 lines

2) The usage of IORESOURCE_IO_FIXED here seems like a hack. It's not related to other existing use of IORESOURCE_IO_FIXED, and it's not intuitive that supplying IORESOURCE_MEM means "I want all the memory-type resources," but supplying IORESOURCE_IO_FIXED means "I *don't* want the fixed I/O-type resources."

Yes, the IORESOURCE_IO_FIXED is a hack here. Then how about explicitly filtering IOPORT [0xCF8-0xCFF] so we could avoid such a hack.

Quoted 27 lines

A struct resource is not as expressive as a struct acpi_resource. We're going through a lot of contortions to pass nuances of acpi_resource through to code that only knows about struct resource. I'm not sure that's a good long-term strategy, but I don't have a better suggestion.

Quoted 20 lines
Quoted 20 lines
arch/x86/pci/acpi.c +2 -3
      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;
drivers/acpi/resource.c +0 -1
index 5589a6e2a023..79b6d3b5ffd2 100644
--- a/drivers/acpi/resource.c
Quoted 8 lines
Quoted 5 lines

drivers/acpi/resource.c EXPORT_SYMBOL_GPL(acpi_dev_get_resources); · line 577

Quoted 5 lines

+++ b/drivers/acpi/resource.c

drivers/acpi/resource.c +0 -0
@@ -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.

3) Since you're touching this comment anyway, can you fix the "hepler" typo above, too?

Sure.
Thanks!
Gerry

Quoted 22 lines
Quoted 22 lines
Quoted 9 lines
drivers/acpi/resource.c +3 -0
+ *
+ * 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)

drivers/acpi/resource.c int acpi_dev_filter_resource_type(struct acpi_resource *ares, · line 599

Quoted 9 lines
drivers/acpi/resource.c +2 -1
@@ -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.

maintainer
#4 lore raw
Quoted 10 lines · Jiang Liu
Quoted 9 lines · Bjorn Helgaas
Quoted 7 lines · Rafael J. Wysocki
Quoted 5 lines · Jiang Liu
arch/x86/pci/acpi.c +5 -0
+      *
+      * Another possible solution is to explicitly filter out IO
+      * port[0xCF8-0xCFF].
+      */
+     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
Quoted 8 lines
Quoted 5 lines

2) The usage of IORESOURCE_IO_FIXED here seems like a hack. It's not related to other existing use of IORESOURCE_IO_FIXED, and it's not intuitive that supplying IORESOURCE_MEM means "I want all the memory-type resources," but supplying IORESOURCE_IO_FIXED means "I *don't* want the fixed I/O-type resources."

Yes, the IORESOURCE_IO_FIXED is a hack here. Then how about explicitly filtering IOPORT [0xCF8-0xCFF] so we could avoid such a hack.

I'm OK with that, although it should be in an arch-specific hook or something.

author
#5 lore raw

PATCH v6 1/1 x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

An IO port or MMIO resource assigned to a PCI host bridge may be consumed by the host bridge itself or available to its child bus/devices. The ACPI specification defines a bit (Producer/Consumer) to tell whether the resource is consumed by the host bridge itself, but firmware hasn't used that bit consistently, so we can't rely on it.

Before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation") started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

On x86 and IA64 platforms, all IO port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF]. This solution will also ease the way to consolidate ACPI PCI host bridge common code from x86, ia64 and ARM64.

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Related discussions at:
http://patchwork.ozlabs.org/patch/461633/
https://lkml.org/lkml/2015/3/29/304

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc <[email protected]> # 4.0
---
 arch/x86/pci/acpi.c     |   24 ++++++++++++++++++++++--
 drivers/acpi/resource.c |    2 +-
 2 files changed, 23 insertions(+), 3 deletions(-)
arch/x86/pci/acpi.c +22 -2
index e4695985f9de..d93963340c3c 100644
@@ -325,6 +325,26 @@ static void release_pci_root_info(struct pci_host_bridge *bridge)
 	kfree(info);
 }
 
+/*
+ * An IO port or MMIO resource assigned to a PCI host bridge may be
+ * consumed by the host bridge itself or available to its child
+ * bus/devices. The ACPI specification defines a bit (Producer/Consumer)
+ * to tell whether the resource is consumed by the host bridge itself,
+ * but firmware hasn't used that bit consistently, so we can't rely on it.
+ *
+ * On x86 and IA64 platforms, all IO port and MMIO resources are assumed
+ * to be available to child bus/devices except one special case:
+ *     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
+ *     to access PCI configuration space.
+ *
+ * So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF].
+ */
+static bool resource_is_pcicfg_ioport(struct resource *res)
+{
+	return (res->flags & IORESOURCE_IO) &&
+		res->start == 0xCF8 && res->end == 0xCFF;
+}
+
 static void probe_pci_root_info(struct pci_root_info *info,
 				struct acpi_device *device,
 				int busnum, int domain,

@@ -346,8 +366,8 @@ 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_is_pcicfg_ioport(entry->res))
 				resource_list_destroy_entry(entry);
 			else
 				entry->res->name = info->name;
drivers/acpi/resource.c +1 -1
index 5589a6e2a023..8244f013f210 100644
@@ -573,7 +573,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
  * @ares: Input ACPI resource object.
  * @types: Valid resource types of IORESOURCE_XXX
  *
- * This is a hepler function to support acpi_dev_get_resources(), which filters
+ * This is a helper function to support acpi_dev_get_resources(), which filters
  * ACPI resource objects according to resource types.
  */
 int acpi_dev_filter_resource_type(struct acpi_resource *ares,
Signature · 1 line
1.7.10.4
maintainer Reviewed-by on v6 1/1
#6 lore raw
Quoted 80 lines · Jiang Liu

An IO port or MMIO resource assigned to a PCI host bridge may be consumed by the host bridge itself or available to its child bus/devices. The ACPI specification defines a bit (Producer/Consumer) to tell whether the resource is consumed by the host bridge itself, but firmware hasn't used that bit consistently, so we can't rely on it.

Before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation") started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

On x86 and IA64 platforms, all IO port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF]. This solution will also ease the way to consolidate ACPI PCI host bridge common code from x86, ia64 and ARM64.

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Related discussions at:
http://patchwork.ozlabs.org/patch/461633/
https://lkml.org/lkml/2015/3/29/304

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc <[email protected]> # 4.0
---
 arch/x86/pci/acpi.c     |   24 ++++++++++++++++++++++--
 drivers/acpi/resource.c |    2 +-
 2 files changed, 23 insertions(+), 3 deletions(-)
arch/x86/pci/acpi.c +20 -0
index e4695985f9de..d93963340c3c 100644
@@ -325,6 +325,26 @@ static void release_pci_root_info(struct pci_host_bridge *bridge)
        kfree(info);
 }
 
+/*
+ * An IO port or MMIO resource assigned to a PCI host bridge may be
+ * consumed by the host bridge itself or available to its child
+ * bus/devices. The ACPI specification defines a bit (Producer/Consumer)
+ * to tell whether the resource is consumed by the host bridge itself,
+ * but firmware hasn't used that bit consistently, so we can't rely on it.
+ *
+ * On x86 and IA64 platforms, all IO port and MMIO resources are assumed
+ * to be available to child bus/devices except one special case:
+ *     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
+ *     to access PCI configuration space.
+ *
+ * So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF].
+ */
+static bool resource_is_pcicfg_ioport(struct resource *res)
+{
+       return (res->flags & IORESOURCE_IO) &&
+               res->start == 0xCF8 && res->end == 0xCFF;
+}
+
 static void probe_pci_root_info(struct pci_root_info *info,
                                struct acpi_device *device,
                                int busnum, int domain,

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 370

Quoted 7 lines
arch/x86/pci/acpi.c +2 -2
@@ -346,8 +366,8 @@ 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_is_pcicfg_ioport(entry->res))

Hey, I can understand this code! This is *way* better that what you had before. Thanks for persevering.

My only remaining comment is that the subject line ("Fix regression caused by commit ...") doesn't really describe the code. It should say something specific, like maybe "x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff] available on PCI bus".

Reviewed-by Bjorn Helgaas <[email protected]>
Quoted 19 lines
arch/x86/pci/acpi.c +0 -0
                                resource_list_destroy_entry(entry);
                        else
                                entry->res->name = info->name;
drivers/acpi/resource.c +1 -2
index 5589a6e2a023..8244f013f210 100644
@@ -573,7 +573,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
  * @ares: Input ACPI resource object.
  * @types: Valid resource types of IORESOURCE_XXX
  *
- * This is a hepler function to support acpi_dev_get_resources(), which filters
+ * This is a helper function to support acpi_dev_get_resources(), which filters
  * ACPI resource objects according to resource types.
  */
 int acpi_dev_filter_resource_type(struct acpi_resource *ares,
--

1.7.10.4

#7 lore raw
Quoted 92 lines · Bjorn Helgaas
Quoted 80 lines · Jiang Liu

An IO port or MMIO resource assigned to a PCI host bridge may be consumed by the host bridge itself or available to its child bus/devices. The ACPI specification defines a bit (Producer/Consumer) to tell whether the resource is consumed by the host bridge itself, but firmware hasn't used that bit consistently, so we can't rely on it.

Before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation") started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

On x86 and IA64 platforms, all IO port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF]. This solution will also ease the way to consolidate ACPI PCI host bridge common code from x86, ia64 and ARM64.

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Related discussions at:
http://patchwork.ozlabs.org/patch/461633/
https://lkml.org/lkml/2015/3/29/304

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc <[email protected]> # 4.0
---
 arch/x86/pci/acpi.c     |   24 ++++++++++++++++++++++--
 drivers/acpi/resource.c |    2 +-
 2 files changed, 23 insertions(+), 3 deletions(-)
arch/x86/pci/acpi.c +20 -0
index e4695985f9de..d93963340c3c 100644
@@ -325,6 +325,26 @@ static void release_pci_root_info(struct pci_host_bridge *bridge)
        kfree(info);
 }
 
+/*
+ * An IO port or MMIO resource assigned to a PCI host bridge may be
+ * consumed by the host bridge itself or available to its child
+ * bus/devices. The ACPI specification defines a bit (Producer/Consumer)
+ * to tell whether the resource is consumed by the host bridge itself,
+ * but firmware hasn't used that bit consistently, so we can't rely on it.
+ *
+ * On x86 and IA64 platforms, all IO port and MMIO resources are assumed
+ * to be available to child bus/devices except one special case:
+ *     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
+ *     to access PCI configuration space.
+ *
+ * So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF].
+ */
+static bool resource_is_pcicfg_ioport(struct resource *res)
+{
+       return (res->flags & IORESOURCE_IO) &&
+               res->start == 0xCF8 && res->end == 0xCFF;
+}
+
 static void probe_pci_root_info(struct pci_root_info *info,
                                struct acpi_device *device,
                                int busnum, int domain,

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 370

Quoted 7 lines
arch/x86/pci/acpi.c +2 -2
@@ -346,8 +366,8 @@ 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_is_pcicfg_ioport(entry->res))

Hey, I can understand this code! This is *way* better that what you had before. Thanks for persevering.

Quoted 6 lines

My only remaining comment is that the subject line ("Fix regression caused by commit ...") doesn't really describe the code. It should say something specific, like maybe "x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff] available on PCI bus".

Reviewed-by Bjorn Helgaas <[email protected]>

Awesome, thanks!

I'm sending my pull request for -rc2 shortly, so I'll queue this one up for the next week. I'll follow your subject suggestion.

Ingo, please let me know if there's any problem with that.

Rafael

#8 lore raw
In reply to Jiang Liu
Quoted 88 lines · Jiang Liu
Quoted 75 lines · Bjorn Helgaas
Quoted 74 lines · Rafael J. Wysocki
Quoted 33 lines · Jiang Liu

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Quoted 6 lines

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>

Bjorn, Ingo, is anyone looking at this? We're still having a regression in this area ...

Quoted 30 lines
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +14 -0
index e4695985f9de..fc2da98985c3 100644
@@ -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
Quoted 8 lines
Quoted 1 line

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 352

Quoted 1 line
arch/x86/pci/acpi.c +1 -0
+      * it's not perfect.

1) I think it's misleading to say "the specs haven't provided a method." As far as I can tell, the Producer/Consumer bit is intended precisely to distinguish resources consumed by a bridge from those forwarded to downstream devices. It would be more accurate to say "the spec defines a bit, but firmware hasn't used that bit consistently, so we can't rely on it."

Hi Bjorn,
Thanks for review, I will refine the words as suggested by you.

Quoted 7 lines
Quoted 4 lines

If you want to say "it's not perfect," it would be useful to mention the ways in which it is not perfect. This code is still a candidate for unification with ia64 and arm64, so we should avoid x86-specific things here as much as possible.

Yes, I have another pending patch set to consolidate IA64 and x86 code for ACPI PCI root.

That's OK, but can we please fix the regression first before doing that unification? Like to make life easier for the "stable" people and whoever wants to backport the fix?

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#9 lore raw
Quoted 94 lines · Rafael J. Wysocki
Quoted 93 lines · Jiang Liu
Quoted 75 lines · Bjorn Helgaas
Quoted 74 lines · Rafael J. Wysocki
Quoted 33 lines · Jiang Liu

An IO port or MMIO resource assigned to a 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 port and MMIO resources are assumed to be available to child bus/devices except one special case:
IO port [0xCF8-0xCFF] is consumed by the host bridge itself to access PCI configuration space.

But the ACPI and PCI Firmware specifications haven't provided a method to tell whether a resource is consumed by the host bridge itself. So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored all IO port resources defined by acpi_resource_io and acpi_resource_fixed_io to filter out IO ports consumed by the host bridge itself.

Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces to simplify implementation")started accepting all IO port and MMIO resources, which caused a regression that IO port resources consumed by the host bridge itself became available to its child devices.

Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by host bridge itself") ignored resources consumed by the host bridge itself by checking the IORESOURCE_WINDOW flag, which accidently removed MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32 and acpi_resource_fixed_memory32.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Quoted 6 lines

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by Bernhard Thaler <[email protected]>
Signed-off-by Jiang Liu <[email protected]>

Bjorn, Ingo, is anyone looking at this? We're still having a regression in this area ...

Quoted 30 lines
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)
arch/x86/pci/acpi.c +14 -0
index e4695985f9de..fc2da98985c3 100644
@@ -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
Quoted 8 lines
Quoted 1 line

arch/x86/pci/acpi.c static void probe_pci_root_info(struct pci_root_info *info, · line 352

Quoted 1 line
arch/x86/pci/acpi.c +1 -0
+      * it's not perfect.

1) I think it's misleading to say "the specs haven't provided a method." As far as I can tell, the Producer/Consumer bit is intended precisely to distinguish resources consumed by a bridge from those forwarded to downstream devices. It would be more accurate to say "the spec defines a bit, but firmware hasn't used that bit consistently, so we can't rely on it."

Hi Bjorn,
Thanks for review, I will refine the words as suggested by you.

Quoted 4 lines

If you want to say "it's not perfect," it would be useful to mention the ways in which it is not perfect. This code is still a candidate for unification with ia64 and arm64, so we should avoid x86-specific things here as much as possible.

Quoted 6 lines
Quoted 2 lines

Yes, I have another pending patch set to consolidate IA64 and x86 code for ACPI PCI root.

That's OK, but can we please fix the regression first before doing that unification? Like to make life easier for the "stable" people and whoever wants to backport the fix?

Hi Rafael,
Yes, I will fix the regression first before sending out the
pending patch set.
Thanks!
Gerry