[v5,3/7] PCI: Use pci_walk_bus to detect unassigned resources

Per Bjorn, use pci_walk_bus instead of for_each_pci_dev or
calling pci_realloc_detect() recursively, that will make code more readable.

Per Bjorn, separate it from big patch that handing assign_unssigned per root bus.

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

---
 drivers/pci/setup-bus.c |   46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

--
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 Fri, May 31, 2013 at 11:03:08PM -0700, Yinghai Lu wrote:
> Per Bjorn, use pci_walk_bus instead of for_each_pci_dev or
> calling pci_realloc_detect() recursively, that will make code more readable.
> 
> Per Bjorn, separate it from big patch that handing assign_unssigned per root bus.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   46 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 15 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
> @@ -1427,30 +1427,46 @@ static bool __init pci_realloc_enabled(v
>  	return pci_realloc_enable >= user_enabled;
>  }
>  
> -static void __init pci_realloc_detect(void)
> -{
>  #if defined(CONFIG_PCI_IOV) && defined(CONFIG_PCI_REALLOC_ENABLE_AUTO)
> -	struct pci_dev *dev = NULL;
> +static int __init check_unassigned_resources(struct pci_dev *dev, void *data)

I'm not going to add a function named "check_*()" because the name gives no
clue about what the return value means.  If it's a boolean function, the
name should be something like a question that has a yes/no answer.

> +{
> +	int i;
> +	int *unassigned = data;
>  
> -	if (pci_realloc_enable != undefined)
> -		return;
> +	for (i = PCI_IOV_RESOURCES; i <= PCI_IOV_RESOURCE_END; i++) {
> +		struct resource *r = &dev->resource[i];
>  
> -	for_each_pci_dev(dev) {
> -		int i;
> +		/* Not assigned, or rejected by kernel ? */
> +		if (r->flags && !r->start) {
> +			(*unassigned)++;
> +			return 1; /* return early from pci_walk_bus */
> +		}
> +	}
>  
> -		for (i = PCI_IOV_RESOURCES; i <= PCI_IOV_RESOURCE_END; i++) {
> -			struct resource *r = &dev->resource[i];
> +	return 0;
> +}
>  
> -			/* Not assigned, or rejected by kernel ? */
> -			if (r->flags && !r->start) {
> -				pci_realloc_enable = auto_enabled;
> +static void  __init pci_realloc_detect(void)
> +{
> +	int unassigned = 0;
> +	struct pci_bus *bus;
>  
> -				return;
> -			}
> +	if (pci_realloc_enable != undefined)
> +		return;
> +
> +	list_for_each_entry(bus, &pci_root_buses, node) {
> +		pci_walk_bus(bus, check_unassigned_resources, &unassigned);
> +		if (unassigned) {
> +			pci_realloc_enable = auto_enabled;
> +			return;
>  		}
>  	}
> -#endif
>  }
> +#else
> +static void __init pci_realloc_detect(void)
> +{
> +}
> +#endif
>  
>  /*
>   * first try will not touch pci bridge res
--
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 Tue, 2013-06-25 at 15:15 -0600, Bjorn Helgaas wrote:
> -     for_each_pci_dev(dev) {
> > -             int i;
> > +             /* Not assigned, or rejected by kernel ? */
> > +             if (r->flags && !r->start) {
> > +                     (*unassigned)++;
> > +                     return 1; /* return early from pci_walk_bus */
> > +             }
> > +     }

BTW. I'm aware you didn't change that logic but ... it's somewhat broken
in the case where the aperture has an offset. You should compare
r->start with the offset, not with 0.

Cheers,
Ben.


--
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 Tue, Jun 25, 2013 at 3:38 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2013-06-25 at 15:15 -0600, Bjorn Helgaas wrote:
>> -     for_each_pci_dev(dev) {
>> > -             int i;
>> > +             /* Not assigned, or rejected by kernel ? */
>> > +             if (r->flags && !r->start) {
>> > +                     (*unassigned)++;
>> > +                     return 1; /* return early from pci_walk_bus */
>> > +             }
>> > +     }
>
> BTW. I'm aware you didn't change that logic but ... it's somewhat broken
> in the case where the aperture has an offset. You should compare
> r->start with the offset, not with 0.

Yes, please fix that in a separate patch that contains only the bugfix.
--
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 Tue, Jun 25, 2013 at 2:15 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> +static int __init check_unassigned_resources(struct pci_dev *dev, void *data)
>
> I'm not going to add a function named "check_*()" because the name gives no
> clue about what the return value means.  If it's a boolean function, the
> name should be something like a question that has a yes/no answer.

that prototype return int is required by pci_walk_bus().

drivers/pci/bus.c:void pci_walk_bus(struct pci_bus *top, int
(*cb)(struct pci_dev *, void *)

return 1, will return early from pci_walk_bus().

count_unassigned_resources() is not good name too, as we bail out early.
find_unassigned_resources() is more weird, looks like it want to return resource

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