[PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Thierry Reding]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

This patch makes the existing pci_host_bridge structure a proper device
that is usable by PCI host drivers in a more standard way. In addition
to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
and pci_create_root_bus interfaces, this unfortunately means having to
add yet another interface doing basically the same thing, and add some
extra code in the initial step.

However, this time it's more likely to be extensible enough that we
won't have to do another one again in the future, and we should be
able to reduce code much more as a result.

The main idea is to pull the allocation of 'struct pci_host_bridge' out
of the registration, and let individual host drivers and architecture
code fill the members before calling the registration function.

There are a number of things we can do based on this:

* Use a single memory allocation for the driver-specific structure
  and the generic PCI host bridge
* consolidate the contents of driver specific structures by moving
  them into pci_host_bridge
* Add a consistent interface for removing a PCI host bridge again
  when unloading a host driver module
* Replace the architecture specific __weak pcibios_* functions with
  callbacks in a pci_host_bridge device
* Move common boilerplate code from host drivers into the generic
  function, based on contents of the structure
* Extend pci_host_bridge with additional members when needed without
  having to add arguments to pci_scan_*.
* Move members of struct pci_bus into pci_host_bridge to avoid
  having lots of identical copies.

As mentioned in a previous email, one open question is whether we want
to export a function for allocating a pci_host_bridge device in
combination with the per-device structure or let the driver itself
call kzalloc.

Changes in v2 (Thierry Reding):
- add a pci_host_bridge_init() helper that drivers can use to perform
  all the necessary steps to initialize the bridge
- rename pci_register_host() to pci_host_bridge_register() to reflect
  the naming used by other functions
- plug memory leak on registration failure

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/probe.c | 111 ++++++++++++++++++++++++++++++++--------------------
 include/linux/pci.h |   8 +++-
 2 files changed, 75 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 77f0bbb05c3f..ba40514acd32 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,19 +521,6 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-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)
-		return NULL;
-
-	INIT_LIST_HEAD(&bridge->windows);
-	bridge->bus = b;
-	return bridge;
-}
-
 static const unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
 	PCI_SPEED_66MHz_PCIX,		/* 1 */
@@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+void pci_host_bridge_init(struct pci_host_bridge *bridge)
+{
+	INIT_LIST_HEAD(&bridge->windows);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_init);
+
+int pci_host_bridge_register(struct pci_host_bridge *bridge)
 {
 	int error;
-	struct pci_host_bridge *bridge;
 	struct pci_bus *b, *b2;
 	struct resource_entry *window, *n;
+	LIST_HEAD(resources);
 	struct resource *res;
 	resource_size_t offset;
 	char bus_addr[64];
 	char *fmt;
+	struct device *parent = bridge->dev.parent;
 
 	b = pci_alloc_bus(NULL);
 	if (!b)
-		return NULL;
+		return -ENOMEM;
+	bridge->bus = b;
 
-	b->sysdata = sysdata;
-	b->ops = ops;
-	b->number = b->busn_res.start = bus;
+	/* temporarily move resources off the list */
+	list_splice_init(&bridge->windows, &resources);
+	b->sysdata = bridge->sysdata;
+	b->msi = bridge->msi;
+	b->ops = bridge->ops;
+	b->number = b->busn_res.start = bridge->busnr;
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	b->domain_nr = pci_bus_find_domain_nr(b, parent);
 #endif
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
+	b2 = pci_find_bus(pci_domain_nr(b), bridge->busnr);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
+		error = -EEXIST;
 		goto err_out;
 	}
 
-	bridge = pci_alloc_host_bridge(b);
-	if (!bridge)
-		goto err_out;
-
-	bridge->dev.parent = parent;
-	bridge->dev.release = pci_release_host_bridge_dev;
-	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b),
+		     bridge->busnr);
 	error = pcibios_root_bridge_prepare(bridge);
-	if (error) {
-		kfree(bridge);
+	if (error)
 		goto err_out;
-	}
 
 	error = device_register(&bridge->dev);
-	if (error) {
+	if (error)
 		put_device(&bridge->dev);
-		goto err_out;
-	}
+
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
 	pci_set_bus_of_node(b);
@@ -2174,7 +2164,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
-	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bridge->busnr);
 	error = device_register(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
@@ -2190,12 +2180,12 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 		printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
 
 	/* Add initial resources to the bus */
-	resource_list_for_each_entry_safe(window, n, resources) {
+	resource_list_for_each_entry_safe(window, n, &resources) {
 		list_move_tail(&window->node, &bridge->windows);
 		res = window->res;
 		offset = window->offset;
 		if (res->flags & IORESOURCE_BUS)
-			pci_bus_insert_busn_res(b, bus, res->end);
+			pci_bus_insert_busn_res(b, bridge->busnr, res->end);
 		else
 			pci_bus_add_resource(b, res, 0);
 		if (offset) {
@@ -2215,16 +2205,16 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	list_add_tail(&b->node, &pci_root_buses);
 	up_write(&pci_bus_sem);
 
-	return b;
+	return 0;
 
 class_dev_reg_err:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
 err_out:
 	kfree(b);
-	return NULL;
+	return error;
 }
-EXPORT_SYMBOL_GPL(pci_create_root_bus);
+EXPORT_SYMBOL_GPL(pci_host_bridge_register);
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
@@ -2289,6 +2279,43 @@ void pci_bus_release_busn_res(struct pci_bus *b)
 			res, ret ? "can not be" : "is");
 }
 
+static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
+		int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources, struct msi_controller *msi)
+{
+	struct pci_host_bridge *bridge;
+	int ret;
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return NULL;
+
+	pci_host_bridge_init(bridge);
+	bridge->dev.parent = parent;
+	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->busnr = bus;
+	bridge->ops = ops;
+	bridge->sysdata = sysdata;
+	bridge->msi = msi;
+	list_splice_init(resources, &bridge->windows);
+
+	ret = pci_host_bridge_register(bridge);
+	if (ret) {
+		kfree(bridge);
+		return NULL;
+	}
+
+	return bridge->bus;
+}
+
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return pci_create_root_bus_msi(parent, bus, ops, sysdata, resources,
+				       NULL);
+}
+EXPORT_SYMBOL_GPL(pci_create_root_bus);
+
 struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
 		struct pci_ops *ops, void *sysdata,
 		struct list_head *resources, struct msi_controller *msi)
@@ -2304,12 +2331,10 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
 			break;
 		}
 
-	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
+	b = pci_create_root_bus_msi(parent, bus, ops, sysdata, resources, msi);
 	if (!b)
 		return NULL;
 
-	b->msi = msi;
-
 	if (!found) {
 		dev_info(&b->dev,
 		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8badb664be00..5bf04e20c1cd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -405,10 +405,14 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
 
 struct pci_host_bridge {
 	struct device dev;
-	struct pci_bus *bus;		/* root bus */
+	struct pci_ops *ops;
+	void *sysdata;
+	int busnr;
+	struct pci_bus *bus;		/* points to root */
 	struct list_head windows;	/* resource_entry */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
+	struct msi_controller *msi;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
@@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);
+void pci_host_bridge_init(struct pci_host_bridge *bridge);
+int pci_host_bridge_register(struct pci_host_bridge *bridge);
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
-- 
2.8.3

[PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Thierry Reding]

From: Arnd Bergmann <arnd@arndb.de>

This patch makes the existing pci_host_bridge structure a proper device
that is usable by PCI host drivers in a more standard way. In addition
to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
and pci_create_root_bus interfaces, this unfortunately means having to
add yet another interface doing basically the same thing, and add some
extra code in the initial step.

However, this time it's more likely to be extensible enough that we
won't have to do another one again in the future, and we should be
able to reduce code much more as a result.

The main idea is to pull the allocation of 'struct pci_host_bridge' out
of the registration, and let individual host drivers and architecture
code fill the members before calling the registration function.

There are a number of things we can do based on this:

* Use a single memory allocation for the driver-specific structure
  and the generic PCI host bridge
* consolidate the contents of driver specific structures by moving
  them into pci_host_bridge
* Add a consistent interface for removing a PCI host bridge again
  when unloading a host driver module
* Replace the architecture specific __weak pcibios_* functions with
  callbacks in a pci_host_bridge device
* Move common boilerplate code from host drivers into the generic
  function, based on contents of the structure
* Extend pci_host_bridge with additional members when needed without
  having to add arguments to pci_scan_*.
* Move members of struct pci_bus into pci_host_bridge to avoid
  having lots of identical copies.

As mentioned in a previous email, one open question is whether we want
to export a function for allocating a pci_host_bridge device in
combination with the per-device structure or let the driver itself
call kzalloc.

Changes in v2 (Thierry Reding):
- add a pci_host_bridge_init() helper that drivers can use to perform
  all the necessary steps to initialize the bridge
- rename pci_register_host() to pci_host_bridge_register() to reflect
  the naming used by other functions
- plug memory leak on registration failure

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/probe.c | 111 ++++++++++++++++++++++++++++++++--------------------
 include/linux/pci.h |   8 +++-
 2 files changed, 75 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 77f0bbb05c3f..ba40514acd32 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,19 +521,6 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-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)
-		return NULL;
-
-	INIT_LIST_HEAD(&bridge->windows);
-	bridge->bus = b;
-	return bridge;
-}
-
 static const unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
 	PCI_SPEED_66MHz_PCIX,		/* 1 */
@@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+void pci_host_bridge_init(struct pci_host_bridge *bridge)
+{
+	INIT_LIST_HEAD(&bridge->windows);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_init);
+
+int pci_host_bridge_register(struct pci_host_bridge *bridge)
 {
 	int error;
-	struct pci_host_bridge *bridge;
 	struct pci_bus *b, *b2;
 	struct resource_entry *window, *n;
+	LIST_HEAD(resources);
 	struct resource *res;
 	resource_size_t offset;
 	char bus_addr[64];
 	char *fmt;
+	struct device *parent = bridge->dev.parent;
 
 	b = pci_alloc_bus(NULL);
 	if (!b)
-		return NULL;
+		return -ENOMEM;
+	bridge->bus = b;
 
-	b->sysdata = sysdata;
-	b->ops = ops;
-	b->number = b->busn_res.start = bus;
+	/* temporarily move resources off the list */
+	list_splice_init(&bridge->windows, &resources);
+	b->sysdata = bridge->sysdata;
+	b->msi = bridge->msi;
+	b->ops = bridge->ops;
+	b->number = b->busn_res.start = bridge->busnr;
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	b->domain_nr = pci_bus_find_domain_nr(b, parent);
 #endif
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
+	b2 = pci_find_bus(pci_domain_nr(b), bridge->busnr);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
+		error = -EEXIST;
 		goto err_out;
 	}
 
-	bridge = pci_alloc_host_bridge(b);
-	if (!bridge)
-		goto err_out;
-
-	bridge->dev.parent = parent;
-	bridge->dev.release = pci_release_host_bridge_dev;
-	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b),
+		     bridge->busnr);
 	error = pcibios_root_bridge_prepare(bridge);
-	if (error) {
-		kfree(bridge);
+	if (error)
 		goto err_out;
-	}
 
 	error = device_register(&bridge->dev);
-	if (error) {
+	if (error)
 		put_device(&bridge->dev);
-		goto err_out;
-	}
+
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
 	pci_set_bus_of_node(b);
@@ -2174,7 +2164,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
-	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bridge->busnr);
 	error = device_register(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
@@ -2190,12 +2180,12 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 		printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
 
 	/* Add initial resources to the bus */
-	resource_list_for_each_entry_safe(window, n, resources) {
+	resource_list_for_each_entry_safe(window, n, &resources) {
 		list_move_tail(&window->node, &bridge->windows);
 		res = window->res;
 		offset = window->offset;
 		if (res->flags & IORESOURCE_BUS)
-			pci_bus_insert_busn_res(b, bus, res->end);
+			pci_bus_insert_busn_res(b, bridge->busnr, res->end);
 		else
 			pci_bus_add_resource(b, res, 0);
 		if (offset) {
@@ -2215,16 +2205,16 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	list_add_tail(&b->node, &pci_root_buses);
 	up_write(&pci_bus_sem);
 
-	return b;
+	return 0;
 
 class_dev_reg_err:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
 err_out:
 	kfree(b);
-	return NULL;
+	return error;
 }
-EXPORT_SYMBOL_GPL(pci_create_root_bus);
+EXPORT_SYMBOL_GPL(pci_host_bridge_register);
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
@@ -2289,6 +2279,43 @@ void pci_bus_release_busn_res(struct pci_bus *b)
 			res, ret ? "can not be" : "is");
 }
 
+static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
+		int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources, struct msi_controller *msi)
+{
+	struct pci_host_bridge *bridge;
+	int ret;
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return NULL;
+
+	pci_host_bridge_init(bridge);
+	bridge->dev.parent = parent;
+	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->busnr = bus;
+	bridge->ops = ops;
+	bridge->sysdata = sysdata;
+	bridge->msi = msi;
+	list_splice_init(resources, &bridge->windows);
+
+	ret = pci_host_bridge_register(bridge);
+	if (ret) {
+		kfree(bridge);
+		return NULL;
+	}
+
+	return bridge->bus;
+}
+
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return pci_create_root_bus_msi(parent, bus, ops, sysdata, resources,
+				       NULL);
+}
+EXPORT_SYMBOL_GPL(pci_create_root_bus);
+
 struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
 		struct pci_ops *ops, void *sysdata,
 		struct list_head *resources, struct msi_controller *msi)
@@ -2304,12 +2331,10 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
 			break;
 		}
 
-	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
+	b = pci_create_root_bus_msi(parent, bus, ops, sysdata, resources, msi);
 	if (!b)
 		return NULL;
 
-	b->msi = msi;
-
 	if (!found) {
 		dev_info(&b->dev,
 		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8badb664be00..5bf04e20c1cd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -405,10 +405,14 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
 
 struct pci_host_bridge {
 	struct device dev;
-	struct pci_bus *bus;		/* root bus */
+	struct pci_ops *ops;
+	void *sysdata;
+	int busnr;
+	struct pci_bus *bus;		/* points to root */
 	struct list_head windows;	/* resource_entry */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
+	struct msi_controller *msi;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
@@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);
+void pci_host_bridge_init(struct pci_host_bridge *bridge);
+int pci_host_bridge_register(struct pci_host_bridge *bridge);
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
-- 
2.8.3

[PATCH v2 2/2] PCI: tegra: Use new pci_register_host() interface

[Thierry Reding]

From: Arnd Bergmann <arnd@arndb.de>

Tegra is one of the remaining platforms that still use the traditional
pci_common_init_dev() interface for probing PCI host bridges.

This demonstrates how to convert it to the pci_register_host interface
I just added in a previous patch. This leads to a more linear probe
sequence that can handle errors better because we avoid callbacks into
the driver, and it makes the driver architecture independent.

As a side note, I should mention that I noticed this driver does not
register any IORESOURCE_IO resource with the bus, but instead registers
the I/O port window as a memory resource, which is surely a bug.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 94 +++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c1c7653264f8..121e04f21282 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -258,6 +258,7 @@ static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
 }
 
 struct tegra_pcie {
+	struct pci_host_bridge bridge;
 	struct device *dev;
 
 	void __iomem *pads;
@@ -302,6 +303,11 @@ struct tegra_pcie {
 	struct dentry *debugfs;
 };
 
+static inline struct tegra_pcie *to_tegra_pcie(struct pci_host_bridge *host)
+{
+	return container_of(host, struct tegra_pcie, bridge);
+}
+
 struct tegra_pcie_port {
 	struct tegra_pcie *pcie;
 	struct device_node *np;
@@ -320,11 +326,6 @@ struct tegra_pcie_bus {
 	unsigned int nr;
 };
 
-static inline struct tegra_pcie *sys_to_pcie(struct pci_sys_data *sys)
-{
-	return sys->private_data;
-}
-
 static inline void afi_writel(struct tegra_pcie *pcie, u32 value,
 			      unsigned long offset)
 {
@@ -428,7 +429,8 @@ free:
 
 static int tegra_pcie_add_bus(struct pci_bus *bus)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct tegra_pcie *pcie = to_tegra_pcie(host);
 	struct tegra_pcie_bus *b;
 
 	b = tegra_pcie_bus_alloc(pcie, bus->number);
@@ -442,7 +444,8 @@ static int tegra_pcie_add_bus(struct pci_bus *bus)
 
 static void tegra_pcie_remove_bus(struct pci_bus *child)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(child->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(child);
+	struct tegra_pcie *pcie = to_tegra_pcie(host);
 	struct tegra_pcie_bus *bus, *tmp;
 
 	list_for_each_entry_safe(bus, tmp, &pcie->buses, list) {
@@ -459,7 +462,8 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 					unsigned int devfn,
 					int where)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct tegra_pcie *pcie = to_tegra_pcie(host);
 	void __iomem *addr = NULL;
 
 	if (bus->number == 0) {
@@ -607,36 +611,33 @@ static void tegra_pcie_relax_enable(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 
-static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
+static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(sys);
+	struct list_head *windows = &pcie->bridge.windows;
 	int err;
 
-	sys->mem_offset = pcie->offset.mem;
-	sys->io_offset = pcie->offset.io;
-
 	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->io);
 	if (err < 0)
 		return err;
 
 	pci_ioremap_io(pcie->pio.start, pcie->io.start);
 
-	pci_add_resource_offset(&sys->resources, &pcie->pio, sys->io_offset);
-	pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
-	pci_add_resource_offset(&sys->resources, &pcie->prefetch,
-				sys->mem_offset);
-	pci_add_resource(&sys->resources, &pcie->busn);
+	pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io);
+	pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem);
+	pci_add_resource_offset(windows, &pcie->prefetch, pcie->offset.mem);
+	pci_add_resource(windows, &pcie->busn);
 
-	err = devm_request_pci_bus_resources(pcie->dev, &sys->resources);
+	err = devm_request_pci_bus_resources(pcie->dev, windows);
 	if (err < 0)
 		return err;
 
-	return 1;
+	return 0;
 }
 
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
+	struct tegra_pcie *pcie = to_tegra_pcie(host);
 	int irq;
 
 	tegra_cpuidle_pcie_irqs_in_use();
@@ -1521,6 +1522,7 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	}
 
 	/* setup AFI/FPCI range */
+	pcie->bridge.msi = &msi->chip;
 	msi->pages = __get_free_pages(GFP_KERNEL, 0);
 	base = virt_to_phys((void *)msi->pages);
 
@@ -2006,10 +2008,9 @@ retry:
 	return false;
 }
 
-static int tegra_pcie_enable(struct tegra_pcie *pcie)
+static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 {
 	struct tegra_pcie_port *port, *tmp;
-	struct hw_pci hw;
 
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
 		dev_info(pcie->dev, "probing port %u, using %u lanes\n",
@@ -2025,22 +2026,6 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
 		tegra_pcie_port_disable(port);
 		tegra_pcie_port_free(port);
 	}
-
-	memset(&hw, 0, sizeof(hw));
-
-#ifdef CONFIG_PCI_MSI
-	hw.msi_ctrl = &pcie->msi.chip;
-#endif
-
-	hw.nr_controllers = 1;
-	hw.private_data = (void **)&pcie;
-	hw.setup = tegra_pcie_setup;
-	hw.map_irq = tegra_pcie_map_irq;
-	hw.ops = &tegra_pcie_ops;
-
-	pci_common_init_dev(pcie->dev, &hw);
-
-	return 0;
 }
 
 static const struct tegra_pcie_soc_data tegra20_pcie_data = {
@@ -2203,7 +2188,9 @@ remove:
 static int tegra_pcie_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
+	struct pci_host_bridge *bridge;
 	struct tegra_pcie *pcie;
+	struct pci_bus *child;
 	int err;
 
 	match = of_match_device(tegra_pcie_of_match, &pdev->dev);
@@ -2219,6 +2206,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	pcie->soc_data = match->data;
 	pcie->dev = &pdev->dev;
 
+	pci_host_bridge_init(&pcie->bridge);
+	bridge = &pcie->bridge;
+
 	err = tegra_pcie_parse_dt(pcie);
 	if (err < 0)
 		return err;
@@ -2235,6 +2225,10 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
+	err = tegra_pcie_request_resources(pcie);
+	if (err)
+		goto put_resources;
+
 	/* setup the AFI address translations */
 	tegra_pcie_setup_translations(pcie);
 
@@ -2248,12 +2242,30 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	err = tegra_pcie_enable(pcie);
+	tegra_pcie_enable_ports(pcie);
+
+	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
+	bridge->busnr = pcie->busn.start;
+	bridge->dev.parent = &pdev->dev;
+	bridge->ops = &tegra_pcie_ops;
+
+	err = pci_host_bridge_register(bridge);
 	if (err < 0) {
-		dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n", err);
+		dev_err(&pdev->dev, "failed to register host: %d\n", err);
 		goto disable_msi;
 	}
 
+	pci_scan_child_bus(bridge->bus);
+
+	pci_fixup_irqs(pci_common_swizzle, tegra_pcie_map_irq);
+	pci_bus_size_bridges(bridge->bus);
+	pci_bus_assign_resources(bridge->bus);
+
+	list_for_each_entry(child, &bridge->bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	pci_bus_add_devices(bridge->bus);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_pcie_debugfs_init(pcie);
 		if (err < 0)
-- 
2.8.3

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Thursday, June 30, 2016 5:19:30 PM CEST Thierry Reding wrote:
> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.

Thanks a lot for following up on my patches and getting them to work!

> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.
> 
> Changes in v2 (Thierry Reding):
> - add a pci_host_bridge_init() helper that drivers can use to perform
>   all the necessary steps to initialize the bridge

For these two points, the addition of pci_host_bridge_init() to me
tips the balance in favour of having a separate allocation function
that also does the initialization, something like

struct struct pci_host_bridge *pci_host_bridge_alloc(size_t priv)
{
	struct pci_host_bridge *bridge;

	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
	if (!bridge)
		return NULL;

	INIT_LIST_HEAD(&bridge->windows);

	if (priv)
		bridge->private = &bridge[1];

	return bridge;
}

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Thursday, June 30, 2016 5:19:30 PM CEST Thierry Reding wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.

Thanks a lot for following up on my patches and getting them to work!

> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.
> 
> Changes in v2 (Thierry Reding):
> - add a pci_host_bridge_init() helper that drivers can use to perform
>   all the necessary steps to initialize the bridge

For these two points, the addition of pci_host_bridge_init() to me
tips the balance in favour of having a separate allocation function
that also does the initialization, something like

struct struct pci_host_bridge *pci_host_bridge_alloc(size_t priv)
{
	struct pci_host_bridge *bridge;

	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
	if (!bridge)
		return NULL;

	INIT_LIST_HEAD(&bridge->windows);

	if (priv)
		bridge->private = &bridge[1];

	return bridge;
}

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Thu, Jun 30, 2016 at 05:37:35PM +0200, Arnd Bergmann wrote:
> On Thursday, June 30, 2016 5:19:30 PM CEST Thierry Reding wrote:
> > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > 
> > This patch makes the existing pci_host_bridge structure a proper device
> > that is usable by PCI host drivers in a more standard way. In addition
> > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > and pci_create_root_bus interfaces, this unfortunately means having to
> > add yet another interface doing basically the same thing, and add some
> > extra code in the initial step.
> 
> Thanks a lot for following up on my patches and getting them to work!
> 
> > As mentioned in a previous email, one open question is whether we want
> > to export a function for allocating a pci_host_bridge device in
> > combination with the per-device structure or let the driver itself
> > call kzalloc.
> > 
> > Changes in v2 (Thierry Reding):
> > - add a pci_host_bridge_init() helper that drivers can use to perform
> >   all the necessary steps to initialize the bridge
> 
> For these two points, the addition of pci_host_bridge_init() to me
> tips the balance in favour of having a separate allocation function
> that also does the initialization, something like
> 
> struct struct pci_host_bridge *pci_host_bridge_alloc(size_t priv)
> {
> 	struct pci_host_bridge *bridge;
> 
> 	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> 	if (!bridge)
> 		return NULL;
> 
> 	INIT_LIST_HEAD(&bridge->windows);
> 
> 	if (priv)
> 		bridge->private = &bridge[1];
> 
> 	return bridge;
> }

I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
to then go back and add re-add it in a different form?

The old pci_alloc_host_bridge() was doing mostly what the above
function does minus the bridge->private line. And the &bridge[1] construct
could very well be pointing to the next struct pci_host_bridge aligned address,
which means parts of priv area are ignored.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Thu, Jun 30, 2016 at 05:37:35PM +0200, Arnd Bergmann wrote:
> On Thursday, June 30, 2016 5:19:30 PM CEST Thierry Reding wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > This patch makes the existing pci_host_bridge structure a proper device
> > that is usable by PCI host drivers in a more standard way. In addition
> > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > and pci_create_root_bus interfaces, this unfortunately means having to
> > add yet another interface doing basically the same thing, and add some
> > extra code in the initial step.
> 
> Thanks a lot for following up on my patches and getting them to work!
> 
> > As mentioned in a previous email, one open question is whether we want
> > to export a function for allocating a pci_host_bridge device in
> > combination with the per-device structure or let the driver itself
> > call kzalloc.
> > 
> > Changes in v2 (Thierry Reding):
> > - add a pci_host_bridge_init() helper that drivers can use to perform
> >   all the necessary steps to initialize the bridge
> 
> For these two points, the addition of pci_host_bridge_init() to me
> tips the balance in favour of having a separate allocation function
> that also does the initialization, something like
> 
> struct struct pci_host_bridge *pci_host_bridge_alloc(size_t priv)
> {
> 	struct pci_host_bridge *bridge;
> 
> 	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> 	if (!bridge)
> 		return NULL;
> 
> 	INIT_LIST_HEAD(&bridge->windows);
> 
> 	if (priv)
> 		bridge->private = &bridge[1];
> 
> 	return bridge;
> }

I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
to then go back and add re-add it in a different form?

The old pci_alloc_host_bridge() was doing mostly what the above
function does minus the bridge->private line. And the &bridge[1] construct
could very well be pointing to the next struct pci_host_bridge aligned address,
which means parts of priv area are ignored.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:14:47 PM CEST Liviu Dudau wrote:
> 
> I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
> to then go back and add re-add it in a different form?

Good point, we could just keep the existing implementation and make it
an exported function without the bus argument.

> The old pci_alloc_host_bridge() was doing mostly what the above
> function does minus the bridge->private line. And the &bridge[1] construct
> could very well be pointing to the next struct pci_host_bridge aligned address,
> which means parts of priv area are ignored.

I don't know what you mean with that. Would you rather write this as?

	bridge->private = bridge + 1;

or leave it out and add a helper function

void *pci_host_bridge_private(struct pci_host_bridge *bridge)
{
	return &bridge[1];
}

or do you mean we should have extra alignment in there so the
private pointer has a minimum alignment higher than the
alignment of struct pci_host_bridge?

I'm absolutely fine with any of those suggestions, whichever
makes the nicest API.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:14:47 PM CEST Liviu Dudau wrote:
> 
> I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
> to then go back and add re-add it in a different form?

Good point, we could just keep the existing implementation and make it
an exported function without the bus argument.

> The old pci_alloc_host_bridge() was doing mostly what the above
> function does minus the bridge->private line. And the &bridge[1] construct
> could very well be pointing to the next struct pci_host_bridge aligned address,
> which means parts of priv area are ignored.

I don't know what you mean with that. Would you rather write this as?

	bridge->private = bridge + 1;

or leave it out and add a helper function

void *pci_host_bridge_private(struct pci_host_bridge *bridge)
{
	return &bridge[1];
}

or do you mean we should have extra alignment in there so the
private pointer has a minimum alignment higher than the
alignment of struct pci_host_bridge?

I'm absolutely fine with any of those suggestions, whichever
makes the nicest API.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Fri, Jul 01, 2016 at 04:24:50PM +0200, Arnd Bergmann wrote:
> On Friday, July 1, 2016 3:14:47 PM CEST Liviu Dudau wrote:
> > 
> > I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
> > to then go back and add re-add it in a different form?
> 
> Good point, we could just keep the existing implementation and make it
> an exported function without the bus argument.
> 
> > The old pci_alloc_host_bridge() was doing mostly what the above
> > function does minus the bridge->private line. And the &bridge[1] construct
> > could very well be pointing to the next struct pci_host_bridge aligned address,
> > which means parts of priv area are ignored.
> 
> I don't know what you mean with that. Would you rather write this as?
> 
> 	bridge->private = bridge + 1;
> 
> or leave it out and add a helper function
> 
> void *pci_host_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return &bridge[1];
> }

none of the above

> 
> or do you mean we should have extra alignment in there so the
> private pointer has a minimum alignment higher than the
> alignment of struct pci_host_bridge?

but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
architecture.

> 
> I'm absolutely fine with any of those suggestions, whichever
> makes the nicest API.

Does anyone need to subclass the pci_host_bridge structure? And is appending data right
after the structure useful?

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Fri, Jul 01, 2016 at 04:24:50PM +0200, Arnd Bergmann wrote:
> On Friday, July 1, 2016 3:14:47 PM CEST Liviu Dudau wrote:
> > 
> > I'm confused. Why are we butchering the old pci_alloc_host_bridge() function
> > to then go back and add re-add it in a different form?
> 
> Good point, we could just keep the existing implementation and make it
> an exported function without the bus argument.
> 
> > The old pci_alloc_host_bridge() was doing mostly what the above
> > function does minus the bridge->private line. And the &bridge[1] construct
> > could very well be pointing to the next struct pci_host_bridge aligned address,
> > which means parts of priv area are ignored.
> 
> I don't know what you mean with that. Would you rather write this as?
> 
> 	bridge->private = bridge + 1;
> 
> or leave it out and add a helper function
> 
> void *pci_host_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return &bridge[1];
> }

none of the above

> 
> or do you mean we should have extra alignment in there so the
> private pointer has a minimum alignment higher than the
> alignment of struct pci_host_bridge?

but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
architecture.

> 
> I'm absolutely fine with any of those suggestions, whichever
> makes the nicest API.

Does anyone need to subclass the pci_host_bridge structure? And is appending data right
after the structure useful?

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:52:44 PM CEST Liviu Dudau wrote:
> 
> > 
> > or do you mean we should have extra alignment in there so the
> > private pointer has a minimum alignment higher than the
> > alignment of struct pci_host_bridge?
> 
> but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
> bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
> architecture.

No, that's not how it works.

> > I'm absolutely fine with any of those suggestions, whichever
> > makes the nicest API.
> 
> Does anyone need to subclass the pci_host_bridge structure? And is appending data right
> after the structure useful?

I was basically following the way alloc_etherdev() and a lot of other
subsystems handle it.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:52:44 PM CEST Liviu Dudau wrote:
> 
> > 
> > or do you mean we should have extra alignment in there so the
> > private pointer has a minimum alignment higher than the
> > alignment of struct pci_host_bridge?
> 
> but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
> bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
> architecture.

No, that's not how it works.

> > I'm absolutely fine with any of those suggestions, whichever
> > makes the nicest API.
> 
> Does anyone need to subclass the pci_host_bridge structure? And is appending data right
> after the structure useful?

I was basically following the way alloc_etherdev() and a lot of other
subsystems handle it.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Fri, Jul 01, 2016 at 05:17:11PM +0200, Arnd Bergmann wrote:
> On Friday, July 1, 2016 3:52:44 PM CEST Liviu Dudau wrote:
> > 
> > > 
> > > or do you mean we should have extra alignment in there so the
> > > private pointer has a minimum alignment higher than the
> > > alignment of struct pci_host_bridge?
> > 
> > but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
> > bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
> > architecture.
> 
> No, that's not how it works.

Really? If struct foo takes 31 bytes, and struct foo *p = (struct foo*)64, what's p's
value after p++ ? 95? I thought the compiler is allowed to consider the structure padded so that
p++ is 96.

> 
> > > I'm absolutely fine with any of those suggestions, whichever
> > > makes the nicest API.
> > 
> > Does anyone need to subclass the pci_host_bridge structure? And is appending data right
> > after the structure useful?
> 
> I was basically following the way alloc_etherdev() and a lot of other
> subsystems handle it.

Probably valid if casted to char* ?

Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 4:40:46 PM CEST Liviu Dudau wrote:
> On Fri, Jul 01, 2016 at 05:17:11PM +0200, Arnd Bergmann wrote:
> > On Friday, July 1, 2016 3:52:44 PM CEST Liviu Dudau wrote:
> > > 
> > > > 
> > > > or do you mean we should have extra alignment in there so the
> > > > private pointer has a minimum alignment higher than the
> > > > alignment of struct pci_host_bridge?
> > > 
> > > but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
> > > bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
> > > architecture.
> > 
> > No, that's not how it works.
> 
> Really? If struct foo takes 31 bytes, and struct foo *p = (struct foo*)64, what's p's
> value after p++ ? 95? I thought the compiler is allowed to consider the structure padded so that
> p++ is 96.

In that example, sizeof(struct foo) is 32, so we get to the right result,
for any type, this is true:

	(char *)((struct foo *)p + 1) == (char *)p + sizeof(struct foo);

However, there is indeed a problem in the case that the private structure
requires a larger alignment than struct pci_host_bridge, so adding some
bytes for alignment the way that alloc_etherdev() does is probably
a good idea to be on the safe side, so we could do

	bridge = kzalloc(ALIGN(sizeof(struct pci_host_bridge), ARCH_KMALLOC_MINALIGN) +
					     sizeof_priv, GFP_KERNEL);
	priv = PTR_ALIGN(bridge + 1, ARCH_KMALLOC_MINALIGN);

which will guarantee that both bridge and priv are aligned to
ARCH_KMALLOC_MINALIGN and the allocation is large enough to hold
both.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 4:40:46 PM CEST Liviu Dudau wrote:
> On Fri, Jul 01, 2016 at 05:17:11PM +0200, Arnd Bergmann wrote:
> > On Friday, July 1, 2016 3:52:44 PM CEST Liviu Dudau wrote:
> > > 
> > > > 
> > > > or do you mean we should have extra alignment in there so the
> > > > private pointer has a minimum alignment higher than the
> > > > alignment of struct pci_host_bridge?
> > > 
> > > but this ^. bridge pointer arithmetic means +1 is not necessarily +sizeof(struct pci_host_bridge)
> > > bytes. AFAIK that can be rounded to the nearest natural alignment for pointers on that
> > > architecture.
> > 
> > No, that's not how it works.
> 
> Really? If struct foo takes 31 bytes, and struct foo *p = (struct foo*)64, what's p's
> value after p++ ? 95? I thought the compiler is allowed to consider the structure padded so that
> p++ is 96.

In that example, sizeof(struct foo) is 32, so we get to the right result,
for any type, this is true:

	(char *)((struct foo *)p + 1) == (char *)p + sizeof(struct foo);

However, there is indeed a problem in the case that the private structure
requires a larger alignment than struct pci_host_bridge, so adding some
bytes for alignment the way that alloc_etherdev() does is probably
a good idea to be on the safe side, so we could do

	bridge = kzalloc(ALIGN(sizeof(struct pci_host_bridge), ARCH_KMALLOC_MINALIGN) +
					     sizeof_priv, GFP_KERNEL);
	priv = PTR_ALIGN(bridge + 1, ARCH_KMALLOC_MINALIGN);

which will guarantee that both bridge and priv are aligned to
ARCH_KMALLOC_MINALIGN and the allocation is large enough to hold
both.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.
> 
> However, this time it's more likely to be extensible enough that we
> won't have to do another one again in the future, and we should be
> able to reduce code much more as a result.

I am really disapointed by the direction in which the whole pci_host_bridge
structure and associated functions is going. When I started to get involved
in this mess that is the creation of a root PCI bus I was hoping that we
are moving towards one or few functions that create the root bus by using
the details provided in the host bridge without having so many variants for
the functionality.

One reason for this holy mess is the duplication of information. Both root
bus and the pci_host_bridge hold pointers to useful information (msi_controller,
pci_ops) and coherency should be guaranteed when one or the other gets created.
Hence the ridiculous dance being done to find if root bus hasn't already been
created and if not reusing the scrap bus we have created at the top of
pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
oops we need more information, and so on. (apologies for the incoherent style,
I'm trying to duplicate the spirit of the probe.c code :) )

I think this patchset has the right intention but it is not doing it in
the right way. To me, the right way is to separate the whole allocation
of the pci_host_bridge structure from the scanning or the root bus (keeping the
INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
*at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
done.

Think of the code structure as a reflection of how the system is structured: the
PCI-to-host bridge is a structure that holds the information required to connect
the functionality of the host code (msi_controller, host-to-bus resource windows)
to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
possible.

> 
> The main idea is to pull the allocation of 'struct pci_host_bridge' out
> of the registration, and let individual host drivers and architecture
> code fill the members before calling the registration function.

That's how we got into the arch/arm mess that we currently have. The host driver
should not drive the instantiation of the pci_host_bridge structure because it
will prevent you from having two drivers running at the same time.

> 
> There are a number of things we can do based on this:
> 
> * Use a single memory allocation for the driver-specific structure
>   and the generic PCI host bridge
> * consolidate the contents of driver specific structures by moving
>   them into pci_host_bridge
> * Add a consistent interface for removing a PCI host bridge again
>   when unloading a host driver module
> * Replace the architecture specific __weak pcibios_* functions with
>   callbacks in a pci_host_bridge device
> * Move common boilerplate code from host drivers into the generic
>   function, based on contents of the structure
> * Extend pci_host_bridge with additional members when needed without
>   having to add arguments to pci_scan_*.
> * Move members of struct pci_bus into pci_host_bridge to avoid
>   having lots of identical copies.
> 
> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.
> 
> Changes in v2 (Thierry Reding):
> - add a pci_host_bridge_init() helper that drivers can use to perform
>   all the necessary steps to initialize the bridge
> - rename pci_register_host() to pci_host_bridge_register() to reflect
>   the naming used by other functions
> - plug memory leak on registration failure
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/pci/probe.c | 111 ++++++++++++++++++++++++++++++++--------------------
>  include/linux/pci.h |   8 +++-
>  2 files changed, 75 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 77f0bbb05c3f..ba40514acd32 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -521,19 +521,6 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -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)
> -		return NULL;
> -
> -	INIT_LIST_HEAD(&bridge->windows);
> -	bridge->bus = b;
> -	return bridge;
> -}
> -

You might have strong arguments for removing this function but they are not properly
explained here and I feel that they should. Specially as ....

>  static const unsigned char pcix_bus_speed[] = {
>  	PCI_SPEED_UNKNOWN,		/* 0 */
>  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> @@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> +{
> +	INIT_LIST_HEAD(&bridge->windows);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_init);

I have no idea why it is useful to re-initialise the bridge->windows list at any
time other than allocation time.

> +
> +int pci_host_bridge_register(struct pci_host_bridge *bridge)
>  {
>  	int error;
> -	struct pci_host_bridge *bridge;
>  	struct pci_bus *b, *b2;
>  	struct resource_entry *window, *n;
> +	LIST_HEAD(resources);
>  	struct resource *res;
>  	resource_size_t offset;
>  	char bus_addr[64];
>  	char *fmt;
> +	struct device *parent = bridge->dev.parent;
>  
>  	b = pci_alloc_bus(NULL);
>  	if (!b)
> -		return NULL;
> +		return -ENOMEM;
> +	bridge->bus = b;
>  
> -	b->sysdata = sysdata;
> -	b->ops = ops;
> -	b->number = b->busn_res.start = bus;
> +	/* temporarily move resources off the list */
> +	list_splice_init(&bridge->windows, &resources);

What are you trying to accomplish here with the moving around of the bridge->windows list
elements? This also leaves the bridge->windows list empty afterwards, is that intended?

> +	b->sysdata = bridge->sysdata;
> +	b->msi = bridge->msi;
> +	b->ops = bridge->ops;
> +	b->number = b->busn_res.start = bridge->busnr;
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	b->domain_nr = pci_bus_find_domain_nr(b, parent);
>  #endif
> -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> +	b2 = pci_find_bus(pci_domain_nr(b), bridge->busnr);
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> +		error = -EEXIST;
>  		goto err_out;
>  	}
>  
> -	bridge = pci_alloc_host_bridge(b);
> -	if (!bridge)
> -		goto err_out;
> -
> -	bridge->dev.parent = parent;
> -	bridge->dev.release = pci_release_host_bridge_dev;
> -	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b),
> +		     bridge->busnr);
>  	error = pcibios_root_bridge_prepare(bridge);
> -	if (error) {
> -		kfree(bridge);
> +	if (error)
>  		goto err_out;
> -	}
>  
>  	error = device_register(&bridge->dev);
> -	if (error) {
> +	if (error)
>  		put_device(&bridge->dev);
> -		goto err_out;
> -	}
> +
>  	b->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(b->bridge);
>  	pci_set_bus_of_node(b);
> @@ -2174,7 +2164,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	b->dev.class = &pcibus_class;
>  	b->dev.parent = b->bridge;
> -	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bridge->busnr);
>  	error = device_register(&b->dev);
>  	if (error)
>  		goto class_dev_reg_err;
> @@ -2190,12 +2180,12 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  		printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
>  
>  	/* Add initial resources to the bus */
> -	resource_list_for_each_entry_safe(window, n, resources) {
> +	resource_list_for_each_entry_safe(window, n, &resources) {
>  		list_move_tail(&window->node, &bridge->windows);
>  		res = window->res;
>  		offset = window->offset;
>  		if (res->flags & IORESOURCE_BUS)
> -			pci_bus_insert_busn_res(b, bus, res->end);
> +			pci_bus_insert_busn_res(b, bridge->busnr, res->end);
>  		else
>  			pci_bus_add_resource(b, res, 0);
>  		if (offset) {
> @@ -2215,16 +2205,16 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	list_add_tail(&b->node, &pci_root_buses);
>  	up_write(&pci_bus_sem);
>  
> -	return b;
> +	return 0;
>  
>  class_dev_reg_err:
>  	put_device(&bridge->dev);
>  	device_unregister(&bridge->dev);
>  err_out:
>  	kfree(b);
> -	return NULL;
> +	return error;
>  }
> -EXPORT_SYMBOL_GPL(pci_create_root_bus);
> +EXPORT_SYMBOL_GPL(pci_host_bridge_register);
>  
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>  {
> @@ -2289,6 +2279,43 @@ void pci_bus_release_busn_res(struct pci_bus *b)
>  			res, ret ? "can not be" : "is");
>  }
>  
> +static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
> +		int bus, struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources, struct msi_controller *msi)
> +{
> +	struct pci_host_bridge *bridge;
> +	int ret;
> +
> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return NULL;
> +
> +	pci_host_bridge_init(bridge);
> +	bridge->dev.parent = parent;
> +	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->busnr = bus;
> +	bridge->ops = ops;
> +	bridge->sysdata = sysdata;
> +	bridge->msi = msi;
> +	list_splice_init(resources, &bridge->windows);
> +
> +	ret = pci_host_bridge_register(bridge);
> +	if (ret) {
> +		kfree(bridge);
> +		return NULL;
> +	}
> +
> +	return bridge->bus;
> +}
> +
> +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	return pci_create_root_bus_msi(parent, bus, ops, sysdata, resources,
> +				       NULL);
> +}
> +EXPORT_SYMBOL_GPL(pci_create_root_bus);
> +
>  struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>  		struct pci_ops *ops, void *sysdata,
>  		struct list_head *resources, struct msi_controller *msi)
> @@ -2304,12 +2331,10 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>  			break;
>  		}
>  
> -	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
> +	b = pci_create_root_bus_msi(parent, bus, ops, sysdata, resources, msi);
>  	if (!b)
>  		return NULL;
>  
> -	b->msi = msi;
> -
>  	if (!found) {
>  		dev_info(&b->dev,
>  		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8badb664be00..5bf04e20c1cd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -405,10 +405,14 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
>  
>  struct pci_host_bridge {
>  	struct device dev;
> -	struct pci_bus *bus;		/* root bus */
> +	struct pci_ops *ops;
> +	void *sysdata;
> +	int busnr;
> +	struct pci_bus *bus;		/* points to root */
>  	struct list_head windows;	/* resource_entry */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> +	struct msi_controller *msi;
>  	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> @@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  				    struct pci_ops *ops, void *sysdata,
>  				    struct list_head *resources);
> +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> +int pci_host_bridge_register(struct pci_host_bridge *bridge);
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>  void pci_bus_release_busn_res(struct pci_bus *b);
> -- 
> 2.8.3
> 

There is too much stuff moving around. I know it is needed and the temptation is to get it
done as quickly as possible, but it makes the reviewing and the commenting on this series hard.
Can you split the patch into one that adds the new members into pci_host_bridge, then do the
renaming/re-organisation in another patch?

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.
> 
> However, this time it's more likely to be extensible enough that we
> won't have to do another one again in the future, and we should be
> able to reduce code much more as a result.

I am really disapointed by the direction in which the whole pci_host_bridge
structure and associated functions is going. When I started to get involved
in this mess that is the creation of a root PCI bus I was hoping that we
are moving towards one or few functions that create the root bus by using
the details provided in the host bridge without having so many variants for
the functionality.

One reason for this holy mess is the duplication of information. Both root
bus and the pci_host_bridge hold pointers to useful information (msi_controller,
pci_ops) and coherency should be guaranteed when one or the other gets created.
Hence the ridiculous dance being done to find if root bus hasn't already been
created and if not reusing the scrap bus we have created at the top of
pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
oops we need more information, and so on. (apologies for the incoherent style,
I'm trying to duplicate the spirit of the probe.c code :) )

I think this patchset has the right intention but it is not doing it in
the right way. To me, the right way is to separate the whole allocation
of the pci_host_bridge structure from the scanning or the root bus (keeping the
INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
*at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
done.

Think of the code structure as a reflection of how the system is structured: the
PCI-to-host bridge is a structure that holds the information required to connect
the functionality of the host code (msi_controller, host-to-bus resource windows)
to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
possible.

> 
> The main idea is to pull the allocation of 'struct pci_host_bridge' out
> of the registration, and let individual host drivers and architecture
> code fill the members before calling the registration function.

That's how we got into the arch/arm mess that we currently have. The host driver
should not drive the instantiation of the pci_host_bridge structure because it
will prevent you from having two drivers running at the same time.

> 
> There are a number of things we can do based on this:
> 
> * Use a single memory allocation for the driver-specific structure
>   and the generic PCI host bridge
> * consolidate the contents of driver specific structures by moving
>   them into pci_host_bridge
> * Add a consistent interface for removing a PCI host bridge again
>   when unloading a host driver module
> * Replace the architecture specific __weak pcibios_* functions with
>   callbacks in a pci_host_bridge device
> * Move common boilerplate code from host drivers into the generic
>   function, based on contents of the structure
> * Extend pci_host_bridge with additional members when needed without
>   having to add arguments to pci_scan_*.
> * Move members of struct pci_bus into pci_host_bridge to avoid
>   having lots of identical copies.
> 
> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.
> 
> Changes in v2 (Thierry Reding):
> - add a pci_host_bridge_init() helper that drivers can use to perform
>   all the necessary steps to initialize the bridge
> - rename pci_register_host() to pci_host_bridge_register() to reflect
>   the naming used by other functions
> - plug memory leak on registration failure
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/probe.c | 111 ++++++++++++++++++++++++++++++++--------------------
>  include/linux/pci.h |   8 +++-
>  2 files changed, 75 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 77f0bbb05c3f..ba40514acd32 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -521,19 +521,6 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -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)
> -		return NULL;
> -
> -	INIT_LIST_HEAD(&bridge->windows);
> -	bridge->bus = b;
> -	return bridge;
> -}
> -

You might have strong arguments for removing this function but they are not properly
explained here and I feel that they should. Specially as ....

>  static const unsigned char pcix_bus_speed[] = {
>  	PCI_SPEED_UNKNOWN,		/* 0 */
>  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> @@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> +{
> +	INIT_LIST_HEAD(&bridge->windows);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_init);

I have no idea why it is useful to re-initialise the bridge->windows list at any
time other than allocation time.

> +
> +int pci_host_bridge_register(struct pci_host_bridge *bridge)
>  {
>  	int error;
> -	struct pci_host_bridge *bridge;
>  	struct pci_bus *b, *b2;
>  	struct resource_entry *window, *n;
> +	LIST_HEAD(resources);
>  	struct resource *res;
>  	resource_size_t offset;
>  	char bus_addr[64];
>  	char *fmt;
> +	struct device *parent = bridge->dev.parent;
>  
>  	b = pci_alloc_bus(NULL);
>  	if (!b)
> -		return NULL;
> +		return -ENOMEM;
> +	bridge->bus = b;
>  
> -	b->sysdata = sysdata;
> -	b->ops = ops;
> -	b->number = b->busn_res.start = bus;
> +	/* temporarily move resources off the list */
> +	list_splice_init(&bridge->windows, &resources);

What are you trying to accomplish here with the moving around of the bridge->windows list
elements? This also leaves the bridge->windows list empty afterwards, is that intended?

> +	b->sysdata = bridge->sysdata;
> +	b->msi = bridge->msi;
> +	b->ops = bridge->ops;
> +	b->number = b->busn_res.start = bridge->busnr;
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	b->domain_nr = pci_bus_find_domain_nr(b, parent);
>  #endif
> -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> +	b2 = pci_find_bus(pci_domain_nr(b), bridge->busnr);
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> +		error = -EEXIST;
>  		goto err_out;
>  	}
>  
> -	bridge = pci_alloc_host_bridge(b);
> -	if (!bridge)
> -		goto err_out;
> -
> -	bridge->dev.parent = parent;
> -	bridge->dev.release = pci_release_host_bridge_dev;
> -	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b),
> +		     bridge->busnr);
>  	error = pcibios_root_bridge_prepare(bridge);
> -	if (error) {
> -		kfree(bridge);
> +	if (error)
>  		goto err_out;
> -	}
>  
>  	error = device_register(&bridge->dev);
> -	if (error) {
> +	if (error)
>  		put_device(&bridge->dev);
> -		goto err_out;
> -	}
> +
>  	b->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(b->bridge);
>  	pci_set_bus_of_node(b);
> @@ -2174,7 +2164,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	b->dev.class = &pcibus_class;
>  	b->dev.parent = b->bridge;
> -	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bridge->busnr);
>  	error = device_register(&b->dev);
>  	if (error)
>  		goto class_dev_reg_err;
> @@ -2190,12 +2180,12 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  		printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
>  
>  	/* Add initial resources to the bus */
> -	resource_list_for_each_entry_safe(window, n, resources) {
> +	resource_list_for_each_entry_safe(window, n, &resources) {
>  		list_move_tail(&window->node, &bridge->windows);
>  		res = window->res;
>  		offset = window->offset;
>  		if (res->flags & IORESOURCE_BUS)
> -			pci_bus_insert_busn_res(b, bus, res->end);
> +			pci_bus_insert_busn_res(b, bridge->busnr, res->end);
>  		else
>  			pci_bus_add_resource(b, res, 0);
>  		if (offset) {
> @@ -2215,16 +2205,16 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	list_add_tail(&b->node, &pci_root_buses);
>  	up_write(&pci_bus_sem);
>  
> -	return b;
> +	return 0;
>  
>  class_dev_reg_err:
>  	put_device(&bridge->dev);
>  	device_unregister(&bridge->dev);
>  err_out:
>  	kfree(b);
> -	return NULL;
> +	return error;
>  }
> -EXPORT_SYMBOL_GPL(pci_create_root_bus);
> +EXPORT_SYMBOL_GPL(pci_host_bridge_register);
>  
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>  {
> @@ -2289,6 +2279,43 @@ void pci_bus_release_busn_res(struct pci_bus *b)
>  			res, ret ? "can not be" : "is");
>  }
>  
> +static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
> +		int bus, struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources, struct msi_controller *msi)
> +{
> +	struct pci_host_bridge *bridge;
> +	int ret;
> +
> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return NULL;
> +
> +	pci_host_bridge_init(bridge);
> +	bridge->dev.parent = parent;
> +	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->busnr = bus;
> +	bridge->ops = ops;
> +	bridge->sysdata = sysdata;
> +	bridge->msi = msi;
> +	list_splice_init(resources, &bridge->windows);
> +
> +	ret = pci_host_bridge_register(bridge);
> +	if (ret) {
> +		kfree(bridge);
> +		return NULL;
> +	}
> +
> +	return bridge->bus;
> +}
> +
> +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	return pci_create_root_bus_msi(parent, bus, ops, sysdata, resources,
> +				       NULL);
> +}
> +EXPORT_SYMBOL_GPL(pci_create_root_bus);
> +
>  struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>  		struct pci_ops *ops, void *sysdata,
>  		struct list_head *resources, struct msi_controller *msi)
> @@ -2304,12 +2331,10 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>  			break;
>  		}
>  
> -	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
> +	b = pci_create_root_bus_msi(parent, bus, ops, sysdata, resources, msi);
>  	if (!b)
>  		return NULL;
>  
> -	b->msi = msi;
> -
>  	if (!found) {
>  		dev_info(&b->dev,
>  		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8badb664be00..5bf04e20c1cd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -405,10 +405,14 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
>  
>  struct pci_host_bridge {
>  	struct device dev;
> -	struct pci_bus *bus;		/* root bus */
> +	struct pci_ops *ops;
> +	void *sysdata;
> +	int busnr;
> +	struct pci_bus *bus;		/* points to root */
>  	struct list_head windows;	/* resource_entry */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> +	struct msi_controller *msi;
>  	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> @@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  				    struct pci_ops *ops, void *sysdata,
>  				    struct list_head *resources);
> +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> +int pci_host_bridge_register(struct pci_host_bridge *bridge);
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>  void pci_bus_release_busn_res(struct pci_bus *b);
> -- 
> 2.8.3
> 

There is too much stuff moving around. I know it is needed and the temptation is to get it
done as quickly as possible, but it makes the reviewing and the commenting on this series hard.
Can you split the patch into one that adds the new members into pci_host_bridge, then do the
renaming/re-organisation in another patch?

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:46:48 PM CEST Liviu Dudau wrote:
> On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > 
> > This patch makes the existing pci_host_bridge structure a proper device
> > that is usable by PCI host drivers in a more standard way. In addition
> > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > and pci_create_root_bus interfaces, this unfortunately means having to
> > add yet another interface doing basically the same thing, and add some
> > extra code in the initial step.
> > 
> > However, this time it's more likely to be extensible enough that we
> > won't have to do another one again in the future, and we should be
> > able to reduce code much more as a result.
> 
> I am really disapointed by the direction in which the whole pci_host_bridge
> structure and associated functions is going. When I started to get involved
> in this mess that is the creation of a root PCI bus I was hoping that we
> are moving towards one or few functions that create the root bus by using
> the details provided in the host bridge without having so many variants for
> the functionality.

That is the plan, we just never made a lot of progress here.

> One reason for this holy mess is the duplication of information. Both root
> bus and the pci_host_bridge hold pointers to useful information (msi_controller,
> pci_ops) and coherency should be guaranteed when one or the other gets created.
> Hence the ridiculous dance being done to find if root bus hasn't already been
> created and if not reusing the scrap bus we have created at the top of
> pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
> oops we need more information, and so on. (apologies for the incoherent style,
> I'm trying to duplicate the spirit of the probe.c code :) )

Agreed.

> I think this patchset has the right intention but it is not doing it in
> the right way. To me, the right way is to separate the whole allocation
> of the pci_host_bridge structure from the scanning or the root bus (keeping the
> INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
> *at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
> and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
> done.

Again, this is what I'm doing here, unfortunately we cannot remove the
sysdata pointer altogether as we still have over a hundred existing PCI
host bridge implementations that all use sysdata.

> Think of the code structure as a reflection of how the system is structured: the
> PCI-to-host bridge is a structure that holds the information required to connect
> the functionality of the host code (msi_controller, host-to-bus resource windows)
> to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
> possible.

Exactly.

> > 
> > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > of the registration, and let individual host drivers and architecture
> > code fill the members before calling the registration function.
> 
> That's how we got into the arch/arm mess that we currently have. The host driver
> should not drive the instantiation of the pci_host_bridge structure because it
> will prevent you from having two drivers running at the same time.

Can you clarify that? I don't see what prevents two drivers from each
creating a pci_host_bridge structure and passing it into
pci_host_bridge_register().

> > -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)
> > -		return NULL;
> > -
> > -	INIT_LIST_HEAD(&bridge->windows);
> > -	bridge->bus = b;
> > -	return bridge;
> > -}
> > -
> 
> You might have strong arguments for removing this function but they are not properly
> explained here and I feel that they should. Specially as ....

In my initial version, it simply became unnecessary because all callers
of pci_host_bridge_register() would have to allocate it anyway, and with
the pci_bus assignment gone, it didn't do much at all.

> >  static const unsigned char pcix_bus_speed[] = {
> >  	PCI_SPEED_UNKNOWN,		/* 0 */
> >  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> > @@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
> >  {
> >  }
> >  
> > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> > +{
> > +	INIT_LIST_HEAD(&bridge->windows);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_bridge_init);
> 
> I have no idea why it is useful to re-initialise the bridge->windows list at any
> time other than allocation time.

That's why I suggested doing this in the allocation function now.

> > +int pci_host_bridge_register(struct pci_host_bridge *bridge)
> >  {
> >  	int error;
> > -	struct pci_host_bridge *bridge;
> >  	struct pci_bus *b, *b2;
> >  	struct resource_entry *window, *n;
> > +	LIST_HEAD(resources);
> >  	struct resource *res;
> >  	resource_size_t offset;
> >  	char bus_addr[64];
> >  	char *fmt;
> > +	struct device *parent = bridge->dev.parent;
> >  
> >  	b = pci_alloc_bus(NULL);
> >  	if (!b)
> > -		return NULL;
> > +		return -ENOMEM;
> > +	bridge->bus = b;
> >  
> > -	b->sysdata = sysdata;
> > -	b->ops = ops;
> > -	b->number = b->busn_res.start = bus;
> > +	/* temporarily move resources off the list */
> > +	list_splice_init(&bridge->windows, &resources);
> 
> What are you trying to accomplish here with the moving around of the bridge->windows list
> elements? This also leaves the bridge->windows list empty afterwards, is that intended?

I was trying to preserve the existing behavior, this can probably
be simplified, but as my initial version was completely untested
I decided not to touch it.

Later in the function, the list is filled one entry at a time from
the local 'resources' list that we traditionally passed as a function
argument before.

Ideally we'd just walk the list instead of doing the
split/list_del/list_add dance, and this would be a logical
cleanup on top of this patch.

> > @@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
> >  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >  				    struct pci_ops *ops, void *sysdata,
> >  				    struct list_head *resources);
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> > +int pci_host_bridge_register(struct pci_host_bridge *bridge);
> >  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> >  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >  void pci_bus_release_busn_res(struct pci_bus *b);
> 
> There is too much stuff moving around. I know it is needed and the temptation is to get it
> done as quickly as possible, but it makes the reviewing and the commenting on this series hard.

This is why I left the list handling you mentioned above unchanged
in my original patch instead of rewriting it.

> Can you split the patch into one that adds the new members
> into pci_host_bridge, then do the renaming/re-organisation in
> another patch?

That should be possible, I guess Thierry can deal with that when respinning
the series.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 3:46:48 PM CEST Liviu Dudau wrote:
> On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > This patch makes the existing pci_host_bridge structure a proper device
> > that is usable by PCI host drivers in a more standard way. In addition
> > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > and pci_create_root_bus interfaces, this unfortunately means having to
> > add yet another interface doing basically the same thing, and add some
> > extra code in the initial step.
> > 
> > However, this time it's more likely to be extensible enough that we
> > won't have to do another one again in the future, and we should be
> > able to reduce code much more as a result.
> 
> I am really disapointed by the direction in which the whole pci_host_bridge
> structure and associated functions is going. When I started to get involved
> in this mess that is the creation of a root PCI bus I was hoping that we
> are moving towards one or few functions that create the root bus by using
> the details provided in the host bridge without having so many variants for
> the functionality.

That is the plan, we just never made a lot of progress here.

> One reason for this holy mess is the duplication of information. Both root
> bus and the pci_host_bridge hold pointers to useful information (msi_controller,
> pci_ops) and coherency should be guaranteed when one or the other gets created.
> Hence the ridiculous dance being done to find if root bus hasn't already been
> created and if not reusing the scrap bus we have created at the top of
> pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
> oops we need more information, and so on. (apologies for the incoherent style,
> I'm trying to duplicate the spirit of the probe.c code :) )

Agreed.

> I think this patchset has the right intention but it is not doing it in
> the right way. To me, the right way is to separate the whole allocation
> of the pci_host_bridge structure from the scanning or the root bus (keeping the
> INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
> *at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
> and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
> done.

Again, this is what I'm doing here, unfortunately we cannot remove the
sysdata pointer altogether as we still have over a hundred existing PCI
host bridge implementations that all use sysdata.

> Think of the code structure as a reflection of how the system is structured: the
> PCI-to-host bridge is a structure that holds the information required to connect
> the functionality of the host code (msi_controller, host-to-bus resource windows)
> to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
> possible.

Exactly.

> > 
> > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > of the registration, and let individual host drivers and architecture
> > code fill the members before calling the registration function.
> 
> That's how we got into the arch/arm mess that we currently have. The host driver
> should not drive the instantiation of the pci_host_bridge structure because it
> will prevent you from having two drivers running at the same time.

Can you clarify that? I don't see what prevents two drivers from each
creating a pci_host_bridge structure and passing it into
pci_host_bridge_register().

> > -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)
> > -		return NULL;
> > -
> > -	INIT_LIST_HEAD(&bridge->windows);
> > -	bridge->bus = b;
> > -	return bridge;
> > -}
> > -
> 
> You might have strong arguments for removing this function but they are not properly
> explained here and I feel that they should. Specially as ....

In my initial version, it simply became unnecessary because all callers
of pci_host_bridge_register() would have to allocate it anyway, and with
the pci_bus assignment gone, it didn't do much at all.

> >  static const unsigned char pcix_bus_speed[] = {
> >  	PCI_SPEED_UNKNOWN,		/* 0 */
> >  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> > @@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
> >  {
> >  }
> >  
> > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> > +{
> > +	INIT_LIST_HEAD(&bridge->windows);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_bridge_init);
> 
> I have no idea why it is useful to re-initialise the bridge->windows list at any
> time other than allocation time.

That's why I suggested doing this in the allocation function now.

> > +int pci_host_bridge_register(struct pci_host_bridge *bridge)
> >  {
> >  	int error;
> > -	struct pci_host_bridge *bridge;
> >  	struct pci_bus *b, *b2;
> >  	struct resource_entry *window, *n;
> > +	LIST_HEAD(resources);
> >  	struct resource *res;
> >  	resource_size_t offset;
> >  	char bus_addr[64];
> >  	char *fmt;
> > +	struct device *parent = bridge->dev.parent;
> >  
> >  	b = pci_alloc_bus(NULL);
> >  	if (!b)
> > -		return NULL;
> > +		return -ENOMEM;
> > +	bridge->bus = b;
> >  
> > -	b->sysdata = sysdata;
> > -	b->ops = ops;
> > -	b->number = b->busn_res.start = bus;
> > +	/* temporarily move resources off the list */
> > +	list_splice_init(&bridge->windows, &resources);
> 
> What are you trying to accomplish here with the moving around of the bridge->windows list
> elements? This also leaves the bridge->windows list empty afterwards, is that intended?

I was trying to preserve the existing behavior, this can probably
be simplified, but as my initial version was completely untested
I decided not to touch it.

Later in the function, the list is filled one entry at a time from
the local 'resources' list that we traditionally passed as a function
argument before.

Ideally we'd just walk the list instead of doing the
split/list_del/list_add dance, and this would be a logical
cleanup on top of this patch.

> > @@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
> >  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >  				    struct pci_ops *ops, void *sysdata,
> >  				    struct list_head *resources);
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> > +int pci_host_bridge_register(struct pci_host_bridge *bridge);
> >  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> >  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >  void pci_bus_release_busn_res(struct pci_bus *b);
> 
> There is too much stuff moving around. I know it is needed and the temptation is to get it
> done as quickly as possible, but it makes the reviewing and the commenting on this series hard.

This is why I left the list handling you mentioned above unchanged
in my original patch instead of rewriting it.

> Can you split the patch into one that adds the new members
> into pci_host_bridge, then do the renaming/re-organisation in
> another patch?

That should be possible, I guess Thierry can deal with that when respinning
the series.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Fri, Jul 01, 2016 at 05:44:09PM +0200, Arnd Bergmann wrote:
> On Friday, July 1, 2016 3:46:48 PM CEST Liviu Dudau wrote:
> > On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > This patch makes the existing pci_host_bridge structure a proper device
> > > that is usable by PCI host drivers in a more standard way. In addition
> > > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > > and pci_create_root_bus interfaces, this unfortunately means having to
> > > add yet another interface doing basically the same thing, and add some
> > > extra code in the initial step.
> > > 
> > > However, this time it's more likely to be extensible enough that we
> > > won't have to do another one again in the future, and we should be
> > > able to reduce code much more as a result.
> > 
> > I am really disapointed by the direction in which the whole pci_host_bridge
> > structure and associated functions is going. When I started to get involved
> > in this mess that is the creation of a root PCI bus I was hoping that we
> > are moving towards one or few functions that create the root bus by using
> > the details provided in the host bridge without having so many variants for
> > the functionality.
> 
> That is the plan, we just never made a lot of progress here.
> 
> > One reason for this holy mess is the duplication of information. Both root
> > bus and the pci_host_bridge hold pointers to useful information (msi_controller,
> > pci_ops) and coherency should be guaranteed when one or the other gets created.
> > Hence the ridiculous dance being done to find if root bus hasn't already been
> > created and if not reusing the scrap bus we have created at the top of
> > pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
> > oops we need more information, and so on. (apologies for the incoherent style,
> > I'm trying to duplicate the spirit of the probe.c code :) )
> 
> Agreed.
> 
> > I think this patchset has the right intention but it is not doing it in
> > the right way. To me, the right way is to separate the whole allocation
> > of the pci_host_bridge structure from the scanning or the root bus (keeping the
> > INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
> > *at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
> > and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
> > done.
> 
> Again, this is what I'm doing here, unfortunately we cannot remove the
> sysdata pointer altogether as we still have over a hundred existing PCI
> host bridge implementations that all use sysdata.
> 
> > Think of the code structure as a reflection of how the system is structured: the
> > PCI-to-host bridge is a structure that holds the information required to connect
> > the functionality of the host code (msi_controller, host-to-bus resource windows)
> > to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
> > possible.
> 
> Exactly.
> 
> > > 
> > > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > > of the registration, and let individual host drivers and architecture
> > > code fill the members before calling the registration function.
> > 
> > That's how we got into the arch/arm mess that we currently have. The host driver
> > should not drive the instantiation of the pci_host_bridge structure because it
> > will prevent you from having two drivers running at the same time.
> 
> Can you clarify that? I don't see what prevents two drivers from each
> creating a pci_host_bridge structure and passing it into
> pci_host_bridge_register().

Maybe I'm exagerating a bit, but I always disliked the pci_common_init() function and
the way it is creating a pci_host_bridge while relying on hw_pci hooks to get
things ready. I'm pretty sure there are still issues around the fact that a lot of
platform drivers have a subsys_initcall() function that calls pci_common_init(). I
don't want us to go down that path again.

Don't get me wrong, clearly someone needs to create an instance of pci_host_bridge.
What I want people to accept is that in the PCI(e) architecture there is nothing that
stops a piece of HW that used to be the bridge between host and underlying bus into
becoming the bridge between a higher PCI(e) bus and the bus underneath. If the writes
to the configuration registers happens somehow, the Host Bridge doesn't even know if
it is talking to the actual host. Can the driver in that case make sure it did not made
assumptions that were tied to a specific SoC implementation?

> 
> > > -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)
> > > -		return NULL;
> > > -
> > > -	INIT_LIST_HEAD(&bridge->windows);
> > > -	bridge->bus = b;
> > > -	return bridge;
> > > -}
> > > -
> > 
> > You might have strong arguments for removing this function but they are not properly
> > explained here and I feel that they should. Specially as ....
> 
> In my initial version, it simply became unnecessary because all callers
> of pci_host_bridge_register() would have to allocate it anyway, and with
> the pci_bus assignment gone, it didn't do much at all.
> 
> > >  static const unsigned char pcix_bus_speed[] = {
> > >  	PCI_SPEED_UNKNOWN,		/* 0 */
> > >  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> > > @@ -2117,53 +2104,56 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
> > >  {
> > >  }
> > >  
> > > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > > -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> > > +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> > > +{
> > > +	INIT_LIST_HEAD(&bridge->windows);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_host_bridge_init);
> > 
> > I have no idea why it is useful to re-initialise the bridge->windows list at any
> > time other than allocation time.
> 
> That's why I suggested doing this in the allocation function now.

Understood. Thanks for the explanations, I did not manage to keep track of all the old
discussions around PCIe, specially if they were prefixed with ACPI ;)

Best regards,
Liviu

> 
> > > +int pci_host_bridge_register(struct pci_host_bridge *bridge)
> > >  {
> > >  	int error;
> > > -	struct pci_host_bridge *bridge;
> > >  	struct pci_bus *b, *b2;
> > >  	struct resource_entry *window, *n;
> > > +	LIST_HEAD(resources);
> > >  	struct resource *res;
> > >  	resource_size_t offset;
> > >  	char bus_addr[64];
> > >  	char *fmt;
> > > +	struct device *parent = bridge->dev.parent;
> > >  
> > >  	b = pci_alloc_bus(NULL);
> > >  	if (!b)
> > > -		return NULL;
> > > +		return -ENOMEM;
> > > +	bridge->bus = b;
> > >  
> > > -	b->sysdata = sysdata;
> > > -	b->ops = ops;
> > > -	b->number = b->busn_res.start = bus;
> > > +	/* temporarily move resources off the list */
> > > +	list_splice_init(&bridge->windows, &resources);
> > 
> > What are you trying to accomplish here with the moving around of the bridge->windows list
> > elements? This also leaves the bridge->windows list empty afterwards, is that intended?
> 
> I was trying to preserve the existing behavior, this can probably
> be simplified, but as my initial version was completely untested
> I decided not to touch it.
> 
> Later in the function, the list is filled one entry at a time from
> the local 'resources' list that we traditionally passed as a function
> argument before.
> 
> Ideally we'd just walk the list instead of doing the
> split/list_del/list_add dance, and this would be a logical
> cleanup on top of this patch.
> 
> > > @@ -817,6 +821,8 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
> > >  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > >  				    struct pci_ops *ops, void *sysdata,
> > >  				    struct list_head *resources);
> > > +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> > > +int pci_host_bridge_register(struct pci_host_bridge *bridge);
> > >  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> > >  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> > >  void pci_bus_release_busn_res(struct pci_bus *b);
> > 
> > There is too much stuff moving around. I know it is needed and the temptation is to get it
> > done as quickly as possible, but it makes the reviewing and the commenting on this series hard.
> 
> This is why I left the list handling you mentioned above unchanged
> in my original patch instead of rewriting it.
> 
> > Can you split the patch into one that adds the new members
> > into pci_host_bridge, then do the renaming/re-organisation in
> > another patch?
> 
> That should be possible, I guess Thierry can deal with that when respinning
> the series.
> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Friday, July 1, 2016 5:09:23 PM CEST Liviu Dudau wrote:
> > > > 
> > > > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > > > of the registration, and let individual host drivers and architecture
> > > > code fill the members before calling the registration function.
> > > 
> > > That's how we got into the arch/arm mess that we currently have. The host driver
> > > should not drive the instantiation of the pci_host_bridge structure because it
> > > will prevent you from having two drivers running at the same time.
> > 
> > Can you clarify that? I don't see what prevents two drivers from each
> > creating a pci_host_bridge structure and passing it into
> > pci_host_bridge_register().
> 
> Maybe I'm exagerating a bit, but I always disliked the pci_common_init() function and
> the way it is creating a pci_host_bridge while relying on hw_pci hooks to get
> things ready. I'm pretty sure there are still issues around the fact that a lot of
> platform drivers have a subsys_initcall() function that calls pci_common_init(). I
> don't want us to go down that path again.

I see what you mean now, and I agree we don't want to do this like
pci_common_init(), but the patch here does a number of things very
differently:

- it's not architecture specific
- it is not tied into architecture specific pcibios_* functions
- it does not create multiple bridges at once
- it does not have three levels of callbacks going back and forth,
  the idea is to eventually end up with one structure for the
  callback pointers including those that today are done as __weak
  functions.

Today we only have four callers of pci_common_init_dev that probe the
PCI host from DT: cns3xxx, versatile, mvebu and rcar-gen2. I hope
we can replace all of them with the new method here directly and
not take any intermediate steps.

The users of pci_common_init() (not _dev) are limited to board files
on really old platforms that are either not getting updated (ixp, iop,
footbridge, ks8695, mv78xx0, sa1100) or that have DT based replacement
code coming (dove, orion5x, cns3xxx, integrator).

> Don't get me wrong, clearly someone needs to create an instance of pci_host_bridge.
> What I want people to accept is that in the PCI(e) architecture there is nothing that
> stops a piece of HW that used to be the bridge between host and underlying bus into
> becoming the bridge between a higher PCI(e) bus and the bus underneath. If the writes
> to the configuration registers happens somehow, the Host Bridge doesn't even know if
> it is talking to the actual host. Can the driver in that case make sure it did not made
> assumptions that were tied to a specific SoC implementation?

Sorry, I'm not really following what you mean with that, or what the
consequence is for the Linux implementation. Can you try to rephrase this?

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Liviu Dudau]

On Fri, Jul 01, 2016 at 06:30:24PM +0200, Arnd Bergmann wrote:
> On Friday, July 1, 2016 5:09:23 PM CEST Liviu Dudau wrote:
> > > > > 
> > > > > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > > > > of the registration, and let individual host drivers and architecture
> > > > > code fill the members before calling the registration function.
> > > > 
> > > > That's how we got into the arch/arm mess that we currently have. The host driver
> > > > should not drive the instantiation of the pci_host_bridge structure because it
> > > > will prevent you from having two drivers running at the same time.
> > > 
> > > Can you clarify that? I don't see what prevents two drivers from each
> > > creating a pci_host_bridge structure and passing it into
> > > pci_host_bridge_register().
> > 
> > Maybe I'm exagerating a bit, but I always disliked the pci_common_init() function and
> > the way it is creating a pci_host_bridge while relying on hw_pci hooks to get
> > things ready. I'm pretty sure there are still issues around the fact that a lot of
> > platform drivers have a subsys_initcall() function that calls pci_common_init(). I
> > don't want us to go down that path again.
> 
> I see what you mean now, and I agree we don't want to do this like
> pci_common_init(), but the patch here does a number of things very
> differently:
> 
> - it's not architecture specific
> - it is not tied into architecture specific pcibios_* functions
> - it does not create multiple bridges at once
> - it does not have three levels of callbacks going back and forth,
>   the idea is to eventually end up with one structure for the
>   callback pointers including those that today are done as __weak
>   functions.
> 
> Today we only have four callers of pci_common_init_dev that probe the
> PCI host from DT: cns3xxx, versatile, mvebu and rcar-gen2. I hope
> we can replace all of them with the new method here directly and
> not take any intermediate steps.
> 
> The users of pci_common_init() (not _dev) are limited to board files
> on really old platforms that are either not getting updated (ixp, iop,
> footbridge, ks8695, mv78xx0, sa1100) or that have DT based replacement
> code coming (dove, orion5x, cns3xxx, integrator).
> 
> > Don't get me wrong, clearly someone needs to create an instance of pci_host_bridge.
> > What I want people to accept is that in the PCI(e) architecture there is nothing that
> > stops a piece of HW that used to be the bridge between host and underlying bus into
> > becoming the bridge between a higher PCI(e) bus and the bus underneath. If the writes
> > to the configuration registers happens somehow, the Host Bridge doesn't even know if
> > it is talking to the actual host. Can the driver in that case make sure it did not made
> > assumptions that were tied to a specific SoC implementation?
> 
> Sorry, I'm not really following what you mean with that, or what the
> consequence is for the Linux implementation. Can you try to rephrase this?

I'm thinking drivers expecting to drive the bridge between the host and the PCI bus.
When that HW is used to bridge between another bus (PCI or PCIe) because the
functionality was there all the time in terms of signals, the driver's assumptions
break down. But maybe I'm being too theoretical and such beasts don't exist? (I remember
seeing a network card that had native PCI chip plus an added PCIe-to-PCI bridge and
the driver was tripping badly, but I can't remember the exact details.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann]

On Monday, July 4, 2016 10:56:27 AM CEST Liviu Dudau wrote:
> On Fri, Jul 01, 2016 at 06:30:24PM +0200, Arnd Bergmann wrote:
> > On Friday, July 1, 2016 5:09:23 PM CEST Liviu Dudau wrote:

> > > Don't get me wrong, clearly someone needs to create an instance of pci_host_bridge.
> > > What I want people to accept is that in the PCI(e) architecture there is nothing that
> > > stops a piece of HW that used to be the bridge between host and underlying bus into
> > > becoming the bridge between a higher PCI(e) bus and the bus underneath. If the writes
> > > to the configuration registers happens somehow, the Host Bridge doesn't even know if
> > > it is talking to the actual host. Can the driver in that case make sure it did not made
> > > assumptions that were tied to a specific SoC implementation?
> > 
> > Sorry, I'm not really following what you mean with that, or what the
> > consequence is for the Linux implementation. Can you try to rephrase this?
> 
> I'm thinking drivers expecting to drive the bridge between the host and the PCI bus.
> When that HW is used to bridge between another bus (PCI or PCIe) because the
> functionality was there all the time in terms of signals, the driver's assumptions
> break down. But maybe I'm being too theoretical and such beasts don't exist? (I remember
> seeing a network card that had native PCI chip plus an added PCIe-to-PCI bridge and
> the driver was tripping badly, but I can't remember the exact details.

I think we should expect all non-host PCI bridges to behave according to the
normal PCI spec and get handled by the PCI core.

There are some handlers for broken bridges in drivers/pci/quirks.c, which
is not nice but there is little alternative, and I don't think that
hooking into the PCI host bridge driver code would help here.

	Arnd

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Bjorn Helgaas]

Hi Thierry and Arnd,

Thanks a lot for pushing this forward.  I would like to have gotten
this into the v4.8 merge window, but I didn't get to it soon enough.

On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.
> 
> However, this time it's more likely to be extensible enough that we
> won't have to do another one again in the future, and we should be
> able to reduce code much more as a result.
> 
> The main idea is to pull the allocation of 'struct pci_host_bridge' out
> of the registration, and let individual host drivers and architecture
> code fill the members before calling the registration function.
> 
> There are a number of things we can do based on this:
> 
> * Use a single memory allocation for the driver-specific structure
>   and the generic PCI host bridge
> * consolidate the contents of driver specific structures by moving
>   them into pci_host_bridge
> * Add a consistent interface for removing a PCI host bridge again
>   when unloading a host driver module
> * Replace the architecture specific __weak pcibios_* functions with
>   callbacks in a pci_host_bridge device
> * Move common boilerplate code from host drivers into the generic
>   function, based on contents of the structure
> * Extend pci_host_bridge with additional members when needed without
>   having to add arguments to pci_scan_*.
> * Move members of struct pci_bus into pci_host_bridge to avoid
>   having lots of identical copies.
> 
> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.

I agree with Arnd that a pci_host_bridge_alloc() would be nice.  The
pointer to alloc_etherdev() was very useful and probably worth
including in the changelog.  It helps a lot if we can copy the style
of some other subsystem, but my head is buried so deep in PCI that I
don't know what they do.

Looking forward to the next iteration.

Bjorn

Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Bjorn Helgaas]

Hi Thierry and Arnd,

Thanks a lot for pushing this forward.  I would like to have gotten
this into the v4.8 merge window, but I didn't get to it soon enough.

On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This patch makes the existing pci_host_bridge structure a proper device
> that is usable by PCI host drivers in a more standard way. In addition
> to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> and pci_create_root_bus interfaces, this unfortunately means having to
> add yet another interface doing basically the same thing, and add some
> extra code in the initial step.
> 
> However, this time it's more likely to be extensible enough that we
> won't have to do another one again in the future, and we should be
> able to reduce code much more as a result.
> 
> The main idea is to pull the allocation of 'struct pci_host_bridge' out
> of the registration, and let individual host drivers and architecture
> code fill the members before calling the registration function.
> 
> There are a number of things we can do based on this:
> 
> * Use a single memory allocation for the driver-specific structure
>   and the generic PCI host bridge
> * consolidate the contents of driver specific structures by moving
>   them into pci_host_bridge
> * Add a consistent interface for removing a PCI host bridge again
>   when unloading a host driver module
> * Replace the architecture specific __weak pcibios_* functions with
>   callbacks in a pci_host_bridge device
> * Move common boilerplate code from host drivers into the generic
>   function, based on contents of the structure
> * Extend pci_host_bridge with additional members when needed without
>   having to add arguments to pci_scan_*.
> * Move members of struct pci_bus into pci_host_bridge to avoid
>   having lots of identical copies.
> 
> As mentioned in a previous email, one open question is whether we want
> to export a function for allocating a pci_host_bridge device in
> combination with the per-device structure or let the driver itself
> call kzalloc.

I agree with Arnd that a pci_host_bridge_alloc() would be nice.  The
pointer to alloc_etherdev() was very useful and probably worth
including in the changelog.  It helps a lot if we can copy the style
of some other subsystem, but my head is buried so deep in PCI that I
don't know what they do.

Looking forward to the next iteration.

Bjorn