| Message ID | 1427683243-14355-1-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Monday, March 30, 2015 10:40:43 AM 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 for BIOS > bugs, with comment to state it's workaround for BIOS bug. > 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 > > 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@wvnet.at> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> Bjorn, any comments here? We still have this regression in 4.0-rc ... > --- > 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; > 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) >
Hi Jiang, Sorry for my delayed response. I've been on vacation for a week and am still trying to catch up. On Mon, Mar 30, 2015 at 10:40:43AM +0800, 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 . This is probably partly my fault. I think the ACPI spec intention is that every _CRS resource descriptor should be interpreted as "Consumer," i.e., as resources consumed by the device itself, unless it's marked otherwise. Only the following types can be marked as "Producer": - Word/DWord/QWord/Extended address space descriptors, - Extended interrupt descriptors, - GPIO interrupt and I/O connections, - I2C/SPI/UART serial bus resource descriptors With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource types"), I made Linux treat Memory24, Memory32, and Memory32Fixed descriptors in PCI host bridge _CRS as Producers. I did it because Windows apparently does that (there are details in https://bugzilla.kernel.org/show_bug.cgi?id=15817), but I wasn't aware of any machines that required it. That was probably a mistake because it didn't fix anything and it covered up ASL usage errors like what PC Engines did. > 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). Sounds good to me. > 2) Ignore IO port resources defined by acpi_resource_io and > acpi_resource_fixed_io if IORESOURCE_WINDOW is specified. Sounds good to me. > 3) Accpet IOMEM resource defined by acpi_resource_memory24, > acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS > bugs, with comment to state it's workaround for BIOS bug. I don't like the fact that this is the behavior for all ACPI devices. Prior to 593669c2ac0f, we had this behavior for PCI host bridges only. I don't think this is what the spec envisioned, so I don't really like doing it for all devices. > 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 Sounds good to me. > 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? > 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@wvnet.at> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > 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. > 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) > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote: > Hi Jiang, > > Sorry for my delayed response. I've been on vacation for a week and am > still trying to catch up. > > On Mon, Mar 30, 2015 at 10:40:43AM +0800, 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 . > > This is probably partly my fault. > > I think the ACPI spec intention is that every _CRS resource descriptor > should be interpreted as "Consumer," i.e., as resources consumed by the > device itself, unless it's marked otherwise. Only the following types can > be marked as "Producer": > > - Word/DWord/QWord/Extended address space descriptors, > - Extended interrupt descriptors, > - GPIO interrupt and I/O connections, > - I2C/SPI/UART serial bus resource descriptors So we're talking about the consumer/producer flag in those extended resource type descriptors, right? My understanding of that flag is that it doesn't say whether or not the device is a producer of a resource in a general sense. It only says whether the device consumes a resource provided by someone else (1) or it consumes a resources provided by itself (0). > With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource > types"), I made Linux treat Memory24, Memory32, and Memory32Fixed > descriptors in PCI host bridge _CRS as Producers. I did it because Windows > apparently does that (there are details in > https://bugzilla.kernel.org/show_bug.cgi?id=15817), It looks like it does that to indicate that those resources are provided by the host bridge itself (which is consistent with the consumer/producer flag interpretation above) > but I wasn't aware of any machines that required it. That was probably a > mistake because it didn't fix anything and it covered up ASL usage errors > like what PC Engines did. I don't think it is required. It only seems to allow Windows to consolidate the handling of host bridge resources. > > 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). > > Sounds good to me. > > > 2) Ignore IO port resources defined by acpi_resource_io and > > acpi_resource_fixed_io if IORESOURCE_WINDOW is specified. > > Sounds good to me. > > > 3) Accpet IOMEM resource defined by acpi_resource_memory24, > > acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS > > bugs, with comment to state it's workaround for BIOS bug. > > I don't like the fact that this is the behavior for all ACPI devices. > Prior to 593669c2ac0f, we had this behavior for PCI host bridges only. > I don't think this is what the spec envisioned, so I don't really like > doing it for all devices. Agreed. > > 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 > > Sounds good to me. > > > 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. > > 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@wvnet.at> > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > > --- > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/4/7 8:28, Rafael J. Wysocki wrote: > On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote: >> Hi Jiang, >> >> Sorry for my delayed response. I've been on vacation for a week and am >> still trying to catch up. >> >> On Mon, Mar 30, 2015 at 10:40:43AM +0800, Jiang Liu wrote: <snip> >>> 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 . >> >> This is probably partly my fault. >> >> I think the ACPI spec intention is that every _CRS resource descriptor >> should be interpreted as "Consumer," i.e., as resources consumed by the >> device itself, unless it's marked otherwise. Only the following types can >> be marked as "Producer": >> >> - Word/DWord/QWord/Extended address space descriptors, >> - Extended interrupt descriptors, >> - GPIO interrupt and I/O connections, >> - I2C/SPI/UART serial bus resource descriptors > > So we're talking about the consumer/producer flag in those extended resource > type descriptors, right? > > My understanding of that flag is that it doesn't say whether or not the device > is a producer of a resource in a general sense. It only says whether the device > consumes a resource provided by someone else (1) or it consumes a resources > provided by itself (0). Hi Rafael, I have read the ACPI spec again. You are right, the spec states that: Consumer/Producer: 1–This device consumes this resource 0–This device produces and consumes this resource This is different from my previous understanding. Previously I thought "CONSUMER" means the device consumes the resource by itself and "PRODUCER" means the device provides resource to other devices. So seems PCI host bridge has different interpretation of CONSUMER/PRODUCER flag from ACPI spec. > >> With 66528fdd45b0 ("x86/PCI: parse additional host bridge window resource >> types"), I made Linux treat Memory24, Memory32, and Memory32Fixed >> descriptors in PCI host bridge _CRS as Producers. I did it because Windows >> apparently does that (there are details in >> https://bugzilla.kernel.org/show_bug.cgi?id=15817), > > It looks like it does that to indicate that those resources are provided > by the host bridge itself (which is consistent with the consumer/producer > flag interpretation above) > >> but I wasn't aware of any machines that required it. That was probably a >> mistake because it didn't fix anything and it covered up ASL usage errors >> like what PC Engines did. > > I don't think it is required. It only seems to allow Windows to consolidate > the handling of host bridge resources. > >>> 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). >> >> Sounds good to me. >> >>> 2) Ignore IO port resources defined by acpi_resource_io and >>> acpi_resource_fixed_io if IORESOURCE_WINDOW is specified. >> >> Sounds good to me. >> >>> 3) Accpet IOMEM resource defined by acpi_resource_memory24, >>> acpi_resource_memory32 and acpi_resource_fixed_memory32 for BIOS >>> bugs, with comment to state it's workaround for BIOS bug. >> >> I don't like the fact that this is the behavior for all ACPI devices. >> Prior to 593669c2ac0f, we had this behavior for PCI host bridges only. >> I don't think this is what the spec envisioned, so I don't really like >> doing it for all devices. > > Agreed. > >>> 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 >> >> Sounds good to me. >> >>> 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. You are right, we should limit acpi_dev_filter_resource_type() usages to PCI host bridges and IOAPIC only. > >>> Another possible fix is to only ignore IO resource consumed by host <snip> >>> @@ -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. Got it, will update comments. Thanks! Gerry -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: * 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 * 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@wvnet.at> >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >>> --- >>> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@wvnet.at> > >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > >>> --- > >>> 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@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
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@wvnet.at> >>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >>>>> --- >>>>> 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@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:". > 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. Your responses would be easier to follow if they were clearly separate from the original message text (ie. add an empty line between the paragraph you're responding to and the response). > 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. OK, so I'd like to see how the code would look like if those two cases had their own "filter" routines, if that's not a problem. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. Maybe I'm reading the spec too naively, but this doesn't seem a matter of experience. >> So according to experiences, PCI host bridge >> interprets Consumer/Producer flag as below to tell whether the resource >> is consumed by host bridge itself. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, April 13, 2015 12:31:20 PM Jiang Liu wrote: > 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? Sounds good to me.
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; 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)
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 for BIOS bugs, with comment to state it's workaround for BIOS bug. 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 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@wvnet.at> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- arch/x86/pci/acpi.c | 5 ++--- drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-)