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

From: Jiang Liu
Date: Tue Apr 07 2015 - 10:54:01 EST

On 2015/4/7 19:47, Rafael J. Wysocki wrote:
> On Tuesday, April 07, 2015 12:30:18 PM Jiang Liu wrote:
>> Before commit 593669c2ac0f("Use common ACPI resource interfaces to
>> simplify implementation"), arch/x86/pci/acpi.c applies following
>> rules when parsing ACPI resources for PCI host bridge:
>> 1) Ignore IO port resources defined by acpi_resource_io and
>> acpi_resource_fixed_io, which should be used to define resource
>> for PCI device instead of PCI bridge.
>> 2) Accept IOMEM resource defined by acpi_resource_memory24,
>> acpi_resource_memory32 and acpi_resource_fixed_memory32.
>> These IOMEM resources are accepted to workaround some BIOS issue,
>> though they should be ignored. For example, PC Engines APU.1C
>> platform defines PCI host bridge IOMEM resources as:
>> Memory32Fixed (ReadOnly,
>> 0x000A0000, // Address Base
>> 0x00020000, // Address Length
>> )
>> Memory32Fixed (ReadOnly,
>> 0x00000000, // Address Base
>> 0x00000000, // Address Length
>> _Y00)
>> 3) Accept all IO port and IOMEM resources defined by
>> acpi_resource_address{16,32,64,extended64}, no matter it's marked as
>> ACPI_CONSUMER or ACPI_PRODUCER.
>>
>> Commit 593669c2ac0f("Use common ACPI resource interfaces to
>> simplify implementation") accept all IO port and IOMEM resources
>> defined by acpi_resource_io, acpi_resource_fixed_io,
>> acpi_resource_memory24, acpi_resource_memory32,
>> acpi_resource_fixed_memory32 and
>> acpi_resource_address{16,32,64,extended64}, which causes IO port
>> resources consumed by host bridge itself are listed in to host bridge
>> resource list.
>>
>> Then commit 63f1789ec716("Ignore resources consumed by host bridge
>> itself") ignores resources consumed by host bridge itself by checking
>> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
>> above for BIOS bug .
>>
>> It's really costed us much time to figure out this whole picture.
>> So we refine interface acpi_dev_filter_resource_type as below,
>> which should be easier for maintence:
>> 1) Caller specifies IORESOURCE_WINDOW flag to explicitly query resource
>> for bridge(PRODUCER), otherwise it's querying resource for
>> device(CONSUMER).
>> 2) Ignore IO port resources defined by acpi_resource_io and
>> acpi_resource_fixed_io if IORESOURCE_WINDOW is specified.
>> 3) Accpet IOMEM resource defined by acpi_resource_memory24,
>> acpi_resource_memory32 and acpi_resource_fixed_memory32 if both
>> IORESOURCE_WINDOW and IORESOURCE_MEM_8AND16BIT are specified to work
>> around BIOS issues.
>> 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
>>
>> V3->V4:
>> 1) Improve comments
>> 2) Use flag IORESOURCE_MEM_8AND16BIT to work around BIOS issue
>
> OK, so how does that address the comments in the previous thread?
>
> It would *really* help if you responded there instead of starting a new
> thread by sending a new patch version. This makes it really difficult to
> follow the entire discussion, which is part of why we keep forgetting things.
Hi Rafael,
Apologize:) I miss understood previous mail. Please ignore
this version.
Regards!
Gerry

>
>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@xxxxxxxx>
>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>
>> ---
>> arch/x86/pci/acpi.c | 8 +++++---
>> drivers/acpi/resource.c | 52 +++++++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index e4695985f9de..150774be0f3f 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -332,12 +332,15 @@ 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;
>> + res_flags = IORESOURCE_IO | IORESOURCE_MEM |
>> + IORESOURCE_WINDOW | IORESOURCE_MEM_8AND16BIT;
>> 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 +349,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..0187e0e11bb8 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
>> }
>> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>
>> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
>> +{
>> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
>> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
>> +}
>> +
>> /**
>> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
>> * types
>> @@ -574,7 +580,19 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>> * @types: Valid resource types of IORESOURCE_XXX
>> *
>> * This is a hepler function to support acpi_dev_get_resources(), which filters
>> - * ACPI resource objects according to resource types.
>> + * ACPI resource objects according to resource types and flags 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.
>
> OK, so what if the resource is not of an "extended" type, say DWORD address space,
> but has a non-empty Resource Source field pointing to the device itself?
>
> Shouldn't we treat that device as a "producer" too?
>
>> + * 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.
>
> This is a legitimate thing for the BIOS to do if my reading of the spec is
> correct, as the "producer" thing really is supposed to mean "this resource
> comes from the device itself".
>
>> */
>> int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>> unsigned long types)
>> @@ -585,27 +603,49 @@ 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:
>> - type = IORESOURCE_MEM;
>> + /*
>> + * 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, report
>> + * resource provision by Memory32Fixed(). Please refer to:
>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
>> + * So a special rule to work around BIOS issues.
>> + */
>> + if ((types & (IORESOURCE_WINDOW | IORESOURCE_MEM_8AND16BIT)) ==
>> + (IORESOURCE_WINDOW | IORESOURCE_MEM_8AND16BIT) ||
>> + acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>> + 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)
>>
>
--
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/