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

[Bjorn Helgaas]

On Wed, Apr 29, 2015 at 11:41 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> wrote:
> An IO port or MMIO resource assigned to a PCI host bridge may be
> consumed by the host bridge itself or available to its child
> bus/devices. The ACPI specification defines a bit (Producer/Consumer)
> to tell whether the resource is consumed by the host bridge itself,
> but firmware hasn't used that bit consistently, so we can't rely on it.
>
> Before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource
> interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored
> all IO port resources defined by acpi_resource_io and
> acpi_resource_fixed_io to filter out IO ports consumed by the host
> bridge itself.
>
> Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces
> to simplify implementation") started accepting all IO port and MMIO
> resources, which caused a regression that IO port resources consumed
> by the host bridge itself became available to its child devices.
>
> Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
> host bridge itself") ignored resources consumed by the host bridge
> itself by checking the IORESOURCE_WINDOW flag, which accidently removed
> MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32
> and acpi_resource_fixed_memory32.
>
> On x86 and IA64 platforms, all IO port and MMIO resources are assumed
> to be available to child bus/devices except one special case:
>     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
>     to access PCI configuration space.
>
> So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF]. This solution
> will also ease the way to consolidate ACPI PCI host bridge common code
> from x86, ia64 and ARM64.
>
> Related ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>
> Related discussions at:
> http://patchwork.ozlabs.org/patch/461633/
> https://lkml.org/lkml/2015/3/29/304
>
> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> Reported-by: Bernhard Thaler <bernhard.thaler@xxxxxxxx>
> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.0
> ---
>  arch/x86/pci/acpi.c     |   24 ++++++++++++++++++++++--
>  drivers/acpi/resource.c |    2 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..d93963340c3c 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -325,6 +325,26 @@ static void release_pci_root_info(struct pci_host_bridge *bridge)
>         kfree(info);
>  }
>
> +/*
> + * An IO port or MMIO resource assigned to a PCI host bridge may be
> + * consumed by the host bridge itself or available to its child
> + * bus/devices. The ACPI specification defines a bit (Producer/Consumer)
> + * to tell whether the resource is consumed by the host bridge itself,
> + * but firmware hasn't used that bit consistently, so we can't rely on it.
> + *
> + * On x86 and IA64 platforms, all IO port and MMIO resources are assumed
> + * to be available to child bus/devices except one special case:
> + *     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
> + *     to access PCI configuration space.
> + *
> + * So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF].
> + */
> +static bool resource_is_pcicfg_ioport(struct resource *res)
> +{
> +       return (res->flags & IORESOURCE_IO) &&
> +               res->start == 0xCF8 && res->end == 0xCFF;
> +}
> +
>  static void probe_pci_root_info(struct pci_root_info *info,
>                                 struct acpi_device *device,
>                                 int busnum, int domain,
> @@ -346,8 +366,8 @@ 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_is_pcicfg_ioport(entry->res))

Hey, I can understand this code!  This is *way* better that what you
had before.  Thanks for persevering.

My only remaining comment is that the subject line ("Fix regression
caused by commit ...") doesn't really describe the code.  It should
say something specific, like maybe "x86/PCI/ACPI: Make all resources
except [io 0xcf8-0xcff] available on PCI bus".

Reviewed-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

>                                 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..8244f013f210 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -573,7 +573,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>   * @ares: Input ACPI resource object.
>   * @types: Valid resource types of IORESOURCE_XXX
>   *
> - * This is a hepler function to support acpi_dev_get_resources(), which filters
> + * This is a helper function to support acpi_dev_get_resources(), which filters
>   * ACPI resource objects according to resource types.
>   */
>  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/