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

login
register
mail settings
Submitter Yinghai Lu
Date June 1, 2013, 6:03 a.m.
Message ID <1370066593-22736-3-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/248055/
State Superseded
Headers show

Comments

Yinghai Lu - June 1, 2013, 6:03 a.m.
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
Bjorn Helgaas - June 25, 2013, 9:15 p.m.
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
Benjamin Herrenschmidt - June 25, 2013, 9:38 p.m.
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
Bjorn Helgaas - June 25, 2013, 9:46 p.m.
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
Yinghai Lu - June 26, 2013, 7:38 a.m.
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

Patch

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