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? > * 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. > * 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 > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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
