[PATCH 0/8] PCI: pci_host_bridge related cleanup

[PATCH 0/8] PCI: pci_host_bridge related cleanup

[Yinghai Lu]

[PATCH 1/8] PCI: Separate host_bridge code out from probe.c
[PATCH 2/8] x86, PCI: have own version for pcibios_bus_to_resource
[PATCH 3/8] x86, PCI: Fix memleak with get_current_resources
[PATCH 4/8] PCI: make pci_host_bridge more robust
[PATCH 5/8] PCI: add generic device into pci_host_bridge struct
[PATCH 6/8] PCI: add host bridge release support
[PATCH 7/8] x86, PCI: break down get_current_resource()
[PATCH 8/8] x86, PCI: add host bridge resource release for using _CRS

will add struct device dev into host_bridge struct.

also will add release support to make sure allocated resource get
freed during root bus removal.

Thanks

Yinghai Lu

[PATCH 1/8] PCI: Separate host_bridge code out from probe.c

[Yinghai Lu]

-v2: Bjorn want add_to_pci_host_bridges to pass struct pci_host_bridge *
      instead of list head pointer.

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

---
 drivers/pci/Makefile      |    2 
 drivers/pci/host-bridge.c |   93 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h         |    2 
 drivers/pci/probe.c       |   81 ----------------------------------------
 4 files changed, 97 insertions(+), 81 deletions(-)

Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -0,0 +1,93 @@
+/*
+ * host_bridge.c - host bridge related code
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/module.h>
+
+#include "pci.h"
+
+static LIST_HEAD(pci_host_bridges);
+
+void add_to_pci_host_bridges(struct pci_host_bridge *bridge)
+{
+	list_add_tail(&bridge->list, &pci_host_bridges);
+}
+
+static struct pci_host_bridge *pci_host_bridge(struct pci_dev *dev)
+{
+	struct pci_bus *bus;
+	struct pci_host_bridge *bridge;
+
+	bus = dev->bus;
+	while (bus->parent)
+		bus = bus->parent;
+
+	list_for_each_entry(bridge, &pci_host_bridges, list) {
+		if (bridge->bus == bus)
+			return bridge;
+	}
+
+	return NULL;
+}
+
+static bool resource_contains(struct resource *res1, struct resource *res2)
+{
+	return res1->start <= res2->start && res1->end >= res2->end;
+}
+
+void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge(dev);
+	struct pci_host_bridge_window *window;
+	resource_size_t offset = 0;
+
+	list_for_each_entry(window, &bridge->windows, list) {
+		if (resource_type(res) != resource_type(window->res))
+			continue;
+
+		if (resource_contains(window->res, res)) {
+			offset = window->offset;
+			break;
+		}
+	}
+
+	region->start = res->start - offset;
+	region->end = res->end - offset;
+}
+EXPORT_SYMBOL(pcibios_resource_to_bus);
+
+static bool region_contains(struct pci_bus_region *region1,
+			    struct pci_bus_region *region2)
+{
+	return region1->start <= region2->start && region1->end >= region2->end;
+}
+
+void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge(dev);
+	struct pci_host_bridge_window *window;
+	struct pci_bus_region bus_region;
+	resource_size_t offset = 0;
+
+	list_for_each_entry(window, &bridge->windows, list) {
+		if (resource_type(res) != resource_type(window->res))
+			continue;
+
+		bus_region.start = window->res->start - window->offset;
+		bus_region.end = window->res->end - window->offset;
+
+		if (region_contains(&bus_region, region)) {
+			offset = window->offset;
+			break;
+		}
+	}
+
+	res->start = region->start + offset;
+	res->end = region->end + offset;
+}
+EXPORT_SYMBOL(pcibios_bus_to_resource);
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -231,6 +231,8 @@ static inline int pci_ari_enabled(struct
 void pci_reassigndev_resource_alignment(struct pci_dev *dev);
 extern void pci_disable_bridge_window(struct pci_dev *dev);
 
+void add_to_pci_host_bridges(struct pci_host_bridge *bridge);
+
 /* Single Root I/O Virtualization */
 struct pci_sriov {
 	int pos;		/* capability position */
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -15,13 +15,10 @@
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
 #define CARDBUS_RESERVE_BUSNR	3
 
-static LIST_HEAD(pci_host_bridges);
-
 /* Ugh.  Need to stop exporting this to modules. */
 LIST_HEAD(pci_root_buses);
 EXPORT_SYMBOL(pci_root_buses);
 
-
 static int find_anything(struct device *dev, void *data)
 {
 	return 1;
@@ -44,82 +41,6 @@ int no_pci_devices(void)
 }
 EXPORT_SYMBOL(no_pci_devices);
 
-static struct pci_host_bridge *pci_host_bridge(struct pci_dev *dev)
-{
-	struct pci_bus *bus;
-	struct pci_host_bridge *bridge;
-
-	bus = dev->bus;
-	while (bus->parent)
-		bus = bus->parent;
-
-	list_for_each_entry(bridge, &pci_host_bridges, list) {
-		if (bridge->bus == bus)
-			return bridge;
-	}
-
-	return NULL;
-}
-
-static bool resource_contains(struct resource *res1, struct resource *res2)
-{
-	return res1->start <= res2->start && res1->end >= res2->end;
-}
-
-void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
-			     struct resource *res)
-{
-	struct pci_host_bridge *bridge = pci_host_bridge(dev);
-	struct pci_host_bridge_window *window;
-	resource_size_t offset = 0;
-
-	list_for_each_entry(window, &bridge->windows, list) {
-		if (resource_type(res) != resource_type(window->res))
-			continue;
-
-		if (resource_contains(window->res, res)) {
-			offset = window->offset;
-			break;
-		}
-	}
-
-	region->start = res->start - offset;
-	region->end = res->end - offset;
-}
-EXPORT_SYMBOL(pcibios_resource_to_bus);
-
-static bool region_contains(struct pci_bus_region *region1,
-			    struct pci_bus_region *region2)
-{
-	return region1->start <= region2->start && region1->end >= region2->end;
-}
-
-void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-			     struct pci_bus_region *region)
-{
-	struct pci_host_bridge *bridge = pci_host_bridge(dev);
-	struct pci_host_bridge_window *window;
-	struct pci_bus_region bus_region;
-	resource_size_t offset = 0;
-
-	list_for_each_entry(window, &bridge->windows, list) {
-		if (resource_type(res) != resource_type(window->res))
-			continue;
-
-		bus_region.start = window->res->start - window->offset;
-		bus_region.end = window->res->end - window->offset;
-
-		if (region_contains(&bus_region, region)) {
-			offset = window->offset;
-			break;
-		}
-	}
-
-	res->start = region->start + offset;
-	res->end = region->end + offset;
-}
-EXPORT_SYMBOL(pcibios_bus_to_resource);
-
 /*
  * PCI Bus Class
  */
@@ -1731,7 +1652,7 @@ struct pci_bus *pci_create_root_bus(stru
 	}
 
 	down_write(&pci_bus_sem);
-	list_add_tail(&bridge->list, &pci_host_bridges);
+	add_to_pci_host_bridges(bridge);
 	list_add_tail(&b->node, &pci_root_buses);
 	up_write(&pci_bus_sem);
 
Index: linux-2.6/drivers/pci/Makefile
===================================================================
--- linux-2.6.orig/drivers/pci/Makefile
+++ linux-2.6/drivers/pci/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the PCI bus specific drivers.
 #
 
-obj-y		+= access.o bus.o probe.o remove.o pci.o \
+obj-y		+= access.o bus.o probe.o host-bridge.o remove.o pci.o \
 			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
 			irq.o vpd.o
 obj-$(CONFIG_PROC_FS) += proc.o

[PATCH 2/8] x86, PCI: have own version for pcibios_bus_to_resource

[Yinghai Lu]

x86 does not need to offset the address. So we can skip that costing offset
searching.

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

---
 arch/x86/pci/i386.c       |   14 ++++++++++++++
 drivers/pci/host-bridge.c |    9 +++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

Index: linux-2.6/arch/x86/pci/i386.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/i386.c
+++ linux-2.6/arch/x86/pci/i386.c
@@ -335,6 +335,20 @@ void __init pcibios_resource_survey(void
  */
 fs_initcall(pcibios_assign_resources);
 
+void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	region->start = res->start;
+	region->end = res->end;
+}
+
+void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	res->start = region->start;
+	res->end = region->end;
+}
+
 static const struct vm_operations_struct pci_mmap_ops = {
 	.access = generic_access_phys,
 };
Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- linux-2.6.orig/drivers/pci/host-bridge.c
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -38,8 +38,9 @@ static bool resource_contains(struct res
 	return res1->start <= res2->start && res1->end >= res2->end;
 }
 
-void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
-			     struct resource *res)
+void __weak pcibios_resource_to_bus(struct pci_dev *dev,
+				    struct pci_bus_region *region,
+				    struct resource *res)
 {
 	struct pci_host_bridge *bridge = pci_host_bridge(dev);
 	struct pci_host_bridge_window *window;
@@ -66,8 +67,8 @@ static bool region_contains(struct pci_b
 	return region1->start <= region2->start && region1->end >= region2->end;
 }
 
-void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-			     struct pci_bus_region *region)
+void __weak pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+				    struct pci_bus_region *region)
 {
 	struct pci_host_bridge *bridge = pci_host_bridge(dev);
 	struct pci_host_bridge_window *window;

[PATCH 3/8] x86, PCI: Fix memleak with get_current_resources

[Yinghai Lu]

in pci_scan_acpi_root, when pci_use_crs is set, get_current_resources is used
to get pci_root_info, and it will allocate name and res array.

later if pci_create_root_bus can not create bus (could be already there...)
it will only free bus res list. but the name and res array is not freed.

let get_current_resource take info pointer instead have local info.

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

---
 arch/x86/pci/acpi.c |   49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/acpi.c
+++ linux-2.6/arch/x86/pci/acpi.c
@@ -305,49 +305,55 @@ static void add_resources(struct pci_roo
 	}
 }
 
+static void free_pci_root_info(struct pci_root_info *info)
+{
+	kfree(info->name);
+	kfree(info->res);
+	memset(info, 0, sizeof(struct pci_root_info));
+}
+
 static void
-get_current_resources(struct acpi_device *device, int busnum,
+get_current_resources(struct pci_root_info *info,
+		      struct acpi_device *device, int busnum,
 		      int domain, struct list_head *resources)
 {
-	struct pci_root_info info;
 	size_t size;
 
-	info.bridge = device;
-	info.res_num = 0;
-	info.resources = resources;
+	info->bridge = device;
+	info->res_num = 0;
+	info->resources = resources;
 	acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_resource,
-				&info);
-	if (!info.res_num)
+				info);
+	if (!info->res_num)
 		return;
 
-	size = sizeof(*info.res) * info.res_num;
-	info.res = kmalloc(size, GFP_KERNEL);
-	if (!info.res)
+	size = sizeof(*info->res) * info->res_num;
+	info->res = kmalloc(size, GFP_KERNEL);
+	if (!info->res)
 		return;
 
-	info.name = kasprintf(GFP_KERNEL, "PCI Bus %04x:%02x", domain, busnum);
-	if (!info.name)
+	info->name = kasprintf(GFP_KERNEL, "PCI Bus %04x:%02x", domain, busnum);
+	if (!info->name)
 		goto name_alloc_fail;
 
-	info.res_num = 0;
+	info->res_num = 0;
 	acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource,
-				&info);
+				info);
 
 	if (pci_use_crs) {
-		add_resources(&info);
+		add_resources(info);
 
 		return;
 	}
 
-	kfree(info.name);
-
 name_alloc_fail:
-	kfree(info.res);
+	free_pci_root_info(info);
 }
 
 struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
 {
 	struct acpi_device *device = root->device;
+	struct pci_root_info info;
 	int domain = root->segment;
 	int busnum = root->secondary.start;
 	LIST_HEAD(resources);
@@ -392,6 +398,7 @@ struct pci_bus * __devinit pci_acpi_scan
 
 	sd->domain = domain;
 	sd->node = node;
+	memset(&info, 0, sizeof(struct pci_root_info));
 	/*
 	 * Maybe the desired pci bus has been already scanned. In such case
 	 * it is unnecessary to scan the pci bus with the given domain,busnum.
@@ -405,7 +412,8 @@ struct pci_bus * __devinit pci_acpi_scan
 		memcpy(bus->sysdata, sd, sizeof(*sd));
 		kfree(sd);
 	} else {
-		get_current_resources(device, busnum, domain, &resources);
+		get_current_resources(&info, device, busnum, domain,
+					&resources);
 
 		/*
 		 * _CRS with no apertures is normal, so only fall back to
@@ -419,6 +427,9 @@ struct pci_bus * __devinit pci_acpi_scan
 			bus->subordinate = pci_scan_child_bus(bus);
 		else
 			pci_free_resource_list(&resources);
+
+		if (!bus && pci_use_crs)
+			free_pci_root_info(&info);
 	}
 
 	/* After the PCI-E bus has been walked and all devices discovered,

[PATCH 4/8] PCI: make pci_host_bridge more robust

[Yinghai Lu]

1. change pci_host_bridge to find_pci_root_bridge.
2. separate find_pci_root_bus().
3. on any possible path return NULL.

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

---
 drivers/pci/host-bridge.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- linux-2.6.orig/drivers/pci/host-bridge.c
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -16,19 +16,31 @@ void add_to_pci_host_bridges(struct pci_
 	list_add_tail(&bridge->list, &pci_host_bridges);
 }
 
-static struct pci_host_bridge *pci_host_bridge(struct pci_dev *dev)
+static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
 {
 	struct pci_bus *bus;
-	struct pci_host_bridge *bridge;
 
 	bus = dev->bus;
 	while (bus->parent)
 		bus = bus->parent;
 
-	list_for_each_entry(bridge, &pci_host_bridges, list) {
+	if (!pci_is_root_bus(bus))
+		return NULL;
+
+	return bus;
+}
+
+static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
+{
+	struct pci_bus *bus = find_pci_root_bus(dev);
+	struct pci_host_bridge *bridge;
+
+	if (!bus)
+		return NULL;
+
+	list_for_each_entry(bridge, &pci_host_bridges, list)
 		if (bridge->bus == bus)
 			return bridge;
-	}
 
 	return NULL;
 }
@@ -42,10 +54,13 @@ void __weak pcibios_resource_to_bus(stru
 				    struct pci_bus_region *region,
 				    struct resource *res)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
+	if (!bridge)
+		goto no_bridge;
+
 	list_for_each_entry(window, &bridge->windows, list) {
 		if (resource_type(res) != resource_type(window->res))
 			continue;
@@ -56,6 +71,7 @@ void __weak pcibios_resource_to_bus(stru
 		}
 	}
 
+no_bridge:
 	region->start = res->start - offset;
 	region->end = res->end - offset;
 }
@@ -70,12 +86,16 @@ static bool region_contains(struct pci_b
 void __weak pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 				    struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
 	struct pci_host_bridge_window *window;
-	struct pci_bus_region bus_region;
 	resource_size_t offset = 0;
 
+	if (!bridge)
+		goto no_bridge;
+
 	list_for_each_entry(window, &bridge->windows, list) {
+		struct pci_bus_region bus_region;
+
 		if (resource_type(res) != resource_type(window->res))
 			continue;
 
@@ -88,6 +108,7 @@ void __weak pcibios_bus_to_resource(stru
 		}
 	}
 
+no_bridge:
 	res->start = region->start + offset;
 	res->end = region->end + offset;
 }

[PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Yinghai Lu]

use that device for pci_root_bus bridge pointer.

With that make code more simple.

Also we can use pci_release_bus_bridge_dev() to release allocated
pci_host_bridge during removing path.

At last, we can use root bus bridge pointer to get host bridge pointer instead
of going over host bridge list, so could kill that host bridge list.

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

---
 drivers/pci/host-bridge.c |   14 ---------
 drivers/pci/pci.h         |    2 -
 drivers/pci/probe.c       |   65 ++++++++++++++++++++++++----------------------
 include/linux/pci.h       |    4 ++
 4 files changed, 38 insertions(+), 47 deletions(-)

Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- linux-2.6.orig/drivers/pci/host-bridge.c
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -9,13 +9,6 @@
 
 #include "pci.h"
 
-static LIST_HEAD(pci_host_bridges);
-
-void add_to_pci_host_bridges(struct pci_host_bridge *bridge)
-{
-	list_add_tail(&bridge->list, &pci_host_bridges);
-}
-
 static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
 {
 	struct pci_bus *bus;
@@ -33,16 +26,11 @@ static struct pci_bus *find_pci_root_bus
 static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
 {
 	struct pci_bus *bus = find_pci_root_bus(dev);
-	struct pci_host_bridge *bridge;
 
 	if (!bus)
 		return NULL;
 
-	list_for_each_entry(bridge, &pci_host_bridges, list)
-		if (bridge->bus == bus)
-			return bridge;
-
-	return NULL;
+	return to_pci_host_bridge(bus->bridge);
 }
 
 static bool resource_contains(struct resource *res1, struct resource *res2)
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -231,8 +231,6 @@ static inline int pci_ari_enabled(struct
 void pci_reassigndev_resource_alignment(struct pci_dev *dev);
 extern void pci_disable_bridge_window(struct pci_dev *dev);
 
-void add_to_pci_host_bridges(struct pci_host_bridge *bridge);
-
 /* Single Root I/O Virtualization */
 struct pci_sriov {
 	int pos;		/* capability position */
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -421,6 +421,19 @@ static struct pci_bus * pci_alloc_bus(vo
 	return b;
 }
 
+static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
+{
+	struct pci_host_bridge *bridge;
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (bridge) {
+		INIT_LIST_HEAD(&bridge->windows);
+		bridge->bus = b;
+	}
+
+	return bridge;
+}
+
 static unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
 	PCI_SPEED_66MHz_PCIX,		/* 1 */
@@ -1121,7 +1134,13 @@ int pci_cfg_space_size(struct pci_dev *d
 
 static void pci_release_bus_bridge_dev(struct device *dev)
 {
-	kfree(dev);
+	struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
+
+	/* TODO: need to free window->res */
+
+	pci_free_resource_list(&bridge->windows);
+
+	kfree(bridge);
 }
 
 struct pci_dev *alloc_pci_dev(void)
@@ -1570,28 +1589,19 @@ struct pci_bus *pci_create_root_bus(stru
 	int error;
 	struct pci_host_bridge *bridge;
 	struct pci_bus *b, *b2;
-	struct device *dev;
 	struct pci_host_bridge_window *window, *n;
 	struct resource *res;
 	resource_size_t offset;
 	char bus_addr[64];
 	char *fmt;
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
-	if (!bridge)
-		return NULL;
 
 	b = pci_alloc_bus();
 	if (!b)
-		goto err_bus;
-
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		goto err_dev;
+		return NULL;
 
 	b->sysdata = sysdata;
 	b->ops = ops;
-
 	b2 = pci_find_bus(pci_domain_nr(b), bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
@@ -1599,13 +1609,17 @@ struct pci_bus *pci_create_root_bus(stru
 		goto err_out;
 	}
 
-	dev->parent = parent;
-	dev->release = pci_release_bus_bridge_dev;
-	dev_set_name(dev, "pci%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(dev);
+	bridge = pci_alloc_host_bridge(b);
+	if (!bridge)
+		goto err_out;
+
+	bridge->dev.parent = parent;
+	bridge->dev.release = pci_release_bus_bridge_dev;
+	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	error = device_register(&bridge->dev);
 	if (error)
-		goto dev_reg_err;
-	b->bridge = get_device(dev);
+		goto bridge_dev_reg_err;
+	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
 	pci_set_bus_of_node(b);
 
@@ -1624,9 +1638,6 @@ struct pci_bus *pci_create_root_bus(stru
 
 	b->number = b->secondary = bus;
 
-	bridge->bus = b;
-	INIT_LIST_HEAD(&bridge->windows);
-
 	if (parent)
 		dev_info(parent, "PCI host bridge to bus %s\n", dev_name(&b->dev));
 	else
@@ -1652,25 +1663,17 @@ struct pci_bus *pci_create_root_bus(stru
 	}
 
 	down_write(&pci_bus_sem);
-	add_to_pci_host_bridges(bridge);
 	list_add_tail(&b->node, &pci_root_buses);
 	up_write(&pci_bus_sem);
 
 	return b;
 
 class_dev_reg_err:
-	device_unregister(dev);
-dev_reg_err:
-	down_write(&pci_bus_sem);
-	list_del(&bridge->list);
-	list_del(&b->node);
-	up_write(&pci_bus_sem);
+	device_unregister(&bridge->dev);
+bridge_dev_reg_err:
+	kfree(bridge);
 err_out:
-	kfree(dev);
-err_dev:
 	kfree(b);
-err_bus:
-	kfree(bridge);
 	return NULL;
 }
 
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -375,11 +375,13 @@ struct pci_host_bridge_window {
 };
 
 struct pci_host_bridge {
-	struct list_head list;
+	struct device dev;
 	struct pci_bus *bus;		/* root bus */
 	struct list_head windows;	/* pci_host_bridge_windows */
 };
 
+#define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
+
 /*
  * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
  * to P2P or CardBus bridge windows) go in a table.  Additional ones (for

[PATCH 6/8] PCI: add host bridge release support

[Yinghai Lu]

We need one hook to release host bridge resource that are created for host
bridge.

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

---
 drivers/pci/host-bridge.c |    8 ++++++++
 drivers/pci/probe.c       |    3 ++-
 include/linux/pci.h       |    5 +++++
 3 files changed, 15 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- linux-2.6.orig/drivers/pci/host-bridge.c
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -33,6 +33,14 @@ static struct pci_host_bridge *find_pci_
 	return to_pci_host_bridge(bus->bridge);
 }
 
+void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
+				 void (*release_fn)(struct pci_host_bridge *),
+				 void *release_data)
+{
+	bridge->release_fn = release_fn;
+	bridge->release_data = release_data;
+}
+
 static bool resource_contains(struct resource *res1, struct resource *res2)
 {
 	return res1->start <= res2->start && res1->end >= res2->end;
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1136,7 +1136,8 @@ static void pci_release_bus_bridge_dev(s
 {
 	struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
 
-	/* TODO: need to free window->res */
+	if (bridge->release_fn)
+		bridge->release_fn(bridge);
 
 	pci_free_resource_list(&bridge->windows);
 
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -378,9 +378,14 @@ struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
 	struct list_head windows;	/* pci_host_bridge_windows */
+	void (*release_fn)(struct pci_host_bridge *);
+	void *release_data;
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
+void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
+		     void (*release_fn)(struct pci_host_bridge *),
+		     void *release_data);
 
 /*
  * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond

[PATCH 7/8] x86, PCI: break down get_current_resource()

[Yinghai Lu]

change it to probe_pci_root_info, aka only do one thing.

1. remove resource list head from pci_root_info
2. make get_current_resources() not pass resources
   move out add_resources calling out of yet.
3. rename get_current_resourcs name to probe_pci_root_info

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

---
 arch/x86/pci/acpi.c |   34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/acpi.c
+++ linux-2.6/arch/x86/pci/acpi.c
@@ -12,7 +12,6 @@ struct pci_root_info {
 	char *name;
 	unsigned int res_num;
 	struct resource *res;
-	struct list_head *resources;
 	int busnum;
 };
 
@@ -277,7 +276,8 @@ static void coalesce_windows(struct pci_
 	}
 }
 
-static void add_resources(struct pci_root_info *info)
+static void add_resources(struct pci_root_info *info,
+			  struct list_head *resources)
 {
 	int i;
 	struct resource *res, *root, *conflict;
@@ -301,7 +301,7 @@ static void add_resources(struct pci_roo
 				 "ignoring host bridge window %pR (conflicts with %s %pR)\n",
 				 res, conflict->name, conflict);
 		else
-			pci_add_resource(info->resources, res);
+			pci_add_resource(resources, res);
 	}
 }
 
@@ -313,41 +313,30 @@ static void free_pci_root_info(struct pc
 }
 
 static void
-get_current_resources(struct pci_root_info *info,
-		      struct acpi_device *device, int busnum,
-		      int domain, struct list_head *resources)
+probe_pci_root_info(struct pci_root_info *info, struct acpi_device *device,
+		    int busnum, int domain)
 {
 	size_t size;
 
 	info->bridge = device;
 	info->res_num = 0;
-	info->resources = resources;
 	acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_resource,
 				info);
 	if (!info->res_num)
 		return;
 
 	size = sizeof(*info->res) * info->res_num;
+	info->res_num = 0;
 	info->res = kmalloc(size, GFP_KERNEL);
 	if (!info->res)
 		return;
 
 	info->name = kasprintf(GFP_KERNEL, "PCI Bus %04x:%02x", domain, busnum);
 	if (!info->name)
-		goto name_alloc_fail;
+		return;
 
-	info->res_num = 0;
 	acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource,
 				info);
-
-	if (pci_use_crs) {
-		add_resources(info);
-
-		return;
-	}
-
-name_alloc_fail:
-	free_pci_root_info(info);
 }
 
 struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
@@ -412,15 +401,18 @@ struct pci_bus * __devinit pci_acpi_scan
 		memcpy(bus->sysdata, sd, sizeof(*sd));
 		kfree(sd);
 	} else {
-		get_current_resources(&info, device, busnum, domain,
-					&resources);
+		probe_pci_root_info(&info, device, busnum, domain);
 
 		/*
 		 * _CRS with no apertures is normal, so only fall back to
 		 * defaults or native bridge info if we're ignoring _CRS.
 		 */
-		if (!pci_use_crs)
+		if (pci_use_crs)
+			add_resources(&info, &resources);
+		else {
+			free_pci_root_info(&info);
 			x86_pci_root_bus_resources(busnum, &resources);
+		}
 		bus = pci_create_root_bus(NULL, busnum, &pci_root_ops, sd,
 					  &resources);
 		if (bus)

[PATCH 8/8] x86, PCI: add host bridge resource release for using _CRS

[Yinghai Lu]

1. allocate pci_root_info instead of use local stack. we need to pass around info
   for release fn.
2. add release_pci_root_info
3. set x86 own host bridge release fn, so will make sure root bridge 
   related resources get freed during root bus removal.

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

---
 arch/x86/pci/acpi.c |   66 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 13 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/acpi.c
+++ linux-2.6/arch/x86/pci/acpi.c
@@ -309,7 +309,33 @@ static void free_pci_root_info(struct pc
 {
 	kfree(info->name);
 	kfree(info->res);
-	memset(info, 0, sizeof(struct pci_root_info));
+	kfree(info);
+}
+
+static void __release_pci_root_info(struct pci_root_info *info)
+{
+	int i;
+	struct resource *res;
+
+	for (i = 0; i < info->res_num; i++) {
+		res = &info->res[i];
+
+		if (!res->parent)
+			continue;
+
+		if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
+			continue;
+
+		release_resource(res);
+	}
+
+	free_pci_root_info(info);
+}
+static void release_pci_root_info(struct pci_host_bridge *bridge)
+{
+	struct pci_root_info *info = bridge->release_data;
+
+	__release_pci_root_info(info);
 }
 
 static void
@@ -342,7 +368,7 @@ probe_pci_root_info(struct pci_root_info
 struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
 {
 	struct acpi_device *device = root->device;
-	struct pci_root_info info;
+	struct pci_root_info *info = NULL;
 	int domain = root->segment;
 	int busnum = root->secondary.start;
 	LIST_HEAD(resources);
@@ -379,15 +405,16 @@ struct pci_bus * __devinit pci_acpi_scan
 	 * It's arguable whether it's worth the trouble to care.
 	 */
 	sd = kzalloc(sizeof(*sd), GFP_KERNEL);
-	if (!sd) {
-		printk(KERN_WARNING "pci_bus %04x:%02x: "
-		       "ignored (out of memory)\n", domain, busnum);
-		return NULL;
-	}
+	if (!sd)
+		goto out_no_memory;
 
 	sd->domain = domain;
 	sd->node = node;
-	memset(&info, 0, sizeof(struct pci_root_info));
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		kfree(sd);
+		goto out_no_memory;
+	}
 	/*
 	 * Maybe the desired pci bus has been already scanned. In such case
 	 * it is unnecessary to scan the pci bus with the given domain,busnum.
@@ -399,20 +426,22 @@ struct pci_bus * __devinit pci_acpi_scan
 		 * be replaced by sd.
 		 */
 		memcpy(bus->sysdata, sd, sizeof(*sd));
+		kfree(info);
 		kfree(sd);
 	} else {
-		probe_pci_root_info(&info, device, busnum, domain);
+		probe_pci_root_info(info, device, busnum, domain);
 
 		/*
 		 * _CRS with no apertures is normal, so only fall back to
 		 * defaults or native bridge info if we're ignoring _CRS.
 		 */
 		if (pci_use_crs)
-			add_resources(&info, &resources);
+			add_resources(info, &resources);
 		else {
-			free_pci_root_info(&info);
+			free_pci_root_info(info);
 			x86_pci_root_bus_resources(busnum, &resources);
 		}
+
 		bus = pci_create_root_bus(NULL, busnum, &pci_root_ops, sd,
 					  &resources);
 		if (bus)
@@ -420,8 +449,14 @@ struct pci_bus * __devinit pci_acpi_scan
 		else
 			pci_free_resource_list(&resources);
 
-		if (!bus && pci_use_crs)
-			free_pci_root_info(&info);
+		if (pci_use_crs) {
+			if (bus)
+				pci_set_host_bridge_release(
+					to_pci_host_bridge(bus->bridge),
+					release_pci_root_info, info);
+			else
+				__release_pci_root_info(info);
+		}
 	}
 
 	/* After the PCI-E bus has been walked and all devices discovered,
@@ -452,6 +487,11 @@ struct pci_bus * __devinit pci_acpi_scan
 	}
 
 	return bus;
+
+out_no_memory:
+	printk(KERN_WARNING "pci_bus %04x:%02x: "
+	       "ignored (out of memory)\n", domain, busnum);
+	return NULL;
 }
 
 int __init pci_acpi_init(void)

Re: [PATCH 2/8] x86, PCI: have own version for pcibios_bus_to_resource

[Bjorn Helgaas]

On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> x86 does not need to offset the address. So we can skip that costing offset
> searching.

I don't understand the point of this.  I'm trying hard to *remove* PCI
code from arch/ because most of it is not really arch-specific.  This
is a good example.  There's nothing in the pcibios_bus_to_resource()
that you're adding back to x86 that is specific to x86, and it makes
things more complicated.  Using __weak adds ambiguity for the reader,
now the pci_add_resource_offset() interface exists but doesn't work,
etc.

I agree that this patch will make x86 a tiny bit faster and a tiny bit
smaller, but I don't think it's enough to matter.  Do you have numbers
that say otherwise?

Bjorn

> ---
>  arch/x86/pci/i386.c       |   14 ++++++++++++++
>  drivers/pci/host-bridge.c |    9 +++++----
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> Index: linux-2.6/arch/x86/pci/i386.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/pci/i386.c
> +++ linux-2.6/arch/x86/pci/i386.c
> @@ -335,6 +335,20 @@ void __init pcibios_resource_survey(void
>  */
>  fs_initcall(pcibios_assign_resources);
>
> +void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
> +                            struct resource *res)
> +{
> +       region->start = res->start;
> +       region->end = res->end;
> +}
> +
> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> +                            struct pci_bus_region *region)
> +{
> +       res->start = region->start;
> +       res->end = region->end;
> +}
> +
>  static const struct vm_operations_struct pci_mmap_ops = {
>        .access = generic_access_phys,
>  };
> Index: linux-2.6/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/host-bridge.c
> +++ linux-2.6/drivers/pci/host-bridge.c
> @@ -38,8 +38,9 @@ static bool resource_contains(struct res
>        return res1->start <= res2->start && res1->end >= res2->end;
>  }
>
> -void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
> -                            struct resource *res)
> +void __weak pcibios_resource_to_bus(struct pci_dev *dev,
> +                                   struct pci_bus_region *region,
> +                                   struct resource *res)
>  {
>        struct pci_host_bridge *bridge = pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;
> @@ -66,8 +67,8 @@ static bool region_contains(struct pci_b
>        return region1->start <= region2->start && region1->end >= region2->end;
>  }
>
> -void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> -                            struct pci_bus_region *region)
> +void __weak pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> +                                   struct pci_bus_region *region)
>  {
>        struct pci_host_bridge *bridge = pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;

Re: [PATCH 4/8] PCI: make pci_host_bridge more robust

[Bjorn Helgaas]

On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> 1. change pci_host_bridge to find_pci_root_bridge.
> 2. separate find_pci_root_bus().
> 3. on any possible path return NULL.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>
> ---
>  drivers/pci/host-bridge.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> Index: linux-2.6/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/host-bridge.c
> +++ linux-2.6/drivers/pci/host-bridge.c
> @@ -16,19 +16,31 @@ void add_to_pci_host_bridges(struct pci_
>        list_add_tail(&bridge->list, &pci_host_bridges);
>  }
>
> -static struct pci_host_bridge *pci_host_bridge(struct pci_dev *dev)
> +static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
>  {
>        struct pci_bus *bus;
> -       struct pci_host_bridge *bridge;
>
>        bus = dev->bus;
>        while (bus->parent)
>                bus = bus->parent;
>
> -       list_for_each_entry(bridge, &pci_host_bridges, list) {
> +       if (!pci_is_root_bus(bus))
> +               return NULL;

pci_is_root_bus() returns "!(pbus->parent)", so the effect of this patch is:

    while (bus->parent)
        bus = bus->parent;

    if (bus->parent)
        return NULL;

The only reason we exited the "while" loop is because "bus->parent ==
NULL", so what's the point of adding the "if" check afterwards?

> +       return bus;
> +}
> +
> +static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
> +{
> +       struct pci_bus *bus = find_pci_root_bus(dev);
> +       struct pci_host_bridge *bridge;
> +
> +       if (!bus)
> +               return NULL;

This should never happen.  If there's a way we can create a pci_dev
that's not under a pci_host_bridge, I think that is a bug, and we
should fix that rather than papering over it here.

I don't think we should apply this patch.

Bjorn

> +       list_for_each_entry(bridge, &pci_host_bridges, list)
>                if (bridge->bus == bus)
>                        return bridge;
> -       }
>
>        return NULL;
>  }
> @@ -42,10 +54,13 @@ void __weak pcibios_resource_to_bus(stru
>                                    struct pci_bus_region *region,
>                                    struct resource *res)
>  {
> -       struct pci_host_bridge *bridge = pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;
>        resource_size_t offset = 0;
>
> +       if (!bridge)
> +               goto no_bridge;
> +
>        list_for_each_entry(window, &bridge->windows, list) {
>                if (resource_type(res) != resource_type(window->res))
>                        continue;
> @@ -56,6 +71,7 @@ void __weak pcibios_resource_to_bus(stru
>                }
>        }
>
> +no_bridge:
>        region->start = res->start - offset;
>        region->end = res->end - offset;
>  }
> @@ -70,12 +86,16 @@ static bool region_contains(struct pci_b
>  void __weak pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>                                    struct pci_bus_region *region)
>  {
> -       struct pci_host_bridge *bridge = pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;
> -       struct pci_bus_region bus_region;
>        resource_size_t offset = 0;
>
> +       if (!bridge)
> +               goto no_bridge;
> +
>        list_for_each_entry(window, &bridge->windows, list) {
> +               struct pci_bus_region bus_region;
> +
>                if (resource_type(res) != resource_type(window->res))
>                        continue;
>
> @@ -88,6 +108,7 @@ void __weak pcibios_bus_to_resource(stru
>                }
>        }
>
> +no_bridge:
>        res->start = region->start + offset;
>        res->end = region->end + offset;
>  }

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Bjorn Helgaas]

On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> use that device for pci_root_bus bridge pointer.
>
> With that make code more simple.
>
> Also we can use pci_release_bus_bridge_dev() to release allocated
> pci_host_bridge during removing path.
>
> At last, we can use root bus bridge pointer to get host bridge pointer instead
> of going over host bridge list, so could kill that host bridge list.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>
> ---
>  drivers/pci/host-bridge.c |   14 ---------
>  drivers/pci/pci.h         |    2 -
>  drivers/pci/probe.c       |   65 ++++++++++++++++++++++++----------------------
>  include/linux/pci.h       |    4 ++
>  4 files changed, 38 insertions(+), 47 deletions(-)
>
> Index: linux-2.6/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/host-bridge.c
> +++ linux-2.6/drivers/pci/host-bridge.c
> @@ -9,13 +9,6 @@
>
>  #include "pci.h"
>
> -static LIST_HEAD(pci_host_bridges);
> -
> -void add_to_pci_host_bridges(struct pci_host_bridge *bridge)
> -{
> -       list_add_tail(&bridge->list, &pci_host_bridges);
> -}
> -
>  static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
>  {
>        struct pci_bus *bus;
> @@ -33,16 +26,11 @@ static struct pci_bus *find_pci_root_bus
>  static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
>  {
>        struct pci_bus *bus = find_pci_root_bus(dev);
> -       struct pci_host_bridge *bridge;
>
>        if (!bus)
>                return NULL;
>
> -       list_for_each_entry(bridge, &pci_host_bridges, list)
> -               if (bridge->bus == bus)
> -                       return bridge;
> -
> -       return NULL;
> +       return to_pci_host_bridge(bus->bridge);
>  }
>
>  static bool resource_contains(struct resource *res1, struct resource *res2)
> Index: linux-2.6/drivers/pci/pci.h
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.h
> +++ linux-2.6/drivers/pci/pci.h
> @@ -231,8 +231,6 @@ static inline int pci_ari_enabled(struct
>  void pci_reassigndev_resource_alignment(struct pci_dev *dev);
>  extern void pci_disable_bridge_window(struct pci_dev *dev);
>
> -void add_to_pci_host_bridges(struct pci_host_bridge *bridge);
> -
>  /* Single Root I/O Virtualization */
>  struct pci_sriov {
>        int pos;                /* capability position */
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c
> +++ linux-2.6/drivers/pci/probe.c
> @@ -421,6 +421,19 @@ static struct pci_bus * pci_alloc_bus(vo
>        return b;
>  }
>
> +static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> +{
> +       struct pci_host_bridge *bridge;
> +
> +       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +       if (bridge) {
> +               INIT_LIST_HEAD(&bridge->windows);
> +               bridge->bus = b;
> +       }
> +
> +       return bridge;
> +}
> +
>  static unsigned char pcix_bus_speed[] = {
>        PCI_SPEED_UNKNOWN,              /* 0 */
>        PCI_SPEED_66MHz_PCIX,           /* 1 */
> @@ -1121,7 +1134,13 @@ int pci_cfg_space_size(struct pci_dev *d
>
>  static void pci_release_bus_bridge_dev(struct device *dev)
>  {
> -       kfree(dev);
> +       struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
> +
> +       /* TODO: need to free window->res */
> +
> +       pci_free_resource_list(&bridge->windows);
> +
> +       kfree(bridge);
>  }
>
>  struct pci_dev *alloc_pci_dev(void)
> @@ -1570,28 +1589,19 @@ struct pci_bus *pci_create_root_bus(stru
>        int error;
>        struct pci_host_bridge *bridge;
>        struct pci_bus *b, *b2;
> -       struct device *dev;
>        struct pci_host_bridge_window *window, *n;
>        struct resource *res;
>        resource_size_t offset;
>        char bus_addr[64];
>        char *fmt;
>
> -       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> -       if (!bridge)
> -               return NULL;
>
>        b = pci_alloc_bus();
>        if (!b)
> -               goto err_bus;
> -
> -       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -       if (!dev)
> -               goto err_dev;
> +               return NULL;
>
>        b->sysdata = sysdata;
>        b->ops = ops;
> -
>        b2 = pci_find_bus(pci_domain_nr(b), bus);
>        if (b2) {
>                /* If we already got to this bus through a different bridge, ignore it */
> @@ -1599,13 +1609,17 @@ struct pci_bus *pci_create_root_bus(stru
>                goto err_out;
>        }
>
> -       dev->parent = parent;
> -       dev->release = pci_release_bus_bridge_dev;
> -       dev_set_name(dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> -       error = device_register(dev);
> +       bridge = pci_alloc_host_bridge(b);
> +       if (!bridge)
> +               goto err_out;
> +
> +       bridge->dev.parent = parent;
> +       bridge->dev.release = pci_release_bus_bridge_dev;
> +       dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +       error = device_register(&bridge->dev);
>        if (error)
> -               goto dev_reg_err;
> -       b->bridge = get_device(dev);
> +               goto bridge_dev_reg_err;
> +       b->bridge = get_device(&bridge->dev);
>        device_enable_async_suspend(b->bridge);
>        pci_set_bus_of_node(b);
>
> @@ -1624,9 +1638,6 @@ struct pci_bus *pci_create_root_bus(stru
>
>        b->number = b->secondary = bus;
>
> -       bridge->bus = b;
> -       INIT_LIST_HEAD(&bridge->windows);
> -
>        if (parent)
>                dev_info(parent, "PCI host bridge to bus %s\n", dev_name(&b->dev));
>        else
> @@ -1652,25 +1663,17 @@ struct pci_bus *pci_create_root_bus(stru
>        }
>
>        down_write(&pci_bus_sem);
> -       add_to_pci_host_bridges(bridge);
>        list_add_tail(&b->node, &pci_root_buses);
>        up_write(&pci_bus_sem);
>
>        return b;
>
>  class_dev_reg_err:
> -       device_unregister(dev);
> -dev_reg_err:
> -       down_write(&pci_bus_sem);
> -       list_del(&bridge->list);
> -       list_del(&b->node);
> -       up_write(&pci_bus_sem);
> +       device_unregister(&bridge->dev);
> +bridge_dev_reg_err:
> +       kfree(bridge);
>  err_out:
> -       kfree(dev);
> -err_dev:
>        kfree(b);
> -err_bus:
> -       kfree(bridge);
>        return NULL;
>  }
>
> Index: linux-2.6/include/linux/pci.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pci.h
> +++ linux-2.6/include/linux/pci.h
> @@ -375,11 +375,13 @@ struct pci_host_bridge_window {
>  };
>
>  struct pci_host_bridge {
> -       struct list_head list;
> +       struct device dev;
>        struct pci_bus *bus;            /* root bus */
>        struct list_head windows;       /* pci_host_bridge_windows */
>  };

This doesn't feel right to me.  You're allocating a new struct device
here, but the arch likely already has one.  In fact, I think we even
passed it in to pci_create_root_bus().

On x86, we already have an ACPI device for the host bridge, and now
we'll have a second one.  (Actually a *third* one, because PNPACPI
also has one, but that's a long-standing problem.)

> +#define        to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
> +
>  /*
>  * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>  * to P2P or CardBus bridge windows) go in a table.  Additional ones (for

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Yinghai Lu]

On Mon, Feb 27, 2012 at 9:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> use that device for pci_root_bus bridge pointer.
>>
>> With that make code more simple.
>>
>> Also we can use pci_release_bus_bridge_dev() to release allocated
>> pci_host_bridge during removing path.
>>
>> At last, we can use root bus bridge pointer to get host bridge pointer instead
>> of going over host bridge list, so could kill that host bridge list.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  drivers/pci/host-bridge.c |   14 ---------
>>  drivers/pci/pci.h         |    2 -
>>  drivers/pci/probe.c       |   65 ++++++++++++++++++++++++----------------------
>>  include/linux/pci.h       |    4 ++
>>  4 files changed, 38 insertions(+), 47 deletions(-)
>>
>> Index: linux-2.6/drivers/pci/host-bridge.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/host-bridge.c
>> +++ linux-2.6/drivers/pci/host-bridge.c
>> -       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> -       if (!dev)
>> -               goto err_dev;
>> +               return NULL;
>>
...

here old code allocate dummy struct device.

>> +       bridge = pci_alloc_host_bridge(b);
>> +       if (!bridge)
>> +               goto err_out;
>> +
>> +       bridge->dev.parent = parent;
>> +       bridge->dev.release = pci_release_bus_bridge_dev;
>> +       dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> +       error = device_register(&bridge->dev);
>>        if (error)
>> -               goto dev_reg_err;
>> -       b->bridge = get_device(dev);
>> +               goto bridge_dev_reg_err;
>> +       b->bridge = get_device(&bridge->dev);
...
>> --- linux-2.6.orig/include/linux/pci.h
>> +++ linux-2.6/include/linux/pci.h
>> @@ -375,11 +375,13 @@ struct pci_host_bridge_window {
>>  };
>>
>>  struct pci_host_bridge {
>> -       struct list_head list;
>> +       struct device dev;
>>        struct pci_bus *bus;            /* root bus */
>>        struct list_head windows;       /* pci_host_bridge_windows */
>>  };
>
> This doesn't feel right to me.  You're allocating a new struct device
> here, but the arch likely already has one.  In fact, I think we even
> passed it in to pci_create_root_bus().

no.

original pci_create_root_bus() will create one dummy struct device and
use it as parent for bus->dev.

>
> On x86, we already have an ACPI device for the host bridge, and now
> we'll have a second one.  (Actually a *third* one, because PNPACPI
> also has one, but that's a long-standing problem.)

not related.

      Yinghai

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Bjorn Helgaas]

On Mon, Feb 27, 2012 at 12:08 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Feb 27, 2012 at 9:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> use that device for pci_root_bus bridge pointer.
>>>
>>> With that make code more simple.
>>>
>>> Also we can use pci_release_bus_bridge_dev() to release allocated
>>> pci_host_bridge during removing path.
>>>
>>> At last, we can use root bus bridge pointer to get host bridge pointer instead
>>> of going over host bridge list, so could kill that host bridge list.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>
>>> ---
>>>  drivers/pci/host-bridge.c |   14 ---------
>>>  drivers/pci/pci.h         |    2 -
>>>  drivers/pci/probe.c       |   65 ++++++++++++++++++++++++----------------------
>>>  include/linux/pci.h       |    4 ++
>>>  4 files changed, 38 insertions(+), 47 deletions(-)
>>>
>>> Index: linux-2.6/drivers/pci/host-bridge.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/pci/host-bridge.c
>>> +++ linux-2.6/drivers/pci/host-bridge.c
>>> -       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>> -       if (!dev)
>>> -               goto err_dev;
>>> +               return NULL;
>>>
> ...
>
> here old code allocate dummy struct device.
>
>>> +       bridge = pci_alloc_host_bridge(b);
>>> +       if (!bridge)
>>> +               goto err_out;
>>> +
>>> +       bridge->dev.parent = parent;
>>> +       bridge->dev.release = pci_release_bus_bridge_dev;
>>> +       dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>>> +       error = device_register(&bridge->dev);
>>>        if (error)
>>> -               goto dev_reg_err;
>>> -       b->bridge = get_device(dev);
>>> +               goto bridge_dev_reg_err;
>>> +       b->bridge = get_device(&bridge->dev);
> ...
>>> --- linux-2.6.orig/include/linux/pci.h
>>> +++ linux-2.6/include/linux/pci.h
>>> @@ -375,11 +375,13 @@ struct pci_host_bridge_window {
>>>  };
>>>
>>>  struct pci_host_bridge {
>>> -       struct list_head list;
>>> +       struct device dev;
>>>        struct pci_bus *bus;            /* root bus */
>>>        struct list_head windows;       /* pci_host_bridge_windows */
>>>  };
>>
>> This doesn't feel right to me.  You're allocating a new struct device
>> here, but the arch likely already has one.  In fact, I think we even
>> passed it in to pci_create_root_bus().
>
> no.
>
> original pci_create_root_bus() will create one dummy struct device and
> use it as parent for bus->dev.
>
>>
>> On x86, we already have an ACPI device for the host bridge, and now
>> we'll have a second one.  (Actually a *third* one, because PNPACPI
>> also has one, but that's a long-standing problem.)
>
> not related.

OK.  Can you educate me?  What is the device we pass in, what does the
dummy one correspond to, why do we have two, what's the difference,
etc., etc.?

Bjorn

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Yinghai Lu]

On Mon, Feb 27, 2012 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Feb 27, 2012 at 12:08 PM, Yinghai Lu <yinghai@kernel.org> wrote:

>>> This doesn't feel right to me.  You're allocating a new struct device
>>> here, but the arch likely already has one.  In fact, I think we even
>>> passed it in to pci_create_root_bus().
>>
>> no.
>>
>> original pci_create_root_bus() will create one dummy struct device and
>> use it as parent for bus->dev.
>>
>>>
>>> On x86, we already have an ACPI device for the host bridge, and now
>>> we'll have a second one.  (Actually a *third* one, because PNPACPI
>>> also has one, but that's a long-standing problem.)
>>
>> not related.
>
> OK.  Can you educate me?  What is the device we pass in, what does the
> dummy one correspond to, why do we have two, what's the difference,
> etc., etc.?

for every bus will have ->bridge, and that is device pointer. to  pci
bridge dev.

for root bus, there is not pci bridge for that. So create_root_bus
allocate local dummy one.

that dummy device will be bus->dev's parent.

Now pci_host_bridge is added, and it is allocated, so just put the
struct device into that pci_host_bridge.

and use pci_host_bridge->dev as parent of root bus->dev's parent.

        Yinghai

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Bjorn Helgaas]

On Mon, Feb 27, 2012 at 1:49 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Feb 27, 2012 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Feb 27, 2012 at 12:08 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
>>>> This doesn't feel right to me.  You're allocating a new struct device
>>>> here, but the arch likely already has one.  In fact, I think we even
>>>> passed it in to pci_create_root_bus().
>>>
>>> no.
>>>
>>> original pci_create_root_bus() will create one dummy struct device and
>>> use it as parent for bus->dev.
>>>
>>>>
>>>> On x86, we already have an ACPI device for the host bridge, and now
>>>> we'll have a second one.  (Actually a *third* one, because PNPACPI
>>>> also has one, but that's a long-standing problem.)
>>>
>>> not related.
>>
>> OK.  Can you educate me?  What is the device we pass in, what does the
>> dummy one correspond to, why do we have two, what's the difference,
>> etc., etc.?
>
> for every bus will have ->bridge, and that is device pointer. to  pci
> bridge dev.
>
> for root bus, there is not pci bridge for that. So create_root_bus
> allocate local dummy one.
>
> that dummy device will be bus->dev's parent.
>
> Now pci_host_bridge is added, and it is allocated, so just put the
> struct device into that pci_host_bridge.
>
> and use pci_host_bridge->dev as parent of root bus->dev's parent.

What's the device passed in to pci_create_root_bus()?  Why is that
different than the root bus dev's parent?

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Yinghai Lu]

On Mon, Feb 27, 2012 at 2:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> for every bus will have ->bridge, and that is device pointer. to  pci
>> bridge dev.
>>
>> for root bus, there is not pci bridge for that. So create_root_bus
>> allocate local dummy one.
>>
>> that dummy device will be bus->dev's parent.
>>
>> Now pci_host_bridge is added, and it is allocated, so just put the
>> struct device into that pci_host_bridge.
>>
>> and use pci_host_bridge->dev as parent of root bus->dev's parent.
>
> What's the device passed in to pci_create_root_bus()?  Why is that
> different than the root bus dev's parent?

ia64 and x86 doesn't pass that.

arch/ia64/pci/pci.c:    pbus = pci_create_root_bus(NULL, bus,
&pci_root_ops, controller,
arch/powerpc/kernel/pci-common.c:       bus =
pci_create_root_bus(hose->parent, hose->first_busno,
arch/sparc/kernel/pci.c:        bus = pci_create_root_bus(parent,
pbm->pci_first_busno, pbm->pci_ops,
arch/x86/pci/acpi.c:            bus = pci_create_root_bus(NULL,
busnum, &pci_root_ops, sd,
drivers/parisc/dino.c:  dino_dev->hba.hba_bus = bus =
pci_create_root_bus(&dev->dev,
drivers/parisc/lba_pci.c:               pci_create_root_bus(&dev->dev,
lba_dev->hba.bus_num.start,

sparc, powerpc, parisc have their local ...

so local hose etc's dev ---> allocated dev ---> root bus's dev

this patch change to

local hose etc's dev ---> allocated host bridge's dev ---> root bus's dev

Re: [PATCH 5/8] PCI: add generic device into pci_host_bridge struct

[Bjorn Helgaas]

On Mon, Feb 27, 2012 at 3:55 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Feb 27, 2012 at 2:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> for every bus will have ->bridge, and that is device pointer. to  pci
>>> bridge dev.
>>>
>>> for root bus, there is not pci bridge for that. So create_root_bus
>>> allocate local dummy one.
>>>
>>> that dummy device will be bus->dev's parent.
>>>
>>> Now pci_host_bridge is added, and it is allocated, so just put the
>>> struct device into that pci_host_bridge.
>>>
>>> and use pci_host_bridge->dev as parent of root bus->dev's parent.
>>
>> What's the device passed in to pci_create_root_bus()?  Why is that
>> different than the root bus dev's parent?
>
> ia64 and x86 doesn't pass that.
>
> arch/ia64/pci/pci.c:    pbus = pci_create_root_bus(NULL, bus,
> &pci_root_ops, controller,
> arch/powerpc/kernel/pci-common.c:       bus =
> pci_create_root_bus(hose->parent, hose->first_busno,
> arch/sparc/kernel/pci.c:        bus = pci_create_root_bus(parent,
> pbm->pci_first_busno, pbm->pci_ops,
> arch/x86/pci/acpi.c:            bus = pci_create_root_bus(NULL,
> busnum, &pci_root_ops, sd,
> drivers/parisc/dino.c:  dino_dev->hba.hba_bus = bus =
> pci_create_root_bus(&dev->dev,
> drivers/parisc/lba_pci.c:               pci_create_root_bus(&dev->dev,
> lba_dev->hba.bus_num.start,
>
> sparc, powerpc, parisc have their local ...
>
> so local hose etc's dev ---> allocated dev ---> root bus's dev
>
> this patch change to
>
> local hose etc's dev ---> allocated host bridge's dev ---> root bus's dev

Why do we have a separate dev for the hose and the root bus's dev?
The hose *is* a PCI host bridge.