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: 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. > >> * 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. Thanks! Gerry > >> * 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. >> */ >> >>> >>>>> 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 <bernhard.thaler@xxxxxxxx> >>>>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> >>>>> --- >>>>> arch/x86/pci/acpi.c | 5 ++--- >>>>> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++---- >>>>> 2 files changed, 31 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c >>>>> index e4695985f9de..8c4b1201f340 100644 >>>>> --- a/arch/x86/pci/acpi.c >>>>> +++ b/arch/x86/pci/acpi.c >>>>> @@ -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; >>>>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c >>>>> index 5589a6e2a023..e761a868bdba 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 >>>>> @@ -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). >>>> >>>> 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. >>> >>>>> 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-acpi" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
