From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753341AbbDMGga (ORCPT <rfc822;w@1wt.eu>);
	Mon, 13 Apr 2015 02:36:30 -0400
Received: from mga11.intel.com ([192.55.52.93]:49997 "EHLO mga11.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751498AbbDMGg1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 13 Apr 2015 02:36:27 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.11,568,1422950400"; 
   d="scan'208";a="694323781"
From: Jiang Liu <jiang.liu@linux.intel.com>
To: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Len Brown <lenb@kernel.org>
Cc: Jiang Liu <jiang.liu@linux.intel.com>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Mon, 13 Apr 2015 14:38:38 +0800
Message-Id: <1428907118-27099-1-git-send-email-jiang.liu@linux.intel.com>
X-Mailer: git-send-email 1.7.10.4
In-Reply-To: <552B4698.4020703@linux.intel.com>
References: <552B4698.4020703@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 .

So revert to the behavior before v3.19 to fix the regression.

Sample ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

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     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..fc2da98985c3 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
 {
 	int ret;
 	struct resource_entry *entry, *tmp;
+	unsigned long res_flags;
 
 	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
 	info->bridge = device;
+
+	/*
+	 * An IO or MMIO resource assigned to PCI host bridge may be consumed
+	 * by the host bridge itself or available to its child bus/devices.
+	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
+	 * be available to child bus/devices except one special case:
+	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to
+	 *	access PCI configuration space.
+	 *
+	 * Due to lack of specification to define resources consumed by host
+	 * bridge itself, all IO port resources defined by acpi_resource_io
+	 * and acpi_resource_fixed_io are ignored to filter out IO
+	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
+	 * it's not perfect.
+	 *
+	 * Another possible solution is to explicitly filter out IO
+	 * port[0xCF8-0xCFF].
+	 */
+	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
 	ret = acpi_dev_get_resources(device, list,
 				     acpi_dev_filter_resource_type_cb,
-				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+				     (void *)res_flags);
 	if (ret < 0)
 		dev_warn(&device->dev,
 			 "failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +366,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..79b6d3b5ffd2 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
  *
  * This is a hepler function to support acpi_dev_get_resources(), which filters
  * ACPI resource objects according to resource types.
+ *
+ * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
+ * descriptors for ACPI host bridges on x86 and IA64 platforms.
  */
 int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 				  unsigned long types)
@@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 		break;
 	case ACPI_RESOURCE_TYPE_IO:
 	case ACPI_RESOURCE_TYPE_FIXED_IO:
-		type = IORESOURCE_IO;
+		if ((types & IORESOURCE_IO_FIXED) == 0)
+			type = IORESOURCE_IO;
 		break;
 	case ACPI_RESOURCE_TYPE_IRQ:
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
-- 
1.7.10.4


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752870AbbDMG41 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 13 Apr 2015 02:56:27 -0400
Received: from mail-wg0-f51.google.com ([74.125.82.51]:35559 "EHLO
	mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751455AbbDMG4X (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 13 Apr 2015 02:56:23 -0400
Date: Mon, 13 Apr 2015 08:56:17 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Jiang Liu <jiang.liu@linux.intel.com>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit
 63f1789ec716
Message-ID: <20150413065617.GA22634@gmail.com>
References: <552B4698.4020703@linux.intel.com>
 <1428907118-27099-1-git-send-email-jiang.liu@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1428907118-27099-1-git-send-email-jiang.liu@linux.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


* Jiang Liu <jiang.liu@linux.intel.com> wrote:

> Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation"), arch/x86/pci/acpi.c applies following

s/applies following
 /applied the 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,

s/workaround
 /work around

It's a verb, not a noun.

>    though they should be ignored. For example, PC Engines APU.1C

Please put the platform name into quotes to make it more clearly stand 
apart from the rest of the text.

>    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.

s/no matter it's marked
 /regardless of whether it's marked

> Commit 593669c2ac0f("Use common ACPI resource interfaces to
> simplify implementation") accept all IO port and IOMEM resources

s/accept
 /started accepting

> 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.

this latter part of the sentence does not parse. Did you want to 
write:

  acpi_resource_address{16,32,64,extended64}, which causes IO port
  resources consumed by the host bridge itself to be listed in the
  host bridge resource list.

?

> Then commit 63f1789ec716("Ignore resources consumed by host bridge
> itself") ignores resources consumed by host bridge itself by checking

s/ignores
 /started ignoring

s/by host bridge
 /by the host bridge

> IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> above for BIOS bug .

s/for BIOS bug .
 /for the BIOS bug.

> So revert to the behavior before v3.19 to fix the regression.
> 
> Sample ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
> 
> 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     |   25 ++++++++++++++++++++++---
>  drivers/acpi/resource.c |    6 +++++-
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..fc2da98985c3 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>  {
>  	int ret;
>  	struct resource_entry *entry, *tmp;
> +	unsigned long res_flags;
>  
>  	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>  	info->bridge = device;
> +
> +	/*
> +	 * An IO or MMIO resource assigned to PCI host bridge may be consumed

s/to PCI host bridge
 /to a PCI host bridge

> +	 * by the host bridge itself or available to its child bus/devices.
> +	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> +	 * be available to child bus/devices except one special case:
> +	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to

s/by host bridge
 /by the host bridge

> +	 *	access PCI configuration space.
> +	 *
> +	 * Due to lack of specification to define resources consumed by host
> +	 * bridge itself, all IO port resources defined by acpi_resource_io

s/by host bridge
 /by the host bridge

> +	 * and acpi_resource_fixed_io are ignored to filter out IO
> +	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> +	 * it's not perfect.
> +	 *
> +	 * Another possible solution is to explicitly filter out IO

s/is to
 /would be to

> +	 * port[0xCF8-0xCFF].
> +	 */
> +	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
>  	ret = acpi_dev_get_resources(device, list,
>  				     acpi_dev_filter_resource_type_cb,
> -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> +				     (void *)res_flags);
>  	if (ret < 0)
>  		dev_warn(&device->dev,
>  			 "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +366,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..79b6d3b5ffd2 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>   *
>   * This is a hepler function to support acpi_dev_get_resources(), which filters
>   * ACPI resource objects according to resource types.
> + *
> + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource

s/Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
  The IORESOURCE_IO_FIXED flag is used to opt out IO and FIXED_IO resource

> + * descriptors for ACPI host bridges on x86 and IA64 platforms.
>   */
>  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>  				  unsigned long types)
> @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>  		break;
>  	case ACPI_RESOURCE_TYPE_IO:
>  	case ACPI_RESOURCE_TYPE_FIXED_IO:
> -		type = IORESOURCE_IO;
> +		if ((types & IORESOURCE_IO_FIXED) == 0)
> +			type = IORESOURCE_IO;
>  		break;
>  	case ACPI_RESOURCE_TYPE_IRQ:
>  	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> -- 
> 1.7.10.4
> 

Thanks,

	Ingo

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753960AbbDMLla (ORCPT <rfc822;w@1wt.eu>);
	Mon, 13 Apr 2015 07:41:30 -0400
Received: from v094114.home.net.pl ([79.96.170.134]:56091 "HELO
	v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with SMTP id S1753642AbbDMLl2 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 13 Apr 2015 07:41:28 -0400
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Ingo Molnar <mingo@kernel.org>
Cc: Jiang Liu <jiang.liu@linux.intel.com>, Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Mon, 13 Apr 2015 14:05:58 +0200
Message-ID: <3503474.rt5sqmZxis@vostro.rjw.lan>
User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; )
In-Reply-To: <20150413065617.GA22634@gmail.com>
References: <552B4698.4020703@linux.intel.com> <1428907118-27099-1-git-send-email-jiang.liu@linux.intel.com> <20150413065617.GA22634@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Monday, April 13, 2015 08:56:17 AM Ingo Molnar wrote:
> 
> * Jiang Liu <jiang.liu@linux.intel.com> wrote:
> 
> > Before commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation"), arch/x86/pci/acpi.c applies following
> 
> s/applies following
>  /applied the 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,
> 
> s/workaround
>  /work around
> 
> It's a verb, not a noun.

Agreed on this one an all of the points below.

> >    though they should be ignored. For example, PC Engines APU.1C
> 
> Please put the platform name into quotes to make it more clearly stand 
> apart from the rest of the text.
> 
> >    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.
> 
> s/no matter it's marked
>  /regardless of whether it's marked
> 
> > Commit 593669c2ac0f("Use common ACPI resource interfaces to
> > simplify implementation") accept all IO port and IOMEM resources
> 
> s/accept
>  /started accepting
> 
> > 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.
> 
> this latter part of the sentence does not parse. Did you want to 
> write:
> 
>   acpi_resource_address{16,32,64,extended64}, which causes IO port
>   resources consumed by the host bridge itself to be listed in the
>   host bridge resource list.
> 
> ?
> 
> > Then commit 63f1789ec716("Ignore resources consumed by host bridge
> > itself") ignores resources consumed by host bridge itself by checking
> 
> s/ignores
>  /started ignoring
> 
> s/by host bridge
>  /by the host bridge
> 
> > IORESOURCE_WINDOW flag, which accidently removed the workaround in 2)
> > above for BIOS bug .
> 
> s/for BIOS bug .
>  /for the BIOS bug.
> 
> > So revert to the behavior before v3.19 to fix the regression.
> > 
> > Sample ACPI table are archived at:
> > https://bugzilla.kernel.org/show_bug.cgi?id=94221
> > 
> > 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     |   25 ++++++++++++++++++++++---
> >  drivers/acpi/resource.c |    6 +++++-
> >  2 files changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index e4695985f9de..fc2da98985c3 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
> >  {
> >  	int ret;
> >  	struct resource_entry *entry, *tmp;
> > +	unsigned long res_flags;
> >  
> >  	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
> >  	info->bridge = device;
> > +
> > +	/*
> > +	 * An IO or MMIO resource assigned to PCI host bridge may be consumed
> 
> s/to PCI host bridge
>  /to a PCI host bridge
> 
> > +	 * by the host bridge itself or available to its child bus/devices.
> > +	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> > +	 * be available to child bus/devices except one special case:
> > +	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to
> 
> s/by host bridge
>  /by the host bridge
> 
> > +	 *	access PCI configuration space.
> > +	 *
> > +	 * Due to lack of specification to define resources consumed by host
> > +	 * bridge itself, all IO port resources defined by acpi_resource_io
> 
> s/by host bridge
>  /by the host bridge
> 
> > +	 * and acpi_resource_fixed_io are ignored to filter out IO
> > +	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> > +	 * it's not perfect.
> > +	 *
> > +	 * Another possible solution is to explicitly filter out IO
> 
> s/is to
>  /would be to
> 
> > +	 * port[0xCF8-0xCFF].
> > +	 */
> > +	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
> >  	ret = acpi_dev_get_resources(device, list,
> >  				     acpi_dev_filter_resource_type_cb,
> > -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> > +				     (void *)res_flags);
> >  	if (ret < 0)
> >  		dev_warn(&device->dev,
> >  			 "failed to parse _CRS method, error code %d\n", ret);
> > @@ -346,8 +366,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..79b6d3b5ffd2 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
> >   *
> >   * This is a hepler function to support acpi_dev_get_resources(), which filters
> >   * ACPI resource objects according to resource types.
> > + *
> > + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
> 
> s/Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
>   The IORESOURCE_IO_FIXED flag is used to opt out IO and FIXED_IO resource
> 
> > + * descriptors for ACPI host bridges on x86 and IA64 platforms.
> >   */
> >  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> >  				  unsigned long types)
> > @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> >  		break;
> >  	case ACPI_RESOURCE_TYPE_IO:
> >  	case ACPI_RESOURCE_TYPE_FIXED_IO:
> > -		type = IORESOURCE_IO;
> > +		if ((types & IORESOURCE_IO_FIXED) == 0)
> > +			type = IORESOURCE_IO;
> >  		break;
> >  	case ACPI_RESOURCE_TYPE_IRQ:
> >  	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752573AbbDTDGu (ORCPT <rfc822;w@1wt.eu>);
	Sun, 19 Apr 2015 23:06:50 -0400
Received: from mga11.intel.com ([192.55.52.93]:62389 "EHLO mga11.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751386AbbDTDGr (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Sun, 19 Apr 2015 23:06:47 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.11,607,1422950400"; 
   d="scan'208";a="716083538"
From: Jiang Liu <jiang.liu@linux.intel.com>
To: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Len Brown <lenb@kernel.org>
Cc: Jiang Liu <jiang.liu@linux.intel.com>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Mon, 20 Apr 2015 11:08:58 +0800
Message-Id: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com>
X-Mailer: git-send-email 1.7.10.4
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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. 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.

But the ACPI and PCI Firmware specifications haven't provided a method
to tell whether a resource is consumed by the host bridge itself.
So 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.

So revert to the behavior before v3.19 to fix the regression.

There is also a discussion about ignoring the Producer/Consumer flag on
IA64 platforms at:
http://patchwork.ozlabs.org/patch/461633/

Related ACPI table are archived at:
https://bugzilla.kernel.org/show_bug.cgi?id=94221

Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
 drivers/acpi/resource.c |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..fc2da98985c3 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
 {
 	int ret;
 	struct resource_entry *entry, *tmp;
+	unsigned long res_flags;
 
 	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
 	info->bridge = device;
+
+	/*
+	 * An IO or MMIO resource assigned to PCI host bridge may be consumed
+	 * by the host bridge itself or available to its child bus/devices.
+	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
+	 * be available to child bus/devices except one special case:
+	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to
+	 *	access PCI configuration space.
+	 *
+	 * Due to lack of specification to define resources consumed by host
+	 * bridge itself, all IO port resources defined by acpi_resource_io
+	 * and acpi_resource_fixed_io are ignored to filter out IO
+	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
+	 * it's not perfect.
+	 *
+	 * Another possible solution is to explicitly filter out IO
+	 * port[0xCF8-0xCFF].
+	 */
+	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
 	ret = acpi_dev_get_resources(device, list,
 				     acpi_dev_filter_resource_type_cb,
-				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+				     (void *)res_flags);
 	if (ret < 0)
 		dev_warn(&device->dev,
 			 "failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +366,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..79b6d3b5ffd2 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
  *
  * This is a hepler function to support acpi_dev_get_resources(), which filters
  * ACPI resource objects according to resource types.
+ *
+ * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
+ * descriptors for ACPI host bridges on x86 and IA64 platforms.
  */
 int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 				  unsigned long types)
@@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 		break;
 	case ACPI_RESOURCE_TYPE_IO:
 	case ACPI_RESOURCE_TYPE_FIXED_IO:
-		type = IORESOURCE_IO;
+		if ((types & IORESOURCE_IO_FIXED) == 0)
+			type = IORESOURCE_IO;
 		break;
 	case ACPI_RESOURCE_TYPE_IRQ:
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
-- 
1.7.10.4


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1031313AbbD2APS (ORCPT <rfc822;w@1wt.eu>);
	Tue, 28 Apr 2015 20:15:18 -0400
Received: from v094114.home.net.pl ([79.96.170.134]:59565 "HELO
	v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with SMTP id S1031108AbbD2APP (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 28 Apr 2015 20:15:15 -0400
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Jiang Liu <jiang.liu@linux.intel.com>, Bjorn Helgaas <bhelgaas@google.com>,
        Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>,
        x86@kernel.org, Len Brown <lenb@kernel.org>,
        Lv Zheng <lv.zheng@intel.com>, LKML <linux-kernel@vger.kernel.org>,
        linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Wed, 29 Apr 2015 02:40:07 +0200
Message-ID: <3492366.RzMG5dcPfN@vostro.rjw.lan>
User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; )
In-Reply-To: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com>
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Monday, April 20, 2015 11:08:58 AM Jiang Liu 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. 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.
> 
> But the ACPI and PCI Firmware specifications haven't provided a method
> to tell whether a resource is consumed by the host bridge itself.
> So 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.
> 
> So revert to the behavior before v3.19 to fix the regression.
> 
> There is also a discussion about ignoring the Producer/Consumer flag on
> IA64 platforms at:
> http://patchwork.ozlabs.org/patch/461633/
> 
> Related ACPI table are archived at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221
> 
> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>

Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
this area ...

> ---
>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
>  drivers/acpi/resource.c |    6 +++++-
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..fc2da98985c3 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>  {
>  	int ret;
>  	struct resource_entry *entry, *tmp;
> +	unsigned long res_flags;
>  
>  	sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>  	info->bridge = device;
> +
> +	/*
> +	 * An IO or MMIO resource assigned to PCI host bridge may be consumed
> +	 * by the host bridge itself or available to its child bus/devices.
> +	 * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> +	 * be available to child bus/devices except one special case:
> +	 *	IO port [0xCF8-0xCFF] is consumed by host bridge itself to
> +	 *	access PCI configuration space.
> +	 *
> +	 * Due to lack of specification to define resources consumed by host
> +	 * bridge itself, all IO port resources defined by acpi_resource_io
> +	 * and acpi_resource_fixed_io are ignored to filter out IO
> +	 * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> +	 * it's not perfect.
> +	 *
> +	 * Another possible solution is to explicitly filter out IO
> +	 * port[0xCF8-0xCFF].
> +	 */
> +	res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
>  	ret = acpi_dev_get_resources(device, list,
>  				     acpi_dev_filter_resource_type_cb,
> -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> +				     (void *)res_flags);
>  	if (ret < 0)
>  		dev_warn(&device->dev,
>  			 "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +366,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..79b6d3b5ffd2 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>   *
>   * This is a hepler function to support acpi_dev_get_resources(), which filters
>   * ACPI resource objects according to resource types.
> + *
> + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
> + * descriptors for ACPI host bridges on x86 and IA64 platforms.
>   */
>  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>  				  unsigned long types)
> @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>  		break;
>  	case ACPI_RESOURCE_TYPE_IO:
>  	case ACPI_RESOURCE_TYPE_FIXED_IO:
> -		type = IORESOURCE_IO;
> +		if ((types & IORESOURCE_IO_FIXED) == 0)
> +			type = IORESOURCE_IO;
>  		break;
>  	case ACPI_RESOURCE_TYPE_IRQ:
>  	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1423021AbbD2NVJ (ORCPT <rfc822;w@1wt.eu>);
	Wed, 29 Apr 2015 09:21:09 -0400
Received: from mail-qc0-f169.google.com ([209.85.216.169]:35408 "EHLO
	mail-qc0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422821AbbD2NVF (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 29 Apr 2015 09:21:05 -0400
MIME-Version: 1.0
In-Reply-To: <3492366.RzMG5dcPfN@vostro.rjw.lan>
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com> <3492366.RzMG5dcPfN@vostro.rjw.lan>
From: Bjorn Helgaas <bhelgaas@google.com>
Date: Wed, 29 Apr 2015 08:20:43 -0500
Message-ID: <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com>
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Jiang Liu <jiang.liu@linux.intel.com>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>,
        "x86@kernel.org" <x86@kernel.org>, Len Brown <lenb@kernel.org>,
        Lv Zheng <lv.zheng@intel.com>, LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, April 20, 2015 11:08:58 AM Jiang Liu 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. 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.
>>
>> But the ACPI and PCI Firmware specifications haven't provided a method
>> to tell whether a resource is consumed by the host bridge itself.
>> So 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.
>>
>> So revert to the behavior before v3.19 to fix the regression.
>>
>> There is also a discussion about ignoring the Producer/Consumer flag on
>> IA64 platforms at:
>> http://patchwork.ozlabs.org/patch/461633/
>>
>> Related ACPI table are archived at:
>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>
>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>
> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
> this area ...

>> ---
>>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
>>  drivers/acpi/resource.c |    6 +++++-
>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index e4695985f9de..fc2da98985c3 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>  {
>>       int ret;
>>       struct resource_entry *entry, *tmp;
>> +     unsigned long res_flags;
>>
>>       sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>>       info->bridge = device;
>> +
>> +     /*
>> +      * An IO or MMIO resource assigned to PCI host bridge may be consumed
>> +      * by the host bridge itself or available to its child bus/devices.
>> +      * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
>> +      * be available to child bus/devices except one special case:
>> +      *      IO port [0xCF8-0xCFF] is consumed by host bridge itself to
>> +      *      access PCI configuration space.
>> +      *
>> +      * Due to lack of specification to define resources consumed by host
>> +      * bridge itself, all IO port resources defined by acpi_resource_io
>> +      * and acpi_resource_fixed_io are ignored to filter out IO
>> +      * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
>> +      * it's not perfect.

1) I think it's misleading to say "the specs haven't provided a
method."  As far as I can tell, the Producer/Consumer bit is intended
precisely to distinguish resources consumed by a bridge from those
forwarded to downstream devices.  It would be more accurate to say
"the spec defines a bit, but firmware hasn't used that bit
consistently, so we can't rely on it."

If you want to say "it's not perfect," it would be useful to mention
the ways in which it is not perfect.  This code is still a candidate
for unification with ia64 and arm64, so we should avoid x86-specific
things here as much as possible.

>> +      *
>> +      * Another possible solution is to explicitly filter out IO
>> +      * port[0xCF8-0xCFF].
>> +      */
>> +     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;

2) The usage of IORESOURCE_IO_FIXED here seems like a hack.  It's not
related to other existing use of IORESOURCE_IO_FIXED, and it's not
intuitive that supplying IORESOURCE_MEM means "I want all the
memory-type resources," but supplying IORESOURCE_IO_FIXED means "I
*don't* want the fixed I/O-type resources."

A struct resource is not as expressive as a struct acpi_resource.
We're going through a lot of contortions to pass nuances of
acpi_resource through to code that only knows about struct resource.
I'm not sure that's a good long-term strategy, but I don't have a
better suggestion.

>>       ret = acpi_dev_get_resources(device, list,
>>                                    acpi_dev_filter_resource_type_cb,
>> -                                  (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>> +                                  (void *)res_flags);
>>       if (ret < 0)
>>               dev_warn(&device->dev,
>>                        "failed to parse _CRS method, error code %d\n", ret);
>> @@ -346,8 +366,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..79b6d3b5ffd2 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>   *
>>   * This is a hepler function to support acpi_dev_get_resources(), which filters
>>   * ACPI resource objects according to resource types.

3)  Since you're touching this comment anyway, can you fix the
"hepler" typo above, too?

>> + *
>> + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
>> + * descriptors for ACPI host bridges on x86 and IA64 platforms.
>>   */
>>  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>                                 unsigned long types)
>> @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>               break;
>>       case ACPI_RESOURCE_TYPE_IO:
>>       case ACPI_RESOURCE_TYPE_FIXED_IO:
>> -             type = IORESOURCE_IO;
>> +             if ((types & IORESOURCE_IO_FIXED) == 0)
>> +                     type = IORESOURCE_IO;
>>               break;
>>       case ACPI_RESOURCE_TYPE_IRQ:
>>       case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1423044AbbD2NdY (ORCPT <rfc822;w@1wt.eu>);
	Wed, 29 Apr 2015 09:33:24 -0400
Received: from mga11.intel.com ([192.55.52.93]:37055 "EHLO mga11.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422870AbbD2NdV (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 29 Apr 2015 09:33:21 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.11,671,1422950400"; 
   d="scan'208";a="563662812"
Message-ID: <5540DD9C.9030707@linux.intel.com>
Date: Wed, 29 Apr 2015 21:33:16 +0800
From: Jiang Liu <jiang.liu@linux.intel.com>
Organization: Intel
User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: Bjorn Helgaas <bhelgaas@google.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Ingo Molnar <mingo@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com> <3492366.RzMG5dcPfN@vostro.rjw.lan> <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com>
In-Reply-To: <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2015/4/29 21:20, Bjorn Helgaas wrote:
> On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, April 20, 2015 11:08:58 AM Jiang Liu 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. 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.
>>>
>>> But the ACPI and PCI Firmware specifications haven't provided a method
>>> to tell whether a resource is consumed by the host bridge itself.
>>> So 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.
>>>
>>> So revert to the behavior before v3.19 to fix the regression.
>>>
>>> There is also a discussion about ignoring the Producer/Consumer flag on
>>> IA64 platforms at:
>>> http://patchwork.ozlabs.org/patch/461633/
>>>
>>> Related ACPI table are archived at:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>
>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>
>> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
>> this area ...
> 
>>> ---
>>>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
>>>  drivers/acpi/resource.c |    6 +++++-
>>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>> index e4695985f9de..fc2da98985c3 100644
>>> --- a/arch/x86/pci/acpi.c
>>> +++ b/arch/x86/pci/acpi.c
>>> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>  {
>>>       int ret;
>>>       struct resource_entry *entry, *tmp;
>>> +     unsigned long res_flags;
>>>
>>>       sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>>>       info->bridge = device;
>>> +
>>> +     /*
>>> +      * An IO or MMIO resource assigned to PCI host bridge may be consumed
>>> +      * by the host bridge itself or available to its child bus/devices.
>>> +      * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
>>> +      * be available to child bus/devices except one special case:
>>> +      *      IO port [0xCF8-0xCFF] is consumed by host bridge itself to
>>> +      *      access PCI configuration space.
>>> +      *
>>> +      * Due to lack of specification to define resources consumed by host
>>> +      * bridge itself, all IO port resources defined by acpi_resource_io
>>> +      * and acpi_resource_fixed_io are ignored to filter out IO
>>> +      * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
>>> +      * it's not perfect.
> 
> 1) I think it's misleading to say "the specs haven't provided a
> method."  As far as I can tell, the Producer/Consumer bit is intended
> precisely to distinguish resources consumed by a bridge from those
> forwarded to downstream devices.  It would be more accurate to say
> "the spec defines a bit, but firmware hasn't used that bit
> consistently, so we can't rely on it."

Hi Bjorn,
	Thanks for review, I will refine the words as suggested by you.

> If you want to say "it's not perfect," it would be useful to mention
> the ways in which it is not perfect.  This code is still a candidate
> for unification with ia64 and arm64, so we should avoid x86-specific
> things here as much as possible.

Yes, I have another pending patch set to consolidate IA64 and x86 code
for ACPI PCI root.

> 
>>> +      *
>>> +      * Another possible solution is to explicitly filter out IO
>>> +      * port[0xCF8-0xCFF].
>>> +      */
>>> +     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
> 
> 2) The usage of IORESOURCE_IO_FIXED here seems like a hack.  It's not
> related to other existing use of IORESOURCE_IO_FIXED, and it's not
> intuitive that supplying IORESOURCE_MEM means "I want all the
> memory-type resources," but supplying IORESOURCE_IO_FIXED means "I
> *don't* want the fixed I/O-type resources."

Yes, the IORESOURCE_IO_FIXED is a hack here. Then how about explicitly
filtering IOPORT [0xCF8-0xCFF] so we could avoid such a hack.

> 
> A struct resource is not as expressive as a struct acpi_resource.
> We're going through a lot of contortions to pass nuances of
> acpi_resource through to code that only knows about struct resource.
> I'm not sure that's a good long-term strategy, but I don't have a
> better suggestion.
> 
>>>       ret = acpi_dev_get_resources(device, list,
>>>                                    acpi_dev_filter_resource_type_cb,
>>> -                                  (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>> +                                  (void *)res_flags);
>>>       if (ret < 0)
>>>               dev_warn(&device->dev,
>>>                        "failed to parse _CRS method, error code %d\n", ret);
>>> @@ -346,8 +366,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..79b6d3b5ffd2 100644
>>> --- a/drivers/acpi/resource.c
>>> +++ b/drivers/acpi/resource.c
>>> @@ -575,6 +575,9 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>>   *
>>>   * This is a hepler function to support acpi_dev_get_resources(), which filters
>>>   * ACPI resource objects according to resource types.
> 
> 3)  Since you're touching this comment anyway, can you fix the
> "hepler" typo above, too?

Sure.
Thanks!
Gerry

> 
>>> + *
>>> + * Flag IORESOURCE_IO_FIXED is used to opt out io and fixed_io resource
>>> + * descriptors for ACPI host bridges on x86 and IA64 platforms.
>>>   */
>>>  int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>>                                 unsigned long types)
>>> @@ -589,7 +592,8 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>>               break;
>>>       case ACPI_RESOURCE_TYPE_IO:
>>>       case ACPI_RESOURCE_TYPE_FIXED_IO:
>>> -             type = IORESOURCE_IO;
>>> +             if ((types & IORESOURCE_IO_FIXED) == 0)
>>> +                     type = IORESOURCE_IO;
>>>               break;
>>>       case ACPI_RESOURCE_TYPE_IRQ:
>>>       case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>>
>>
>> --
>> I speak only for myself.
>> Rafael J. Wysocki, Intel Open Source Technology Center.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1423113AbbD2Nha (ORCPT <rfc822;w@1wt.eu>);
	Wed, 29 Apr 2015 09:37:30 -0400
Received: from mail-qg0-f48.google.com ([209.85.192.48]:33448 "EHLO
	mail-qg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422876AbbD2NhY (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 29 Apr 2015 09:37:24 -0400
MIME-Version: 1.0
In-Reply-To: <5540DD9C.9030707@linux.intel.com>
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com>
 <3492366.RzMG5dcPfN@vostro.rjw.lan> <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com>
 <5540DD9C.9030707@linux.intel.com>
From: Bjorn Helgaas <bhelgaas@google.com>
Date: Wed, 29 Apr 2015 08:37:03 -0500
Message-ID: <CAErSpo7D_CDM+1mzO0ADdzbAE9xwnwiyJ-dk0WFthr+ReKpidg@mail.gmail.com>
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
To: Jiang Liu <jiang.liu@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>,
        "x86@kernel.org" <x86@kernel.org>, Len Brown <lenb@kernel.org>,
        Lv Zheng <lv.zheng@intel.com>, LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Apr 29, 2015 at 8:33 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> On 2015/4/29 21:20, Bjorn Helgaas wrote:
>> On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, April 20, 2015 11:08:58 AM Jiang Liu wrote:

>>>> +      *
>>>> +      * Another possible solution is to explicitly filter out IO
>>>> +      * port[0xCF8-0xCFF].
>>>> +      */
>>>> +     res_flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_IO_FIXED;
>>
>> 2) The usage of IORESOURCE_IO_FIXED here seems like a hack.  It's not
>> related to other existing use of IORESOURCE_IO_FIXED, and it's not
>> intuitive that supplying IORESOURCE_MEM means "I want all the
>> memory-type resources," but supplying IORESOURCE_IO_FIXED means "I
>> *don't* want the fixed I/O-type resources."
>
> Yes, the IORESOURCE_IO_FIXED is a hack here. Then how about explicitly
> filtering IOPORT [0xCF8-0xCFF] so we could avoid such a hack.

I'm OK with that, although it should be in an arch-specific hook or something.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1423086AbbD2Nxl (ORCPT <rfc822;w@1wt.eu>);
	Wed, 29 Apr 2015 09:53:41 -0400
Received: from mga11.intel.com ([192.55.52.93]:8407 "EHLO mga11.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422821AbbD2Nxg (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 29 Apr 2015 09:53:36 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.11,671,1422950400"; 
   d="scan'208";a="563673194"
Message-ID: <5540E259.4040105@linux.intel.com>
Date: Wed, 29 Apr 2015 21:53:29 +0800
From: Jiang Liu <jiang.liu@linux.intel.com>
Organization: Intel
User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <bhelgaas@google.com>
CC: Ingo Molnar <mingo@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com> <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com> <5540DD9C.9030707@linux.intel.com> <2290160.KqiWSgJfFU@vostro.rjw.lan>
In-Reply-To: <2290160.KqiWSgJfFU@vostro.rjw.lan>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 2015/4/29 22:15, Rafael J. Wysocki wrote:
> On Wednesday, April 29, 2015 09:33:16 PM Jiang Liu wrote:
>> On 2015/4/29 21:20, Bjorn Helgaas wrote:
>>> On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Monday, April 20, 2015 11:08:58 AM Jiang Liu 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. 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.
>>>>>
>>>>> But the ACPI and PCI Firmware specifications haven't provided a method
>>>>> to tell whether a resource is consumed by the host bridge itself.
>>>>> So 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.
>>>>>
>>>>> So revert to the behavior before v3.19 to fix the regression.
>>>>>
>>>>> There is also a discussion about ignoring the Producer/Consumer flag on
>>>>> IA64 platforms at:
>>>>> http://patchwork.ozlabs.org/patch/461633/
>>>>>
>>>>> Related ACPI table are archived at:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>>
>>>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>>> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>>
>>>> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
>>>> this area ...
>>>
>>>>> ---
>>>>>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
>>>>>  drivers/acpi/resource.c |    6 +++++-
>>>>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>>>> index e4695985f9de..fc2da98985c3 100644
>>>>> --- a/arch/x86/pci/acpi.c
>>>>> +++ b/arch/x86/pci/acpi.c
>>>>> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>>  {
>>>>>       int ret;
>>>>>       struct resource_entry *entry, *tmp;
>>>>> +     unsigned long res_flags;
>>>>>
>>>>>       sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>>>>>       info->bridge = device;
>>>>> +
>>>>> +     /*
>>>>> +      * An IO or MMIO resource assigned to PCI host bridge may be consumed
>>>>> +      * by the host bridge itself or available to its child bus/devices.
>>>>> +      * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
>>>>> +      * be available to child bus/devices except one special case:
>>>>> +      *      IO port [0xCF8-0xCFF] is consumed by host bridge itself to
>>>>> +      *      access PCI configuration space.
>>>>> +      *
>>>>> +      * Due to lack of specification to define resources consumed by host
>>>>> +      * bridge itself, all IO port resources defined by acpi_resource_io
>>>>> +      * and acpi_resource_fixed_io are ignored to filter out IO
>>>>> +      * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
>>>>> +      * it's not perfect.
>>>
>>> 1) I think it's misleading to say "the specs haven't provided a
>>> method."  As far as I can tell, the Producer/Consumer bit is intended
>>> precisely to distinguish resources consumed by a bridge from those
>>> forwarded to downstream devices.  It would be more accurate to say
>>> "the spec defines a bit, but firmware hasn't used that bit
>>> consistently, so we can't rely on it."
>>
>> Hi Bjorn,
>> 	Thanks for review, I will refine the words as suggested by you.
>>
>>> If you want to say "it's not perfect," it would be useful to mention
>>> the ways in which it is not perfect.  This code is still a candidate
>>> for unification with ia64 and arm64, so we should avoid x86-specific
>>> things here as much as possible.
>>
>> Yes, I have another pending patch set to consolidate IA64 and x86 code
>> for ACPI PCI root.
> 
> That's OK, but can we please fix the regression first before doing that
> unification?  Like to make life easier for the "stable" people and
> whoever wants to backport the fix?

Hi Rafael,
	Yes, I will fix the regression first before sending out the
pending patch set.
Thanks!
Gerry

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1423046AbbD2Nup (ORCPT <rfc822;w@1wt.eu>);
	Wed, 29 Apr 2015 09:50:45 -0400
Received: from v094114.home.net.pl ([79.96.170.134]:49323 "HELO
	v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with SMTP id S1422821AbbD2Num (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 29 Apr 2015 09:50:42 -0400
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Jiang Liu <jiang.liu@linux.intel.com>, Bjorn Helgaas <bhelgaas@google.com>
Cc: Ingo Molnar <mingo@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Subject: Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Wed, 29 Apr 2015 16:15:34 +0200
Message-ID: <2290160.KqiWSgJfFU@vostro.rjw.lan>
User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; )
In-Reply-To: <5540DD9C.9030707@linux.intel.com>
References: <1429499338-16967-1-git-send-email-jiang.liu@linux.intel.com> <CAErSpo4zBFUBwSJjF1CuB8w06CDwYx+ZZaL1X71Joi0uMCObbg@mail.gmail.com> <5540DD9C.9030707@linux.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wednesday, April 29, 2015 09:33:16 PM Jiang Liu wrote:
> On 2015/4/29 21:20, Bjorn Helgaas wrote:
> > On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> On Monday, April 20, 2015 11:08:58 AM Jiang Liu 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. 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.
> >>>
> >>> But the ACPI and PCI Firmware specifications haven't provided a method
> >>> to tell whether a resource is consumed by the host bridge itself.
> >>> So 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.
> >>>
> >>> So revert to the behavior before v3.19 to fix the regression.
> >>>
> >>> There is also a discussion about ignoring the Producer/Consumer flag on
> >>> IA64 platforms at:
> >>> http://patchwork.ozlabs.org/patch/461633/
> >>>
> >>> Related ACPI table are archived at:
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
> >>>
> >>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
> >>> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
> >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>
> >> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
> >> this area ...
> > 
> >>> ---
> >>>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
> >>>  drivers/acpi/resource.c |    6 +++++-
> >>>  2 files changed, 27 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> >>> index e4695985f9de..fc2da98985c3 100644
> >>> --- a/arch/x86/pci/acpi.c
> >>> +++ b/arch/x86/pci/acpi.c
> >>> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
> >>>  {
> >>>       int ret;
> >>>       struct resource_entry *entry, *tmp;
> >>> +     unsigned long res_flags;
> >>>
> >>>       sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
> >>>       info->bridge = device;
> >>> +
> >>> +     /*
> >>> +      * An IO or MMIO resource assigned to PCI host bridge may be consumed
> >>> +      * by the host bridge itself or available to its child bus/devices.
> >>> +      * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
> >>> +      * be available to child bus/devices except one special case:
> >>> +      *      IO port [0xCF8-0xCFF] is consumed by host bridge itself to
> >>> +      *      access PCI configuration space.
> >>> +      *
> >>> +      * Due to lack of specification to define resources consumed by host
> >>> +      * bridge itself, all IO port resources defined by acpi_resource_io
> >>> +      * and acpi_resource_fixed_io are ignored to filter out IO
> >>> +      * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
> >>> +      * it's not perfect.
> > 
> > 1) I think it's misleading to say "the specs haven't provided a
> > method."  As far as I can tell, the Producer/Consumer bit is intended
> > precisely to distinguish resources consumed by a bridge from those
> > forwarded to downstream devices.  It would be more accurate to say
> > "the spec defines a bit, but firmware hasn't used that bit
> > consistently, so we can't rely on it."
> 
> Hi Bjorn,
> 	Thanks for review, I will refine the words as suggested by you.
> 
> > If you want to say "it's not perfect," it would be useful to mention
> > the ways in which it is not perfect.  This code is still a candidate
> > for unification with ia64 and arm64, so we should avoid x86-specific
> > things here as much as possible.
> 
> Yes, I have another pending patch set to consolidate IA64 and x86 code
> for ACPI PCI root.

That's OK, but can we please fix the regression first before doing that
unification?  Like to make life easier for the "stable" people and
whoever wants to backport the fix?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751899AbbD3EjQ (ORCPT <rfc822;w@1wt.eu>);
	Thu, 30 Apr 2015 00:39:16 -0400
Received: from mga09.intel.com ([134.134.136.24]:52133 "EHLO mga09.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750871AbbD3EjM (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 30 Apr 2015 00:39:12 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.11,675,1422950400"; 
   d="scan'208";a="487505605"
From: Jiang Liu <jiang.liu@linux.intel.com>
To: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Len Brown <lenb@kernel.org>
Cc: Jiang Liu <jiang.liu@linux.intel.com>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: [Bugfix v6] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Thu, 30 Apr 2015 12:41:28 +0800
Message-Id: <1430368888-21402-1-git-send-email-jiang.liu@linux.intel.com>
X-Mailer: git-send-email 1.7.10.4
In-Reply-To: <CAErSpo7D_CDM+1mzO0ADdzbAE9xwnwiyJ-dk0WFthr+ReKpidg@mail.gmail.com>
References: <CAErSpo7D_CDM+1mzO0ADdzbAE9xwnwiyJ-dk0WFthr+ReKpidg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@wvnet.at>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Cc: <stable@vger.kernel.org> # 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))
 				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


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751320AbbD3N2m (ORCPT <rfc822;w@1wt.eu>);
	Thu, 30 Apr 2015 09:28:42 -0400
Received: from mail-qg0-f43.google.com ([209.85.192.43]:35664 "EHLO
	mail-qg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750766AbbD3N2j (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 30 Apr 2015 09:28:39 -0400
MIME-Version: 1.0
In-Reply-To: <1430368888-21402-1-git-send-email-jiang.liu@linux.intel.com>
References: <CAErSpo7D_CDM+1mzO0ADdzbAE9xwnwiyJ-dk0WFthr+ReKpidg@mail.gmail.com>
 <1430368888-21402-1-git-send-email-jiang.liu@linux.intel.com>
From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 30 Apr 2015 08:28:17 -0500
Message-ID: <CAErSpo4XrOx4DmR-xUzy45UxS6iErANJbojC_X1JVdjsAzL8WQ@mail.gmail.com>
Subject: Re: [Bugfix v6] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
To: Jiang Liu <jiang.liu@linux.intel.com>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        Len Brown <lenb@kernel.org>, Lv Zheng <lv.zheng@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Apr 29, 2015 at 11:41 PM, Jiang Liu <jiang.liu@linux.intel.com> 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@wvnet.at>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> Cc: <stable@vger.kernel.org> # 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@google.com>

>                                 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
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753031AbbD3Tv7 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 30 Apr 2015 15:51:59 -0400
Received: from v094114.home.net.pl ([79.96.170.134]:61872 "HELO
	v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with SMTP id S1752882AbbD3Tv5 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 30 Apr 2015 15:51:57 -0400
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Bjorn Helgaas <bhelgaas@google.com>, Ingo Molnar <mingo@redhat.com>
Cc: Jiang Liu <jiang.liu@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>,
        "x86@kernel.org" <x86@kernel.org>, Len Brown <lenb@kernel.org>,
        Lv Zheng <lv.zheng@intel.com>, LKML <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Subject: Re: [Bugfix v6] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
Date: Thu, 30 Apr 2015 22:16:50 +0200
Message-ID: <2420339.5qc3EhDri2@vostro.rjw.lan>
User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; )
In-Reply-To: <CAErSpo4XrOx4DmR-xUzy45UxS6iErANJbojC_X1JVdjsAzL8WQ@mail.gmail.com>
References: <CAErSpo7D_CDM+1mzO0ADdzbAE9xwnwiyJ-dk0WFthr+ReKpidg@mail.gmail.com> <1430368888-21402-1-git-send-email-jiang.liu@linux.intel.com> <CAErSpo4XrOx4DmR-xUzy45UxS6iErANJbojC_X1JVdjsAzL8WQ@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thursday, April 30, 2015 08:28:17 AM Bjorn Helgaas wrote:
> On Wed, Apr 29, 2015 at 11:41 PM, Jiang Liu <jiang.liu@linux.intel.com> 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@wvnet.at>
> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # 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@google.com>

Awesome, thanks!

I'm sending my pull request for -rc2 shortly, so I'll queue this one up for
the next week.  I'll follow your subject suggestion.

Ingo, please let me know if there's any problem with that.

Rafael


