[Bugfix] x86/PCI: Fix regression caused by commit 4d6b4e69a245

Commit Message

Jiang Liu Nov. 16, 2015, 4:27 a.m. UTC

From: Liu Jiang <[e-mail]>

Commit 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support
PCI host bridge") converted x86 to use the common interface
acpi_pci_root_create, but the conversion missed on code piece in
arch/x86/pci/bus_numa.c, which causes regression on some legacy
AMD platforms as reported by Arthur Marsh <[e-mail]>.
The root causes is that acpi_pci_root_create() fails to insert
host bridge resources into iomem_resource/ioport_resource because
x86_pci_root_bus_resources() has already inserted those resources.
So change x86_pci_root_bus_resources() to not insert resources into
iomem_resource/ioport_resource.

Signed-off-by: Jiang Liu <[e-mail]>
Reported-and-tested-by: Arthur Marsh <[e-mail]>
Cc: Keith Busch <[e-mail]>
Cc: Arthur Marsh <[e-mail]>
---
 arch/x86/pci/bus_numa.c |   16 +++-------------
 drivers/acpi/pci_root.c |    7 +++++++
 2 files changed, 10 insertions(+), 13 deletions(-)

Comments

Bjorn Helgaas Nov. 24, 2015, 4:49 p.m. UTC | #1

On Mon, Nov 16, 2015 at 12:27:37PM +0800, Jiang Liu wrote:
> From: Liu Jiang <[e-mail]>
> 
> Commit 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support
> PCI host bridge") converted x86 to use the common interface
> acpi_pci_root_create, but the conversion missed on code piece in
> arch/x86/pci/bus_numa.c, which causes regression on some legacy
> AMD platforms as reported by Arthur Marsh <[e-mail]>.
> The root causes is that acpi_pci_root_create() fails to insert
> host bridge resources into iomem_resource/ioport_resource because
> x86_pci_root_bus_resources() has already inserted those resources.
> So change x86_pci_root_bus_resources() to not insert resources into
> iomem_resource/ioport_resource.

Fixes: 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support PCI host bridge")

> Signed-off-by: Jiang Liu <[e-mail]>
> Reported-and-tested-by: Arthur Marsh <[e-mail]>

What's the status of this?  It looks like a regression we need to fix
for v4.4.

AFAICT, Arthur did *not* test this patch (rather, his response says he
did test it and the test failed).

4d6b4e69a245 was merged by Rafael, and I assume he'll merge the fix
unless I hear otherwise.

Bjorn

> Cc: Keith Busch <[e-mail]>
> Cc: Arthur Marsh <[e-mail]>
> ---
>  arch/x86/pci/bus_numa.c |   16 +++-------------
>  drivers/acpi/pci_root.c |    7 +++++++
>  2 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/pci/bus_numa.c b/arch/x86/pci/bus_numa.c
> index 7bcf06a7cd12..ce53b5b64f51 100644
> --- a/arch/x86/pci/bus_numa.c
> +++ b/arch/x86/pci/bus_numa.c
> @@ -41,27 +41,17 @@ void x86_pci_root_bus_resources(int bus, struct list_head *resources)
>  	       bus);
>  
>  	/* already added by acpi ? */
> -	resource_list_for_each_entry(window, resources)
> +	resource_list_for_each_entry(window, &info->resources)
>  		if (window->res->flags & IORESOURCE_BUS) {
>  			found = true;
>  			break;
>  		}
> -
>  	if (!found)
>  		pci_add_resource(resources, &info->busn);
>  
> -	list_for_each_entry(root_res, &info->resources, list) {
> -		struct resource *res;
> -		struct resource *root;
> +	list_for_each_entry(root_res, &info->resources, list)
> +		pci_add_resource(resources, &root_res->res);
>  
> -		res = &root_res->res;
> -		pci_add_resource(resources, res);
> -		if (res->flags & IORESOURCE_IO)
> -			root = &ioport_resource;
> -		else
> -			root = &iomem_resource;
> -		insert_resource(root, res);
> -	}
>  	return;
>  
>  default_resources:
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 850d7bf0c873..ae3fe4e64203 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -768,6 +768,13 @@ static void pci_acpi_root_add_resources(struct acpi_pci_root_info *info)
>  		else
>  			continue;
>  
> +		/*
> +		 * Some legacy x86 host bridge drivers use iomem_resource and
> +		 * ioport_resource as default resource pool, skip it.
> +		 */
> +		if (res == root)
> +			continue;
> +
>  		conflict = insert_resource_conflict(root, res);
>  		if (conflict) {
>  			dev_info(&info->bridge->dev,
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Nov. 24, 2015, 10:19 p.m. UTC | #2

On Tue, Nov 24, 2015 at 5:49 PM, Bjorn Helgaas <[e-mail]> wrote:
> On Mon, Nov 16, 2015 at 12:27:37PM +0800, Jiang Liu wrote:
>> From: Liu Jiang <[e-mail]>
>>
>> Commit 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support
>> PCI host bridge") converted x86 to use the common interface
>> acpi_pci_root_create, but the conversion missed on code piece in
>> arch/x86/pci/bus_numa.c, which causes regression on some legacy
>> AMD platforms as reported by Arthur Marsh <[e-mail]>.
>> The root causes is that acpi_pci_root_create() fails to insert
>> host bridge resources into iomem_resource/ioport_resource because
>> x86_pci_root_bus_resources() has already inserted those resources.
>> So change x86_pci_root_bus_resources() to not insert resources into
>> iomem_resource/ioport_resource.
>
> Fixes: 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support PCI host bridge")
>
>> Signed-off-by: Jiang Liu <[e-mail]>
>> Reported-and-tested-by: Arthur Marsh <[e-mail]>
>
> What's the status of this?  It looks like a regression we need to fix
> for v4.4.
>
> AFAICT, Arthur did *not* test this patch (rather, his response says he
> did test it and the test failed).
>
> 4d6b4e69a245 was merged by Rafael, and I assume he'll merge the fix
> unless I hear otherwise.

Quite frankly, I'm more likely to revert the offending commit at this
point as that's not the only regression reported against it and the
fix only helps in one case (out of three known to me).

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Keith Busch Nov. 24, 2015, 11:04 p.m. UTC | #3

On Tue, Nov 24, 2015 at 11:19:34PM +0100, Rafael J. Wysocki wrote:
> Quite frankly, I'm more likely to revert the offending commit at this
> point as that's not the only regression reported against it and the
> fix only helps in one case (out of three known to me).

Using 4.4-rc1 and can confirm the patch fixes my regression report. The
revert also fixes it, so either way is good for me!
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jiang Liu Nov. 25, 2015, 8:12 a.m. UTC | #4

On 2015/11/25 6:19, Rafael J. Wysocki wrote:
> On Tue, Nov 24, 2015 at 5:49 PM, Bjorn Helgaas <[e-mail]> wrote:
>> On Mon, Nov 16, 2015 at 12:27:37PM +0800, Jiang Liu wrote:
>>> From: Liu Jiang <[e-mail]>
>>>
>>> Commit 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support
>>> PCI host bridge") converted x86 to use the common interface
>>> acpi_pci_root_create, but the conversion missed on code piece in
>>> arch/x86/pci/bus_numa.c, which causes regression on some legacy
>>> AMD platforms as reported by Arthur Marsh <[e-mail]>.
>>> The root causes is that acpi_pci_root_create() fails to insert
>>> host bridge resources into iomem_resource/ioport_resource because
>>> x86_pci_root_bus_resources() has already inserted those resources.
>>> So change x86_pci_root_bus_resources() to not insert resources into
>>> iomem_resource/ioport_resource.
>>
>> Fixes: 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support PCI host bridge")
>>
>>> Signed-off-by: Jiang Liu <[e-mail]>
>>> Reported-and-tested-by: Arthur Marsh <[e-mail]>
>>
>> What's the status of this?  It looks like a regression we need to fix
>> for v4.4.
>>
>> AFAICT, Arthur did *not* test this patch (rather, his response says he
>> did test it and the test failed).
>>
>> 4d6b4e69a245 was merged by Rafael, and I assume he'll merge the fix
>> unless I hear otherwise.
> 
> Quite frankly, I'm more likely to revert the offending commit at this
> point as that's not the only regression reported against it and the
> fix only helps in one case (out of three known to me).
Hi Rafael,
	I got regression report from Hans de Bruin<[e-mail]>,
Keith Busch <[e-mail]>, and Arthur Marsh
<[e-mail]>. Hans and Keith also reports
the patch fixes the regression. For Arthur's case, the debug
patch works for him, but the formal patch based on the debug
patch fails, so I need to do more investigation about this.
Is there any other report related to commit 4d6b4e69a245 so
I could help to investigate?
Thanks,
Gerry

> 
> Thanks,
> Rafael
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wysocki, Rafael J Nov. 25, 2015, 1:11 p.m. UTC | #5

On 11/25/2015 9:12 AM, Jiang Liu wrote:
> On 2015/11/25 6:19, Rafael J. Wysocki wrote:
>> On Tue, Nov 24, 2015 at 5:49 PM, Bjorn Helgaas <[e-mail]> wrote:
>>> On Mon, Nov 16, 2015 at 12:27:37PM +0800, Jiang Liu wrote:
>>>> From: Liu Jiang <[e-mail]>
>>>>
>>>> Commit 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support
>>>> PCI host bridge") converted x86 to use the common interface
>>>> acpi_pci_root_create, but the conversion missed on code piece in
>>>> arch/x86/pci/bus_numa.c, which causes regression on some legacy
>>>> AMD platforms as reported by Arthur Marsh <[e-mail]>.
>>>> The root causes is that acpi_pci_root_create() fails to insert
>>>> host bridge resources into iomem_resource/ioport_resource because
>>>> x86_pci_root_bus_resources() has already inserted those resources.
>>>> So change x86_pci_root_bus_resources() to not insert resources into
>>>> iomem_resource/ioport_resource.
>>> Fixes: 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support PCI host bridge")
>>>
>>>> Signed-off-by: Jiang Liu <[e-mail]>
>>>> Reported-and-tested-by: Arthur Marsh <[e-mail]>
>>> What's the status of this?  It looks like a regression we need to fix
>>> for v4.4.
>>>
>>> AFAICT, Arthur did *not* test this patch (rather, his response says he
>>> did test it and the test failed).
>>>
>>> 4d6b4e69a245 was merged by Rafael, and I assume he'll merge the fix
>>> unless I hear otherwise.
>> Quite frankly, I'm more likely to revert the offending commit at this
>> point as that's not the only regression reported against it and the
>> fix only helps in one case (out of three known to me).
> Hi Rafael,
> 	I got regression report from Hans de Bruin<[e-mail]>,
> Keith Busch <[e-mail]>, and Arthur Marsh
> <[e-mail]>. Hans and Keith also reports
> the patch fixes the regression. For Arthur's case, the debug
> patch works for him, but the formal patch based on the debug
> patch fails, so I need to do more investigation about this.
> Is there any other report related to commit 4d6b4e69a245 so
> I could help to investigate?

OK, so the proposed bug fix works for everybody, right?

In that case there's no reason not to apply it.

If there's anyone who can reproduce the problem and for whom that patch 
doesn't work, please let me know.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/x86/pci/bus_numa.c b/arch/x86/pci/bus_numa.c
index 7bcf06a7cd12..ce53b5b64f51 100644
--- a/arch/x86/pci/bus_numa.c
+++ b/arch/x86/pci/bus_numa.c
@@ -41,27 +41,17 @@  void x86_pci_root_bus_resources(int bus, struct list_head *resources)
 	       bus);
 
 	/* already added by acpi ? */
-	resource_list_for_each_entry(window, resources)
+	resource_list_for_each_entry(window, &info->resources)
 		if (window->res->flags & IORESOURCE_BUS) {
 			found = true;
 			break;
 		}
-
 	if (!found)
 		pci_add_resource(resources, &info->busn);
 
-	list_for_each_entry(root_res, &info->resources, list) {
-		struct resource *res;
-		struct resource *root;
+	list_for_each_entry(root_res, &info->resources, list)
+		pci_add_resource(resources, &root_res->res);
 
-		res = &root_res->res;
-		pci_add_resource(resources, res);
-		if (res->flags & IORESOURCE_IO)
-			root = &ioport_resource;
-		else
-			root = &iomem_resource;
-		insert_resource(root, res);
-	}
 	return;
 
 default_resources:
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf0c873..ae3fe4e64203 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -768,6 +768,13 @@  static void pci_acpi_root_add_resources(struct acpi_pci_root_info *info)
 		else
 			continue;
 
+		/*
+		 * Some legacy x86 host bridge drivers use iomem_resource and
+		 * ioport_resource as default resource pool, skip it.
+		 */
+		if (res == root)
+			continue;
+
 		conflict = insert_resource_conflict(root, res);
 		if (conflict) {
 			dev_info(&info->bridge->dev,