[RFC] experimental pci_register_host API

[RFC] experimental pci_register_host API

[Arnd Bergmann]

As mentioned in another thread, I have tried to come up with
a way to make the PCI host driver registration more flexible
and simpler.

We have actually discussed this multiple times in the past,
but always ended up elsewhere, so this is a proof of concept
work, leaving all the existing interfaces in place, and
adding a way to allocate a pci_host_bridge structure from
a driver and register that after filling out all the interesting
fields.

This is not tested at all, and certainly not meant for
inclusion until the concept has been discussed better.

Please have a look.

        Arnd

[PATCH 1/3] [RFC] pci: add new method for register PCI hosts
[PATCH 2/3] [RFC] pci: host-common: use new pci_register_host
[PATCH 3/3] [RFC] pci: tegra: use new pci_register_host interface

[PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Arnd Bergmann]

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.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/pci/probe.c | 100 ++++++++++++++++++++++++++++++----------------------
 include/linux/pci.h |   7 +++-
 2 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ae7daeb83e21..fe9d9221b11e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -520,19 +520,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 */
@@ -2108,51 +2095,47 @@ 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)
+int pci_register_host(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;
 	pci_bus_assign_domain_nr(b, parent);
-	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);
@@ -2163,7 +2146,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;
@@ -2179,12 +2162,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) {
@@ -2204,16 +2187,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_register_host);
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
@@ -2278,6 +2261,39 @@ 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 = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	int ret;
+
+	if (!bridge)
+		return NULL;
+
+	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_register_host(bridge);
+	if (ret)
+		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)
@@ -2293,12 +2309,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 81f070a47ee7..8bb5dff617a1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -400,10 +400,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,
@@ -812,6 +816,7 @@ 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);
+int pci_register_host(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.7.0

[PATCH 2/3] [RFC] pci: host-common: use new pci_register_host interface

[Arnd Bergmann]

This changes the pci-host-common implementation to call the
new pci_register_host() interface instead of pci_scan_root_bus().

As a result, we get to share the pci_host_bridge structure
as it was originally intended anyway: We ended up having two
copies of pci_host_bridge here because we never got it done
until now. We can also remove the now unused "resources"
list_head, as we add the resources to the pci_host_bridge
directly.

On top of this, further generalizations are possible to shrink
the pci-host-common.c file, in particular requesting the
resources and scanning for child devices can be moved
into pci_register_host().

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/pci/host/pci-host-common.c | 38 ++++++++++++++++++++++++--------------
 drivers/pci/host/pci-host-common.h |  1 -
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
index e9f850f07968..3fc529f0297e 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -26,7 +26,7 @@
 
 static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
 {
-	pci_free_resource_list(&pci->resources);
+	pci_free_resource_list(&pci->host.windows);
 }
 
 static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
@@ -37,12 +37,12 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
 	resource_size_t iobase;
 	struct resource_entry *win;
 
-	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources,
+	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->host.windows,
 					       &iobase);
 	if (err)
 		return err;
 
-	resource_list_for_each_entry(win, &pci->resources) {
+	resource_list_for_each_entry(win, &pci->host.windows) {
 		struct resource *parent, *res = win->res;
 
 		switch (resource_type(res)) {
@@ -130,6 +130,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	return 0;
 }
 
+static void gen_pci_release(struct device *dev)
+{
+	struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev);
+
+	gen_pci_release_of_pci_ranges(pci);
+	kfree(pci);
+}
+
 int pci_host_common_probe(struct platform_device *pdev,
 			  struct gen_pci *pci)
 {
@@ -137,7 +145,7 @@ int pci_host_common_probe(struct platform_device *pdev,
 	const char *type;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct pci_bus *bus, *child;
+	struct pci_bus *child;
 
 	type = of_get_property(np, "device_type", NULL);
 	if (!type || strcmp(type, "pci")) {
@@ -148,8 +156,8 @@ int pci_host_common_probe(struct platform_device *pdev,
 	of_pci_check_probe_only();
 
 	pci->host.dev.parent = dev;
+	pci->host.dev.release = gen_pci_release;
 	INIT_LIST_HEAD(&pci->host.windows);
-	INIT_LIST_HEAD(&pci->resources);
 
 	/* Parse our PCI ranges and request their resources */
 	err = gen_pci_parse_request_of_pci_ranges(pci);
@@ -168,24 +176,26 @@ int pci_host_common_probe(struct platform_device *pdev,
 		pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 
 
-	bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
-				&pci->cfg.ops->ops, pci, &pci->resources);
-	if (!bus) {
-		dev_err(dev, "Scanning rootbus failed");
-		return -ENODEV;
+	pci->host.ops = &pci->cfg.ops->ops;
+	pci->host.sysdata = pci;
+	pci->host.busnr = pci->cfg.bus_range->start;
+	err = pci_register_host(&pci->host);
+	if (!err) {
+		dev_err(dev, "registering host failed");
+		return err;
 	}
 
 	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
 
 	if (!pci_has_flag(PCI_PROBE_ONLY)) {
-		pci_bus_size_bridges(bus);
-		pci_bus_assign_resources(bus);
+		pci_bus_size_bridges(pci->host.bus);
+		pci_bus_assign_resources(pci->host.bus);
 
-		list_for_each_entry(child, &bus->children, node)
+		list_for_each_entry(child, &pci->host.bus->children, node)
 			pcie_bus_configure_settings(child);
 	}
 
-	pci_bus_add_devices(bus);
+	pci_bus_add_devices(pci->host.bus);
 	return 0;
 }
 
diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h
index 09f3fa0a55d7..c89a756420ee 100644
--- a/drivers/pci/host/pci-host-common.h
+++ b/drivers/pci/host/pci-host-common.h
@@ -38,7 +38,6 @@ struct gen_pci_cfg_windows {
 struct gen_pci {
 	struct pci_host_bridge			host;
 	struct gen_pci_cfg_windows		cfg;
-	struct list_head			resources;
 };
 
 int pci_host_common_probe(struct platform_device *pdev,
-- 
2.7.0

[PATCH 3/3] [RFC] pci: tegra: use new pci_register_host interface

[Arnd Bergmann]

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>
---
 drivers/pci/host/pci-tegra.c | 67 ++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c388468c202a..ece4f5d0180b 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -265,6 +265,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;
@@ -615,14 +616,10 @@ 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);
 	int err;
 
-	sys->mem_offset = pcie->offset.mem;
-	sys->io_offset = pcie->offset.io;
-
 	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->io);
 	if (err < 0)
 		return err;
@@ -639,15 +636,15 @@ static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 	if (err)
 		return err;
 
-	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(&pcie->bridge.windows, &pcie->pio, pcie->offset.io);
+	pci_add_resource_offset(&pcie->bridge.windows, &pcie->mem, pcie->offset.mem);
+	pci_add_resource_offset(&pcie->bridge.windows, &pcie->prefetch,
+				pcie->offset.mem);
+	pci_add_resource(&pcie->bridge.windows, &pcie->busn);
 
 	pci_ioremap_io(pcie->pio.start, pcie->io.start);
 
-	return 1;
+	return 0;
 }
 
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
@@ -1535,6 +1532,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);
 
@@ -2036,10 +2034,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",
@@ -2055,22 +2052,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 = {
@@ -2230,6 +2211,8 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	struct tegra_pcie *pcie;
+	struct pci_host_bridge *bridge;
+	struct pci_bus *child;
 	int err;
 
 	match = of_match_device(tegra_pcie_of_match, &pdev->dev);
@@ -2244,6 +2227,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&pcie->ports);
 	pcie->soc_data = match->data;
 	pcie->dev = &pdev->dev;
+	bridge = &pcie->bridge;
 
 	err = tegra_pcie_parse_dt(pcie);
 	if (err < 0)
@@ -2261,6 +2245,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);
 
@@ -2274,12 +2262,29 @@ 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->sysdata = pcie;
+	bridge->ops = &tegra_pcie_ops;
+	err = pci_register_host(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_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.7.0

Re: [RFC] experimental pci_register_host API

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On Sat, Apr 30, 2016 at 01:01:36AM +0200, Arnd Bergmann wrote:
> As mentioned in another thread, I have tried to come up with
> a way to make the PCI host driver registration more flexible
> and simpler.
> 
> We have actually discussed this multiple times in the past,
> but always ended up elsewhere, so this is a proof of concept
> work, leaving all the existing interfaces in place, and
> adding a way to allocate a pci_host_bridge structure from
> a driver and register that after filling out all the interesting
> fields.
> 
> This is not tested at all, and certainly not meant for
> inclusion until the concept has been discussed better.
> 
> Please have a look.

Hi Arnd,

I like this idea very much and welcome the timing. After the longer than
expected XUSB detour I was going to look at PCI next. Making the driver
work on both 32-bit and 64-bit ARM is the first thing that needs solving
but once that's in place it should be fairly trivial to add support for
Tegra X1 on top.

From a quick glance I think this series is a really good starting point.
I had a couple of questions, but I'll ask them as replies to each patch
for context.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7405 bytes --]

On Sat, Apr 30, 2016 at 01:01:37AM +0200, Arnd Bergmann wrote:
> 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 think the most common pattern in other parts of the kernel is the
latter. That gives drivers the most flexibility to do whatever they
want or need.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/pci/probe.c | 100 ++++++++++++++++++++++++++++++----------------------
>  include/linux/pci.h |   7 +++-
>  2 files changed, 63 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ae7daeb83e21..fe9d9221b11e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -520,19 +520,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 */
> @@ -2108,51 +2095,47 @@ 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)
> +int pci_register_host(struct pci_host_bridge *bridge)

Perhaps pci_register_host_bridge() to mirror the structure name in the
registration function?

>  {
>  	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 */

Might be worth mentioning why we move the resources off the list.

> +	list_splice_init(&bridge->windows, &resources);
> +	b->sysdata = bridge->sysdata;

Does the sysdata not become effectively obsolete after this series? My
understanding is that it's primarily used to store driver-specific data
along with a PCI bus, but if drivers can embed struct pci_host_bridge
they can simply upcast bus->bridge. I do notice that bus->bridge is
currently a struct device *, perhaps we can replace it by a back pointer
to the parent struct pci_host_bridge, which would have to gain a struct
device *parent to point at the device that instantiated the bridge. This
is becoming somewhat complicated, but maybe that can be simplified at
some point.

> +	b->msi = bridge->msi;
> +	b->ops = bridge->ops;
> +	b->number = b->busn_res.start = bridge->busnr;
>  	pci_bus_assign_domain_nr(b, parent);
> -	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);

I'm not sure I understand why we continue after failing to register the
device. Is the usage model here that drivers set up bridge->dev with an
initial set of values here, such as what the bridge->dev.parent is? One
complication I can imagine with that is that drivers would need to have
an implementation for the bridge device's ->release() callback. Perhaps
this could be simplified by having a default release callback (maybe
set up by pci_register_host() if none was specified by the driver) that
calls a callback in struct pci_host_bridge which gets passed a struct
pci_host_bridge. I think that would make usage more uniform from the
driver perspective.

On a side-note, perhaps it would be worth adding a structure that
carries host bridge operations (struct pci_host_bridge_ops)?

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 81f070a47ee7..8bb5dff617a1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -400,10 +400,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;

While at it, is there any reason why this can't be made unsigned? I know
this must sound pedantic, but whenever I see a signed integer variable I
immediately ask myself what the meaning of negative values would be, and
I can't think of any scenario where this one could possible be negative.
But perhaps I'm missing something?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] [RFC] pci: tegra: use new pci_register_host interface

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On Sat, Apr 30, 2016 at 01:01:39AM +0200, Arnd Bergmann wrote:
> 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.

How's that? I thought pci_add_resource_offset() was exactly what was
registering the I/O resource, using the resource's flags to determine
what type to register. Do we have to use a different API to register an
I/O resource in particular?

Overall this change looks really good. How do you want to proceed with
the series? Would it be helpful if I picked up this patch and submit it
to Bjorn for v4.8, provided that the core changes make it in?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Tomasz Nowicki]

On 04/30/2016 01:01 AM, Arnd Bergmann wrote:
> 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.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/pci/probe.c | 100 ++++++++++++++++++++++++++++++----------------------
>   include/linux/pci.h |   7 +++-
>   2 files changed, 63 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ae7daeb83e21..fe9d9221b11e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -520,19 +520,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 */
> @@ -2108,51 +2095,47 @@ 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)
> +int pci_register_host(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;
>   	pci_bus_assign_domain_nr(b, parent);

Have you considered to move domain assigning out of here? Domain is 
common for every bus under host bridge, hence it could go into the 
pci_host_bridge structure. Then I would vote for extra host bridge 
allocation call which would assign standard things like this.

> -	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);
> @@ -2163,7 +2146,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;
> @@ -2179,12 +2162,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) {
> @@ -2204,16 +2187,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_register_host);
>   
>   int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>   {
> @@ -2278,6 +2261,39 @@ 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 = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	int ret;
> +
> +	if (!bridge)
> +		return NULL;
> +
> +	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_register_host(bridge);
> +	if (ret)
> +		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)
> @@ -2293,12 +2309,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 81f070a47ee7..8bb5dff617a1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -400,10 +400,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,
> @@ -812,6 +816,7 @@ 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);
> +int pci_register_host(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);

Re: [PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Arnd Bergmann]

On Monday 02 May 2016 09:35:41 Tomasz Nowicki wrote:
> > +int pci_register_host(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;
> >       pci_bus_assign_domain_nr(b, parent);
> 
> Have you considered to move domain assigning out of here? Domain is 
> common for every bus under host bridge, hence it could go into the 
> pci_host_bridge structure. Then I would vote for extra host bridge 
> allocation call which would assign standard things like this.
> 

I had not thought of it, but it sounds like a good idea, thanks!

The other members of struct bus that we assign here (sysdata,
msi, and ops) are probably also constant for the bridge (need
to verify this), so ideally we'd move all four out of the
bus and replace them with a pointer to the pci_host_bridge
to avoid walking up the bus hierarchy every time we want to
access them.

	Arnd

Re: [PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Liviu.Dudau]

On Mon, May 02, 2016 at 09:09:43AM +0200, Thierry Reding wrote:
> On Sat, Apr 30, 2016 at 01:01:37AM +0200, Arnd Bergmann wrote:
> > 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 think the most common pattern in other parts of the kernel is the
> latter. That gives drivers the most flexibility to do whatever they
> want or need.
> 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/pci/probe.c | 100 ++++++++++++++++++++++++++++++----------------------
> >  include/linux/pci.h |   7 +++-
> >  2 files changed, 63 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index ae7daeb83e21..fe9d9221b11e 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -520,19 +520,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 */
> > @@ -2108,51 +2095,47 @@ 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)
> > +int pci_register_host(struct pci_host_bridge *bridge)
> 
> Perhaps pci_register_host_bridge() to mirror the structure name in the
> registration function?
> 
> >  {
> >  	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 */
> 
> Might be worth mentioning why we move the resources off the list.
> 
> > +	list_splice_init(&bridge->windows, &resources);
> > +	b->sysdata = bridge->sysdata;
> 
> Does the sysdata not become effectively obsolete after this series? My
> understanding is that it's primarily used to store driver-specific data
> along with a PCI bus, but if drivers can embed struct pci_host_bridge
> they can simply upcast bus->bridge. 

I second that. If we do this change (which is long overdue and I fully support),
let's kill sysdata now. Generic host bridge code doesn't use sysdata on purpose.

Best regards,
Liviu

> I do notice that bus->bridge is
> currently a struct device *, perhaps we can replace it by a back pointer
> to the parent struct pci_host_bridge, which would have to gain a struct
> device *parent to point at the device that instantiated the bridge. This
> is becoming somewhat complicated, but maybe that can be simplified at
> some point.
> 
> > +	b->msi = bridge->msi;
> > +	b->ops = bridge->ops;
> > +	b->number = b->busn_res.start = bridge->busnr;
> >  	pci_bus_assign_domain_nr(b, parent);
> > -	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);
> 
> I'm not sure I understand why we continue after failing to register the
> device. Is the usage model here that drivers set up bridge->dev with an
> initial set of values here, such as what the bridge->dev.parent is? One
> complication I can imagine with that is that drivers would need to have
> an implementation for the bridge device's ->release() callback. Perhaps
> this could be simplified by having a default release callback (maybe
> set up by pci_register_host() if none was specified by the driver) that
> calls a callback in struct pci_host_bridge which gets passed a struct
> pci_host_bridge. I think that would make usage more uniform from the
> driver perspective.
> 
> On a side-note, perhaps it would be worth adding a structure that
> carries host bridge operations (struct pci_host_bridge_ops)?
> 
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 81f070a47ee7..8bb5dff617a1 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -400,10 +400,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;
> 
> While at it, is there any reason why this can't be made unsigned? I know
> this must sound pedantic, but whenever I see a signed integer variable I
> immediately ask myself what the meaning of negative values would be, and
> I can't think of any scenario where this one could possible be negative.
> But perhaps I'm missing something?
> 
> Thierry



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

Re: [PATCH 1/3] [RFC] pci: add new method for register PCI hosts

[Arnd Bergmann]

On Tuesday 03 May 2016 11:04:23 Liviu.Dudau@arm.com wrote:
> > > +   list_splice_init(&bridge->windows, &resources);
> > > +   b->sysdata = bridge->sysdata;
> > 
> > Does the sysdata not become effectively obsolete after this series? My
> > understanding is that it's primarily used to store driver-specific data
> > along with a PCI bus, but if drivers can embed struct pci_host_bridge
> > they can simply upcast bus->bridge. 
> 
> I second that. If we do this change (which is long overdue and I fully support),
> let's kill sysdata now. Generic host bridge code doesn't use sysdata on purpose.

I think we still need it in the intermediate time for any remaining users of
the existing interfaces (pci_scan_bus, pci_scan_root_bus, pci_create_root_bus).

It may take a while until those are all gone, but I agree that it makes sense
to not even set the pointer for any driver we convert to the new interface.

	Arnd

Re: [PATCH 2/3] [RFC] pci: host-common: use new pci_register_host interface

[Bjorn Helgaas]

On Sat, Apr 30, 2016 at 01:01:38AM +0200, Arnd Bergmann wrote:
> This changes the pci-host-common implementation to call the
> new pci_register_host() interface instead of pci_scan_root_bus().
> 
> As a result, we get to share the pci_host_bridge structure
> as it was originally intended anyway: We ended up having two
> copies of pci_host_bridge here because we never got it done
> until now. We can also remove the now unused "resources"
> list_head, as we add the resources to the pci_host_bridge
> directly.
> 
> On top of this, further generalizations are possible to shrink
> the pci-host-common.c file, in particular requesting the
> resources and scanning for child devices can be moved
> into pci_register_host().
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/pci/host/pci-host-common.c | 38 ++++++++++++++++++++++++--------------
>  drivers/pci/host/pci-host-common.h |  1 -
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index e9f850f07968..3fc529f0297e 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
> @@ -26,7 +26,7 @@
>  
>  static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
>  {
> -	pci_free_resource_list(&pci->resources);
> +	pci_free_resource_list(&pci->host.windows);
>  }
>  
>  static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
> @@ -37,12 +37,12 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
>  	resource_size_t iobase;
>  	struct resource_entry *win;
>  
> -	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources,
> +	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->host.windows,
>  					       &iobase);
>  	if (err)
>  		return err;
>  
> -	resource_list_for_each_entry(win, &pci->resources) {
> +	resource_list_for_each_entry(win, &pci->host.windows) {
>  		struct resource *parent, *res = win->res;
>  
>  		switch (resource_type(res)) {
> @@ -130,6 +130,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  	return 0;
>  }
>  
> +static void gen_pci_release(struct device *dev)
> +{
> +	struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev);
> +
> +	gen_pci_release_of_pci_ranges(pci);
> +	kfree(pci);
> +}

I don't really like the fact that the alloc of struct gen_pci is so
far away from the free.  I haven't looked hard enough to figure out if
it's reasonable to put them closer.

>  int pci_host_common_probe(struct platform_device *pdev,
>  			  struct gen_pci *pci)
>  {
> @@ -137,7 +145,7 @@ int pci_host_common_probe(struct platform_device *pdev,
>  	const char *type;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	struct pci_bus *bus, *child;
> +	struct pci_bus *child;
>  
>  	type = of_get_property(np, "device_type", NULL);
>  	if (!type || strcmp(type, "pci")) {
> @@ -148,8 +156,8 @@ int pci_host_common_probe(struct platform_device *pdev,
>  	of_pci_check_probe_only();
>  
>  	pci->host.dev.parent = dev;
> +	pci->host.dev.release = gen_pci_release;
>  	INIT_LIST_HEAD(&pci->host.windows);
> -	INIT_LIST_HEAD(&pci->resources);
>  
>  	/* Parse our PCI ranges and request their resources */
>  	err = gen_pci_parse_request_of_pci_ranges(pci);
> @@ -168,24 +176,26 @@ int pci_host_common_probe(struct platform_device *pdev,
>  		pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
>  
>  
> -	bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
> -				&pci->cfg.ops->ops, pci, &pci->resources);
> -	if (!bus) {
> -		dev_err(dev, "Scanning rootbus failed");
> -		return -ENODEV;
> +	pci->host.ops = &pci->cfg.ops->ops;
> +	pci->host.sysdata = pci;
> +	pci->host.busnr = pci->cfg.bus_range->start;
> +	err = pci_register_host(&pci->host);
> +	if (!err) {
> +		dev_err(dev, "registering host failed");
> +		return err;
>  	}

Where do we actually scan the bus here?  I don't see it in
pci_register_host().

>  	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>  
>  	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> -		pci_bus_size_bridges(bus);
> -		pci_bus_assign_resources(bus);
> +		pci_bus_size_bridges(pci->host.bus);
> +		pci_bus_assign_resources(pci->host.bus);
>  
> -		list_for_each_entry(child, &bus->children, node)
> +		list_for_each_entry(child, &pci->host.bus->children, node)
>  			pcie_bus_configure_settings(child);
>  	}
>  
> -	pci_bus_add_devices(bus);
> +	pci_bus_add_devices(pci->host.bus);
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h
> index 09f3fa0a55d7..c89a756420ee 100644
> --- a/drivers/pci/host/pci-host-common.h
> +++ b/drivers/pci/host/pci-host-common.h
> @@ -38,7 +38,6 @@ struct gen_pci_cfg_windows {
>  struct gen_pci {
>  	struct pci_host_bridge			host;
>  	struct gen_pci_cfg_windows		cfg;
> -	struct list_head			resources;
>  };
>  
>  int pci_host_common_probe(struct platform_device *pdev,
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] [RFC] pci: host-common: use new pci_register_host interface

[Arnd Bergmann]

On Wednesday 04 May 2016 18:14:18 Bjorn Helgaas wrote:
> On Sat, Apr 30, 2016 at 01:01:38AM +0200, Arnd Bergmann wrote:

> >  
> > +static void gen_pci_release(struct device *dev)
> > +{
> > +	struct gen_pci *pci = container_of(dev, struct gen_pci, host.dev);
> > +
> > +	gen_pci_release_of_pci_ranges(pci);
> > +	kfree(pci);
> > +}
> 
> I don't really like the fact that the alloc of struct gen_pci is so
> far away from the free.  I haven't looked hard enough to figure out if
> it's reasonable to put them closer.

It should be easy enough to move the release function next to the
one that does the allocation. If we go the other route of having
a generic pci_host_alloc() function that every host driver has to
call instead of kzalloc(), we can probably drop the need to specify
a release function in each driver.

> > +	err = pci_register_host(&pci->host);
> > +	if (!err) {
> > +		dev_err(dev, "registering host failed");
> > +		return err;
> >  	}
> 
> Where do we actually scan the bus here?  I don't see it in
> pci_register_host().

Ah, you are right, that was a mistake. As I said, I have not
tried running the code. I left this out of pci_register_host()
for compatibility with pci_create_root_bus(), which also doesn't
scan the bus, but then I didn't notice that I effectively remove
the scan during the conversion of this driver.

> >  	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> >  
> >  	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > -		pci_bus_size_bridges(bus);
> > -		pci_bus_assign_resources(bus);
> > +		pci_bus_size_bridges(pci->host.bus);
> > +		pci_bus_assign_resources(pci->host.bus);
> >  
> > -		list_for_each_entry(child, &bus->children, node)
> > +		list_for_each_entry(child, &pci->host.bus->children, node)
> >  			pcie_bus_configure_settings(child);
> >  	}
> >  
> > -	pci_bus_add_devices(bus);
> > +	pci_bus_add_devices(pci->host.bus);
> >  	return 0;

I was actually thinking we could move both the scan and all the code
above into pci_register_host(), based on some flags or other struct members
we assign in the pci_host_bridge structure, with the most common combination
being the default.

I'm still unsure why we need to do the pci_fixup_irqs() instead of
having the normal irq setting do the right thing, but if necessary,
the host driver can set a flag to ask the core to do it, or we could
add an optional function pointer to the of_irq_parse_and_map_pci
function (or a host specific one if needed) to struct pci_ops and
the call pci_common_swizzle with that.

For all the other stuff (size_bridges, assign_resources, configure_settings,
add_devices), I'd say a host driver should not really have to worry about
this unless it needs to do something special inbetween. Of course
we can't do it for the existing pci_scan_root_bus() etc, because the
callers expect it not to be done.

	Arnd