Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

[Bjorn Helgaas]

On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Monday, April 20, 2015 11:08:58 AM Jiang Liu wrote:
>> 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@xxxxxxxx>
>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>
>
> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
> this area ...

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

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

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

>> +      *
>> +      * Another possible solution is to explicitly filter out IO
>> +      * port[0xCF8-0xCFF].
>> +      */
>> +     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;

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

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

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

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

>> + *
>> + * 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:
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/