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

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +29 -4

index 5589a6e2a023..e761a868bdba 100644
@@ -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)

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +6 -0

index 5589a6e2a023..e761a868bdba 100644
@@ -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:

drivers/acpi/resource.c +15 -4

 		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)

Signature · 1 line

1.7.10.4

Hi Jiang,

Sorry for my delayed response. I've been on vacation for a week and am still trying to catch up.

Quoted 32 lines · Jiang Liu

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

2) Ignore IO port resources defined by acpi_resource_io and acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.

Sounds good to me.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 61 lines

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +6 -0

index 5589a6e2a023..e761a868bdba 100644
@@ -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:

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

Quoted 10 lines

drivers/acpi/resource.c +8 -0

 	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.

Quoted 28 lines

drivers/acpi/resource.c +12 -4

 		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;

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

Quoted 10 lines

drivers/acpi/resource.c +3 -0

 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Bjorn Helgaas

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:

Quoted 12 lines

Quoted 4 lines

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":

Quoted 4 lines

- 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).

Quoted 6 lines

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)

Quoted 3 lines

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.

Quoted 14 lines

Quoted 6 lines

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.

Quoted 2 lines

2) Ignore IO port resources defined by acpi_resource_io and acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.

Sounds good to me.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 11 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Another possible fix is to only ignore IO resource consumed by host

Quoted 19 lines

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

Quoted 13 lines

drivers/acpi/resource.c +8 -0

@@ -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).

Quoted 2 lines

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.

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

Quoted 5 lines

Quoted 2 lines

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.

Quoted 78 lines

Quoted 62 lines

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +6 -0

index 5589a6e2a023..e761a868bdba 100644
@@ -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:

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

Quoted 10 lines

drivers/acpi/resource.c +8 -0

 	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).

Quoted 2 lines

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.

Quoted 42 lines

Quoted 28 lines

drivers/acpi/resource.c +12 -4

 		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;

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

Quoted 10 lines

drivers/acpi/resource.c +3 -0

 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

*/

Quoted 127 lines

Quoted 78 lines

Quoted 62 lines

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +6 -0

index 5589a6e2a023..e761a868bdba 100644
@@ -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:

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

Quoted 10 lines

drivers/acpi/resource.c +8 -0

 	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).

Quoted 2 lines

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.

Quoted 42 lines

Quoted 28 lines

drivers/acpi/resource.c +12 -4

 		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;

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

Quoted 10 lines

drivers/acpi/resource.c +3 -0

 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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?

Quoted 8 lines

* 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.

Quoted 144 lines

* 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.

*/

Quoted 127 lines

Quoted 78 lines

Quoted 62 lines

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 <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

arch/x86/pci/acpi.c +2 -3

index e4695985f9de..8c4b1201f340 100644
@@ -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;

drivers/acpi/resource.c +6 -0

index 5589a6e2a023..e761a868bdba 100644
@@ -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:

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

Quoted 10 lines

drivers/acpi/resource.c +8 -0

 	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).

Quoted 2 lines

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.

Quoted 42 lines

Quoted 28 lines

drivers/acpi/resource.c +12 -4

 		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;

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

Quoted 10 lines

drivers/acpi/resource.c +3 -0

 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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.

Quoted 11 lines

Quoted 8 lines

* 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.

Quoted 37 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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?

Quoted 7 lines

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:

Quoted 4 lines

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:".

Quoted 3 lines

So according to experiences, PCI host bridge
interprets Consumer/Producer flag as below to tell whether the resource is consumed by host bridge itself.

Quoted 53 lines · Rafael J. Wysocki

Quoted 37 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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?

Quoted 7 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 70 lines · Bjorn Helgaas

Quoted 53 lines · Rafael J. Wysocki

Quoted 37 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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?

Quoted 7 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 2 lines

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?

Quoted 89 lines · Rafael J. Wysocki

Quoted 70 lines · Bjorn Helgaas

Quoted 53 lines · Rafael J. Wysocki

Quoted 37 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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?

Quoted 7 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

Quoted 2 lines

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

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 <[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 +0 -0

index e4695985f9de..fc2da98985c3 100644

arch/x86/pci/acpi.c +5 -3

+	 * 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 +0 -0

index 5589a6e2a023..79b6d3b5ffd2 100644

drivers/acpi/resource.c +3 -1

+ * 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

Quoted 2 lines · Jiang Liu

Before commit 593669c2ac0f("Use common ACPI resource interfaces to simplify implementation"), arch/x86/pci/acpi.c applies following

s/applies following
/applied the following

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 9 lines

   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)

Quoted 3 lines

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

Quoted 2 lines

Commit 593669c2ac0f("Use common ACPI resource interfaces to simplify implementation") accept all IO port and IOMEM resources

s/accept
/started accepting

Quoted 6 lines

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.

?

Quoted 2 lines

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

Quoted 2 lines

IORESOURCE_WINDOW flag, which accidently removed the workaround in 2) above for BIOS bug .

s/for BIOS bug .
/for the BIOS bug.

Quoted 18 lines

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 <[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 +0 -0

index e4695985f9de..fc2da98985c3 100644

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

Quoted 10 lines

arch/x86/pci/acpi.c +4 -0

@@ -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

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

Quoted 4 lines

arch/x86/pci/acpi.c +4 -0

+	 * 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

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

Quoted 4 lines

arch/x86/pci/acpi.c +4 -0

+	 *	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

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

Quoted 5 lines

arch/x86/pci/acpi.c +5 -0

+	 * 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

Quoted 25 lines

arch/x86/pci/acpi.c +5 -3

+	 * 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 +0 -0

index 5589a6e2a023..79b6d3b5ffd2 100644

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

Quoted 5 lines

drivers/acpi/resource.c +2 -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.
+ *
+ * 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

Quoted 14 lines

drivers/acpi/resource.c +3 -1

+ * 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:

Quoted 97 lines · Rafael J. Wysocki

Quoted 89 lines · Rafael J. Wysocki

Quoted 70 lines · Bjorn Helgaas

Quoted 53 lines · Rafael J. Wysocki

Quoted 37 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Jiang Liu

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Bjorn Helgaas

Hi Jiang,

<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 8 lines

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.

Quoted 11 lines

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:

Quoted 5 lines

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?

Quoted 7 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

Quoted 2 lines

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?

Quoted 6 lines

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?