diff mbox

PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

Message ID CAE9FiQXD7hvt0cUYMzvq0r6CbZn=iJZc7BHRaF2nACbG7qwuBQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Yinghai Lu June 17, 2014, 11:39 p.m. UTC
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

Comments

Andreas Noever June 18, 2014, 10:40 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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