PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

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

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

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

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