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

[Bjorn Helgaas <bhelgaas@google.com>]

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