[PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Andreas Noever]

pci_assign_unassigned_bridge_resources is used to assign resources below
a hotplug bridge. If the first attempt fails it will release some
resources and try again. If a resource allocation on the hotplug bridge
itself fails then pci_assign_unassigned_bridge_resources will invoke
pci_bus_release_bridge_resources on the parent bus of the hotplug
bridge. This in turn will release resources assigned to siblings of the
hotplug bridge which may already be in use.

This patch checks for this case and prevents
pci_bus_release_bridge_resources to be invoked on the parent bus.

The problem can be reproduced by having two sibling hotplug bridges A
and B. The problem will occour if the parent of A and B does not have
enough resources to satisfy window allocations for B during a hotplug
event.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---

I must admit that I do not fully understand how
pci_assign_unassigned_bridge_resources works. Under which scenario would the
second allocation attempt be successful?

Thanks,
Andreas

 drivers/pci/setup-bus.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 138bdd6..2e418d6 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1560,6 +1560,12 @@ void __init pci_assign_unassigned_resources(void)
 		pci_assign_unassigned_root_bus_resources(root_bus);
 }
 
+/**
+ * pci_assign_unassigned_bridge_resources - greenfield resource allocation
+ *
+ * Try to assign io and memory resources on and below @bridge. The caller must
+ * ensure that no device below @bridge is active.
+ */
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 {
 	struct pci_bus *parent = bridge->subordinate;
@@ -1567,7 +1573,7 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 					want additional resources */
 	int tried_times = 0;
 	LIST_HEAD(fail_head);
-	struct pci_dev_resource *fail_res;
+	struct pci_dev_resource *fail_res, *tmp;
 	int retval;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
 				  IORESOURCE_PREFETCH;
@@ -1594,10 +1600,21 @@ again:
 	 * Try to release leaf bridge's resources that doesn't fit resource of
 	 * child device under that bridge
 	 */
-	list_for_each_entry(fail_res, &fail_head, list)
+	list_for_each_entry_safe(fail_res, tmp, &fail_head, list) {
+		 /*
+		  * The allocation of the mem/io window of the top level bridge
+		  * can fail.  Without the following check we would release our
+		  * siblings' resources.
+		  */
+		if (fail_res->dev == bridge) {
+			list_del(&fail_res->list);
+			kfree(fail_res);
+			continue;
+		}
 		pci_bus_release_bridge_resources(fail_res->dev->bus,
 						 fail_res->flags & type_mask,
 						 whole_subtree);
+	}
 
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {
-- 
2.0.0

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Bjorn Helgaas]

[+cc Yinghai]

On Mon, Jun 09, 2014 at 10:45:30PM +0200, Andreas Noever wrote:
> pci_assign_unassigned_bridge_resources is used to assign resources below
> a hotplug bridge. If the first attempt fails it will release some
> resources and try again. If a resource allocation on the hotplug bridge
> itself fails then pci_assign_unassigned_bridge_resources will invoke
> pci_bus_release_bridge_resources on the parent bus of the hotplug
> bridge. This in turn will release resources assigned to siblings of the
> hotplug bridge which may already be in use.
> 
> This patch checks for this case and prevents
> pci_bus_release_bridge_resources to be invoked on the parent bus.
> 
> The problem can be reproduced by having two sibling hotplug bridges A
> and B. The problem will occour if the parent of A and B does not have
> enough resources to satisfy window allocations for B during a hotplug
> event.
> 
> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
> ---
> 
> I must admit that I do not fully understand how
> pci_assign_unassigned_bridge_resources works. Under which scenario would the
> second allocation attempt be successful?

I don't understand how all this works either.  Yinghai?

We definitely don't want to release resources that are already in use.  Can
you review and ack or nack this?

>  drivers/pci/setup-bus.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 138bdd6..2e418d6 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1560,6 +1560,12 @@ void __init pci_assign_unassigned_resources(void)
>  		pci_assign_unassigned_root_bus_resources(root_bus);
>  }
>  
> +/**
> + * pci_assign_unassigned_bridge_resources - greenfield resource allocation
> + *
> + * Try to assign io and memory resources on and below @bridge. The caller must
> + * ensure that no device below @bridge is active.
> + */
>  void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  {
>  	struct pci_bus *parent = bridge->subordinate;
> @@ -1567,7 +1573,7 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  					want additional resources */
>  	int tried_times = 0;
>  	LIST_HEAD(fail_head);
> -	struct pci_dev_resource *fail_res;
> +	struct pci_dev_resource *fail_res, *tmp;
>  	int retval;
>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>  				  IORESOURCE_PREFETCH;
> @@ -1594,10 +1600,21 @@ again:
>  	 * Try to release leaf bridge's resources that doesn't fit resource of
>  	 * child device under that bridge
>  	 */
> -	list_for_each_entry(fail_res, &fail_head, list)
> +	list_for_each_entry_safe(fail_res, tmp, &fail_head, list) {
> +		 /*
> +		  * The allocation of the mem/io window of the top level bridge
> +		  * can fail.  Without the following check we would release our
> +		  * siblings' resources.
> +		  */
> +		if (fail_res->dev == bridge) {
> +			list_del(&fail_res->list);
> +			kfree(fail_res);
> +			continue;
> +		}
>  		pci_bus_release_bridge_resources(fail_res->dev->bus,
>  						 fail_res->flags & type_mask,
>  						 whole_subtree);
> +	}
>  
>  	/* restore size and flags */
>  	list_for_each_entry(fail_res, &fail_head, list) {
> -- 
> 2.0.0
> 

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Tue, Jun 17, 2014 at 3:16 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Yinghai]
>
> On Mon, Jun 09, 2014 at 10:45:30PM +0200, Andreas Noever wrote:
>> The problem can be reproduced by having two sibling hotplug bridges A
>> and B. The problem will occour if the parent of A and B does not have
>> enough resources to satisfy window allocations for B during a hotplug
>> event.

> I don't understand how all this works either.  Yinghai?
>
> We definitely don't want to release resources that are already in use.  Can
> you review and ack or nack this?

Hi Andreas,

Can you check if attached patch fix the problem on your test case?

In some case, if we can not assign pref mmio properly for the bridge,
we may need to even clear non-pref mmio for the bridge.

Thanks

Yinghai

[-- Attachment #2: fix_release_sibling_bridge_res.patch --]
[-- Type: text/x-patch, Size: 1090 bytes --]

Subject: [PATCH] pci: Don't release sibiling bridge resources

On hotplug case, we should not touch sibling bridges that is out
side of the slots.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1676,10 +1676,16 @@ again:
 	 * Try to release leaf bridge's resources that doesn't fit resource of
 	 * child device under that bridge
 	 */
-	list_for_each_entry(fail_res, &fail_head, list)
-		pci_bus_release_bridge_resources(fail_res->dev->bus,
+	list_for_each_entry(fail_res, &fail_head, list) {
+		struct pci_bus *bus = fail_res->dev->bus;
+
+		if (fail_res->dev == bridge)
+			bus = bridge->subordinate;
+
+		pci_bus_release_bridge_resources(bus,
 						 fail_res->flags & type_mask,
 						 whole_subtree);
+	}
 
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Andreas Noever]

On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jun 17, 2014 at 3:16 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc Yinghai]
>>
>> On Mon, Jun 09, 2014 at 10:45:30PM +0200, Andreas Noever wrote:
>>> The problem can be reproduced by having two sibling hotplug bridges A
>>> and B. The problem will occour if the parent of A and B does not have
>>> enough resources to satisfy window allocations for B during a hotplug
>>> event.
>
>> I don't understand how all this works either.  Yinghai?
>>
>> We definitely don't want to release resources that are already in use.  Can
>> you review and ack or nack this?
>
> Hi Andreas,
>
> Can you check if attached patch fix the problem on your test case?

It seems to fix the testcase (no unwanted resources are released). But
why do you reassign bus and not just skip the top level bridge? If one
of the allocations below bridge failed then a resource of that device
will be in fail_res and bridge->subordinate will get released anyways?
Also by not removing fail_res from the list you trigger the code in
the next loop for the top level bridge (in particular the res->flags =
0 line looks dangerous).

Could you explain why this function attempts to assign resources two
times? In which scenario will a second attempt be successful?

Thanks,
Andreas

> In some case, if we can not assign pref mmio properly for the bridge,
> we may need to even clear non-pref mmio for the bridge.
>
> Thanks
>
> Yinghai

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On Wed, Jun 18, 2014 at 3:40 PM, Andreas Noever
<andreas.noever@gmail.com> wrote:
> On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> It seems to fix the testcase (no unwanted resources are released). But
> why do you reassign bus and not just skip the top level bridge? If one
> of the allocations below bridge failed then a resource of that device
> will be in fail_res and bridge->subordinate will get released anyways?
> Also by not removing fail_res from the list you trigger the code in
> the next loop for the top level bridge (in particular the res->flags =
> 0 line looks dangerous).

Should not be dangerous, just second try.

>
> Could you explain why this function attempts to assign resources two
> times? In which scenario will a second attempt be successful?

For example, at first mmio is assigned (by firmware), but pref mmio fails,
then before second try,  mmio get cleared, then we could separate
mmio and mmio pref. So need to try again for pref mmio.

Also I missed one MEM_64 for hotplug path.

So we need two patches.

Thanks

Yinghai

[-- Attachment #2: fix_release_sibling_bridge_res.patch --]
[-- Type: text/x-patch, Size: 1092 bytes --]

Subject: [PATCH] pci: Don't release sibiling bridge resources

On hotplug case, we should not touch sibling bridges that is out
side of the slots.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1676,10 +1676,16 @@ again:
 	 * Try to release leaf bridge's resources that doesn't fit resource of
 	 * child device under that bridge
 	 */
-	list_for_each_entry(fail_res, &fail_head, list)
-		pci_bus_release_bridge_resources(fail_res->dev->bus,
+	list_for_each_entry(fail_res, &fail_head, list) {
+		struct pci_bus *bus = fail_res->dev->bus;
+
+		if (fail_res->dev == bridge)
+			bus = bridge->subordinate;
+
+		pci_bus_release_bridge_resources(bus,
 						 fail_res->flags & type_mask,
 						 whole_subtree);
+	}
 
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {

[-- Attachment #3: assign_unassigned_bridge_MEM_64.patch --]
[-- Type: text/x-patch, Size: 865 bytes --]

Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path

We miss that in
|  commit 5b28541552ef5eeffc41d6936105f38c2508e566
|    PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
for pci hotplug path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
 	struct pci_dev_resource *fail_res;
 	int retval;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-				  IORESOURCE_PREFETCH;
+				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
 
 again:
 	__pci_bus_size_bridges(parent, &add_list);

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Bjorn Helgaas]

On Wed, Jun 18, 2014 at 09:41:02PM -0700, Yinghai Lu wrote:
> On Wed, Jun 18, 2014 at 3:40 PM, Andreas Noever
> <andreas.noever@gmail.com> wrote:
> > On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > It seems to fix the testcase (no unwanted resources are released). But
> > why do you reassign bus and not just skip the top level bridge? If one
> > of the allocations below bridge failed then a resource of that device
> > will be in fail_res and bridge->subordinate will get released anyways?
> > Also by not removing fail_res from the list you trigger the code in
> > the next loop for the top level bridge (in particular the res->flags =
> > 0 line looks dangerous).
> 
> Should not be dangerous, just second try.

I still don't understand this.  Why do we set "res->flags = 0"?  That
clears out the resource type.  Where do we figure out the type of "res"
again?

> > Could you explain why this function attempts to assign resources two
> > times? In which scenario will a second attempt be successful?
> 
> For example, at first mmio is assigned (by firmware), but pref mmio fails,
> then before second try,  mmio get cleared, then we could separate
> mmio and mmio pref. So need to try again for pref mmio.
> 
> Also I missed one MEM_64 for hotplug path.
> 
> So we need two patches.
> 
> Thanks
> 
> Yinghai

> Subject: [PATCH] pci: Don't release sibiling bridge resources
> 
> On hotplug case, we should not touch sibling bridges that is out
> side of the slots.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -1676,10 +1676,16 @@ again:
>  	 * Try to release leaf bridge's resources that doesn't fit resource of
>  	 * child device under that bridge
>  	 */
> -	list_for_each_entry(fail_res, &fail_head, list)
> -		pci_bus_release_bridge_resources(fail_res->dev->bus,
> +	list_for_each_entry(fail_res, &fail_head, list) {
> +		struct pci_bus *bus = fail_res->dev->bus;
> +
> +		if (fail_res->dev == bridge)
> +			bus = bridge->subordinate;
> +
> +		pci_bus_release_bridge_resources(bus,
>  						 fail_res->flags & type_mask,
>  						 whole_subtree);
> +	}
>  
>  	/* restore size and flags */
>  	list_for_each_entry(fail_res, &fail_head, list) {

> Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path
> 
> We miss that in
> |  commit 5b28541552ef5eeffc41d6936105f38c2508e566
> |    PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
> for pci hotplug path.

This changelog is useless.  I don't have time to spend a few hours
figuring out why we want this change.

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
>  	struct pci_dev_resource *fail_res;
>  	int retval;
>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> -				  IORESOURCE_PREFETCH;
> +				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>  
>  again:
>  	__pci_bus_size_bridges(parent, &add_list);

Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

[Yinghai Lu]

On Mon, Jun 30, 2014 at 3:47 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Should not be dangerous, just second try.
>
> I still don't understand this.  Why do we set "res->flags = 0"?  That
> clears out the resource type.  Where do we figure out the type of "res"
> again?

pci_bridge_check_ranges()

>> Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path
>>
>> We miss that in
>> |  commit 5b28541552ef5eeffc41d6936105f38c2508e566
>> |    PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
>> for pci hotplug path.
>
> This changelog is useless.  I don't have time to spend a few hours
> figuring out why we want this change.

We have MEM_64 in type_mask from pci_assign_unassigned_root_bus_resources().
And I missed the same change for hotplug path.


That is for exact type checking.

Thanks

Yinghai