diff mbox

[RFC,2/3] PCI: Pass available resources into pci_create_bus()

Message ID 1314167063-15785-3-git-send-email-dengcheng.zhu@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Deng-Cheng Zhu Aug. 24, 2011, 6:24 a.m. UTC
Currently, after pci_create_bus(), resources available on the bus could be
handled by pci_scan_child_bus(). The problem is that, in
pci_scan_child_bus(), before calling arch-dependent pcibios_fixup_bus(),
PCI quirks will probably conflict (while doing pci_claim_resource()) with
resources like system controller's I/O resource that have not yet been
added to the bus. So, add those resources right before returning the newly
created bus in pci_create_bus().

A related discussion thread can be found here:
http://www.spinics.net/lists/mips/msg41654.html

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/microblaze/pci/pci-common.c |    3 ++-
 arch/powerpc/kernel/pci-common.c |    3 ++-
 arch/sparc/kernel/pci.c          |    3 ++-
 arch/x86/pci/acpi.c              |    2 +-
 drivers/pci/probe.c              |   15 +++++++++++----
 include/linux/pci.h              |    3 ++-
 6 files changed, 20 insertions(+), 9 deletions(-)

Comments

Bjorn Helgaas Aug. 24, 2011, 2:28 p.m. UTC | #1
On Wed, Aug 24, 2011 at 12:24 AM, Deng-Cheng Zhu
<dengcheng.zhu@gmail.com> wrote:
> Currently, after pci_create_bus(), resources available on the bus could be
> handled by pci_scan_child_bus(). The problem is that, in
> pci_scan_child_bus(), before calling arch-dependent pcibios_fixup_bus(),
> PCI quirks will probably conflict (while doing pci_claim_resource()) with
> resources like system controller's I/O resource that have not yet been
> added to the bus. So, add those resources right before returning the newly
> created bus in pci_create_bus().

I like this approach a lot.  Thanks for working it up.  It's a nice
small change with very little impact to other architectures, and you
have a nice clear changelog.  You might mention something about the
fact that by default, the bus starts out with all of ioport_resource
and iomem_resource -- that will mean something to people who know how
host bridges work.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8473727..7735fe7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1516,7 +1516,8 @@ unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus)
>  }
>
>  struct pci_bus * pci_create_bus(struct device *parent,
> -               int bus, struct pci_ops *ops, void *sysdata)
> +               int bus, struct pci_ops *ops, void *sysdata,
> +               struct pci_bus_resource *bus_res)
>  {
>        int error;
>        struct pci_bus *b, *b2;
> @@ -1570,8 +1571,14 @@ struct pci_bus * pci_create_bus(struct device *parent,
>        pci_create_legacy_files(b);
>
>        b->number = b->secondary = bus;
> -       b->resource[0] = &ioport_resource;
> -       b->resource[1] = &iomem_resource;
> +
> +       /* Add initial resources to the bus */
> +       if (bus_res != NULL) {
> +               list_add_tail(&b->resources, &bus_res->list);
> +       } else {
> +               pci_bus_add_resource(b, &ioport_resource, 0);
> +               pci_bus_add_resource(b, &iomem_resource, 0);
> +       }

Using pci_bus_add_resource() here *seems* like it should be the right
thing, but I don't think it will work correctly.

The problem is that struct pci_bus has both a table of resources
(bus->resource[]) *and* a list (bus->resources).
pci_bus_add_resource() always puts the new resource on the list, but
various arch code still references the table directly, e.g., sparc has
"pbus->resource[0] = &pbm->io_space" in pcibios_fixup_bus().

As written, I think this patch will break sparc because the host
bridge will end up with both pbm->io_space (in the table) and
ioport_resource (in the list).

I think something like this would work, though:

    if (bus_res)
        list_add_tail(&b->resources, &bus_res->list);
    else {
        b->resource[0] = &ioport_resource;
        b->resource[1] = &iomem_resource;
    }

Bjorn
--
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
Deng-Cheng Zhu Aug. 25, 2011, 6:56 a.m. UTC | #2
2011/8/24 Bjorn Helgaas <bhelgaas@google.com>:
> I like this approach a lot.  Thanks for working it up.  It's a nice
> small change with very little impact to other architectures, and you
> have a nice clear changelog.  You might mention something about the
> fact that by default, the bus starts out with all of ioport_resource
> and iomem_resource -- that will mean something to people who know how
> host bridges work.

Thanks! And I'll add this info to the patch description.

> Using pci_bus_add_resource() here *seems* like it should be the right
> thing, but I don't think it will work correctly.
>
> The problem is that struct pci_bus has both a table of resources
> (bus->resource[]) *and* a list (bus->resources).
> pci_bus_add_resource() always puts the new resource on the list, but
> various arch code still references the table directly, e.g., sparc has
> "pbus->resource[0] = &pbm->io_space" in pcibios_fixup_bus().
>
> As written, I think this patch will break sparc because the host
> bridge will end up with both pbm->io_space (in the table) and
> ioport_resource (in the list).

Good catch! I overlooked this point.

> I think something like this would work, though:
>
>    if (bus_res)
>        list_add_tail(&b->resources, &bus_res->list);
>    else {
>        b->resource[0] = &ioport_resource;
>        b->resource[1] = &iomem_resource;
>    }

Yes, it should work.


Thanks again for your time,

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

diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 4cfae20..9c35aa6 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -1581,7 +1581,8 @@  static void __devinit pcibios_scan_phb(struct pci_controller *hose)
 		 node ? node->full_name : "<NO NAME>");
 
 	/* Create an empty bus for the toplevel */
-	bus = pci_create_bus(hose->parent, hose->first_busno, hose->ops, hose);
+	bus = pci_create_bus(hose->parent, hose->first_busno,
+			     hose->ops, hose, NULL);
 	if (bus == NULL) {
 		printk(KERN_ERR "Failed to create bus for PCI domain %04x\n",
 		       hose->global_number);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 32656f1..2ede26a 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1703,7 +1703,8 @@  void __devinit pcibios_scan_phb(struct pci_controller *hose)
 		 node ? node->full_name : "<NO NAME>");
 
 	/* Create an empty bus for the toplevel */
-	bus = pci_create_bus(hose->parent, hose->first_busno, hose->ops, hose);
+	bus = pci_create_bus(hose->parent, hose->first_busno,
+			     hose->ops, hose, NULL);
 	if (bus == NULL) {
 		pr_err("Failed to create bus for PCI domain %04x\n",
 			hose->global_number);
diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index 1e94f94..77c38bb 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -689,7 +689,8 @@  struct pci_bus * __devinit pci_scan_one_pbm(struct pci_pbm_info *pbm,
 
 	printk("PCI: Scanning PBM %s\n", node->full_name);
 
-	bus = pci_create_bus(parent, pbm->pci_first_busno, pbm->pci_ops, pbm);
+	bus = pci_create_bus(parent, pbm->pci_first_busno,
+			     pbm->pci_ops, pbm, NULL);
 	if (!bus) {
 		printk(KERN_ERR "Failed to create bus for %s\n",
 		       node->full_name);
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index c953302..bab2113 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -353,7 +353,7 @@  struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
 		memcpy(bus->sysdata, sd, sizeof(*sd));
 		kfree(sd);
 	} else {
-		bus = pci_create_bus(NULL, busnum, &pci_root_ops, sd);
+		bus = pci_create_bus(NULL, busnum, &pci_root_ops, sd, NULL);
 		if (bus) {
 			get_current_resources(device, busnum, domain, bus);
 			bus->subordinate = pci_scan_child_bus(bus);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8473727..7735fe7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1516,7 +1516,8 @@  unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus)
 }
 
 struct pci_bus * pci_create_bus(struct device *parent,
-		int bus, struct pci_ops *ops, void *sysdata)
+		int bus, struct pci_ops *ops, void *sysdata,
+		struct pci_bus_resource *bus_res)
 {
 	int error;
 	struct pci_bus *b, *b2;
@@ -1570,8 +1571,14 @@  struct pci_bus * pci_create_bus(struct device *parent,
 	pci_create_legacy_files(b);
 
 	b->number = b->secondary = bus;
-	b->resource[0] = &ioport_resource;
-	b->resource[1] = &iomem_resource;
+
+	/* Add initial resources to the bus */
+	if (bus_res != NULL) {
+		list_add_tail(&b->resources, &bus_res->list);
+	} else {
+		pci_bus_add_resource(b, &ioport_resource, 0);
+		pci_bus_add_resource(b, &iomem_resource, 0);
+	}
 
 	return b;
 
@@ -1592,7 +1599,7 @@  struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent,
 {
 	struct pci_bus *b;
 
-	b = pci_create_bus(parent, bus, ops, sysdata);
+	b = pci_create_bus(parent, bus, ops, sysdata, NULL);
 	if (b)
 		b->subordinate = pci_scan_child_bus(b);
 	return b;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8c230cb..5e1bacd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -666,7 +666,8 @@  static inline struct pci_bus * __devinit pci_scan_bus(int bus, struct pci_ops *o
 	return root_bus;
 }
 struct pci_bus *pci_create_bus(struct device *parent, int bus,
-			       struct pci_ops *ops, void *sysdata);
+			       struct pci_ops *ops, void *sysdata,
+			       struct pci_bus_resource *bus_res);
 struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
 				int busnr);
 void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);