linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
@ 2015-04-20  3:08 Jiang Liu
  2015-04-29  0:40 ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Jiang Liu @ 2015-04-20  3:08 UTC (permalink / raw)
  To: Rafael J . Wysocki, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, x86, Len Brown
  Cc: Jiang Liu, Lv Zheng, LKML, linux-pci, linux-acpi

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 <bernhard.thaler@wvnet.at>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..fc2da98985c3 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
 {
 	int ret;
 	struct resource_entry *entry, *tmp;
+	unsigned long res_flags;
 
 	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
 	info->bridge = device;
+
+	/*
+	 * An IO or MMIO resource assigned to PCI host bridge may be consumed
+	 * by the host bridge itself or available to its child bus/devices.
+	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
+	 * be available to child bus/devices except one special case:
+	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to
+	 *	access PCI configuration space.
+	 *
+	 * Due to lack of specification to define resources consumed by host
+	 * bridge itself, all IO port resources defined by acpi_resource_io
+	 * and acpi_resource_fixed_io are ignored to filter out IO
+	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
+	 * it's not perfect.
+	 *
+	 * Another possible solution is to explicitly filter out IO
+	 * port[0xCF8-0xCFF].
+	 */
+	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
 	ret = acpi_dev_get_resources(device, list,
 				     acpi_dev_filter_resource_type_cb,
-				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+				     (void *)res_flags);
 	if (ret < 0)
 		dev_warn(&device->dev,
 			 "failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +366,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
 			"no IO and memory resources present in _CRS\n");
 	else
 		resource_list_for_each_entry_safe(entry, tmp, list) {
-			if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
-			    (entry->res->flags & IORESOURCE_DISABLED))
+			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
 			else
 				entry->res->name = info->name;
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 5589a6e2a023..79b6d3b5ffd2 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
  *
  * This is a hepler function to support acpi_dev_get_resources(), which filters
  * ACPI resource objects according to resource types.
+ *
+ * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
+ * descriptors for ACPI host bridges on x86 and IA64 platforms.
  */
 int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 				  unsigned long types)
@@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 		break;
 	case ACPI_RESOURCE_TYPE_IO:
 	case ACPI_RESOURCE_TYPE_FIXED_IO:
-		type = IORESOURCE_IO;
+		if ((types & IORESOURCE_IO_FIXED) == 0)
+			type = IORESOURCE_IO;
 		break;
 	case ACPI_RESOURCE_TYPE_IRQ:
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
@ 2015-04-13  4:31 Jiang Liu
  2015-04-13  6:38 ` [Bugfix v5] " Jiang Liu
  0 siblings, 1 reply; 13+ messages in thread
From: Jiang Liu @ 2015-04-13  4:31 UTC (permalink / raw)
  To: Rafael J. Wysocki, Rafael J. Wysocki
  Cc: Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86@kernel.org, Len Brown, Lv Zheng, LKML,
	linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org

On 2015/4/10 8:28, Rafael J. Wysocki wrote:
> On Fri, Apr 10, 2015 at 12:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, April 09, 2015 05:00:08 PM Bjorn Helgaas wrote:
>>> On Thu, Apr 9, 2015 at 4:37 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Thursday, April 09, 2015 10:50:02 AM Jiang Liu wrote:
>>>>> On 2015/4/9 7:44, Rafael J. Wysocki wrote:
>>>>>> On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>>>>>>> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>>>>>>>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>>>>>>>>> Hi Jiang,
>>>>>>> <snip>
>>>>>>>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>>>>>>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>>>>>>>>
>>>>>>>>> We should assume it will eventually be used for all ACPI devices,
>>>>>>>>> shouldn't we?
>>>>>>>>
>>>>>>>> I'm not sure about that, really.  In fact, I'd restrict its use to devices
>>>>>>>> types that actually can "produce" resources (ie. do not require the resources
>>>>>>>> to be provided by their ancestors or to be available from a global pool).
>>>>>>>>
>>>>>>>> Otherwise we're pretty much guaranteed to get into trouble.
>>>>>>>>
>>>>>>>> And all of the above rules need to be documented in the kernel source tree
>>>>>>>> or people will get confused.
>>>>>>> Hi Rafael,
>>>>>>>     How about following comments for acpi_dev_filter_resource_type()?
>>>>>>> Thanks!
>>>>>>> Gerry
>>>>>>> --------------------------------------------------------------------
>>>>>>> /**
>>>>>>>  * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>>>>>>>  * descriptor means:
>>>>>>>  *      1(CONSUMER): This device consumes this resource
>>>>>>>  *      0(PRODUCER): This device produces and consumes this resource
>>>>>>>  * But for ACPI PCI host bridge, it is interpreted in another way:
>>>>>>
>>>>>> So first of all, this leads to a question: Why is it interpreted for ACPI PCI
>>>>>> host bridges differently?
>>>>>>
>>>>>> Is it something we've figured out from experience, or is there a standard
>>>>>> mandating that?
>>>>> Hi Rafael,
>>>>>       I think we got this knowledge by experiences. PCI FW spec only
>>>>> states _CRS reports resources assigned to the host bridge by firmware.
>>>>> There's no statement about whether the resource is consumed by host
>>>>> bridge itself or provided to it's child bus/devices. On x86/IA64 side,
>>>>> the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
>>>>> but not sure about ARM64 side. So how about:
>>>>
>>>> This:
>>>>
>>>>> PCI Firmware specification states that _CRS reports resources
>>>>> assigned to the host bridge, but there's no way to tell whether
>>>>> the resource is consumed by host bridge itself or provided to
>>>>> its child bus/devices.
>>>>
>>>> looks OK to me, but I'd replace the below with something like:
>>>>
>>>> "However, experience shows, that in the PCI host bridge case firmware writers
>>>>  expect the resource to be provided to devices on the bus (below the bridge) for
>>>>  consumption entirely if its Consumer/Producer flag is clear.  That expectation
>>>>  is reflected by the code in this routine as follows:".
>>>
>>> What a mess.  The spec is regrettably lacking in Consumer/Producer
>>> specifics.  But I don't see what's confusing about the descriptors
>>> that have Consumer/Producer bits.
>>>
>>> QWord, DWord, and Word descriptors don't seem to have a
>>> Consumer/Producer bit, but they do contain _TRA, so they must be
>>> intended for bridge windows.  Can they also be used for device
>>> registers?  I don't know.
>>>
>>> The Extended Address descriptor has a Consumer/Producer bit, and I
>>> would interpret the spec to mean that if the flag is clear (the device
>>> produces and consumes this resource), the resource is available on the
>>> bus below the bridge, i.e., the bridge consumes the resource on its
>>> upstream side and produces it on its downstream side.
>>
>> OK, that makes sense for bridges.

>>> If the bit were
>>> set (the device only consumes the resource), I would expect that to
>>> mean the descriptor is for bridge registers like 0xcf8/0xcfc that
>>> never appear on the downstream side.
>>
>> That part is clear.  What is not clear is whether or not we can *always*
>> expect the resources with Consumer/Producer *clear* to be produced on the
>> downstram side.  That appears to be the case for host bridges if my
>> understanding of things is correct, but is it the case in general?
>>
>>> Maybe I'm reading the spec too naively, but this doesn't seem a matter
>>> of experience.
>>
>> Well, the specification is not clear enough in that respect, at least as
>> far as I can say, or maybe it is, but then does firmware always follow that
>> interpretation?
> 
> So I guess I'd like to propose to go back to the 3.19 behavior for PCI host
> bridges and then to handle the IOAPIC as a separate case.
> 
> We can think about consolidating the two later.
> 
> Any objections to doing that?
Hi Rafael,
	When reverting to the behavior before v3.19, I found a simpler
solution. The logic before v3.19 is:
on x86 and IA64, all IO port and MMIO resources assigned to PCI host
bridge are available to child bus/devices, except one special case to
filter out IO port[0xCF8-0xCFF] for PCI configuration space access.

And with pre-v3.19 kernels, all IO port defined by IO and fixed_IO
resource descriptors are ignored to filter out IO port[0xCF8-0xCFF].

So I plan to make following change to revert to the behavior before
v3.19:
1) make acpi_dev_filter_resource_type() filter descriptors based on
   descriptor type, and don't check consumer_producer flag anymore.
2) use IORESOURCE_IO_FIXED flag to filter out io and fixed_io resource
   descriptors.
3) x86 ACPI pci host bridge driver calls acpi_dev_filter_resource_type()
   with flag IORESOURCE_IO_FIXED,

By this way, we could keep acpi_dev_filter_resource_type()
as a generic interface and could be reused. And the change
is small too. Any comments?

Thanks!
Gerry
> Rafael
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-04-30 19:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-20  3:08 [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716 Jiang Liu
2015-04-29  0:40 ` Rafael J. Wysocki
2015-04-29 13:20   ` Bjorn Helgaas
2015-04-29 13:33     ` Jiang Liu
2015-04-29 13:37       ` Bjorn Helgaas
2015-04-30  4:41         ` [Bugfix v6] " Jiang Liu
2015-04-30 13:28           ` Bjorn Helgaas
2015-04-30 20:16             ` Rafael J. Wysocki
2015-04-29 14:15       ` [Bugfix v5] " Rafael J. Wysocki
2015-04-29 13:53         ` Jiang Liu
  -- strict thread matches above, loose matches on Subject: below --
2015-04-13  4:31 [Bugfix v3] " Jiang Liu
2015-04-13  6:38 ` [Bugfix v5] " Jiang Liu
2015-04-13  6:56   ` Ingo Molnar
2015-04-13 12:05     ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).