| Message ID | 1461970899-4150603-2-git-send-email-arnd@arndb.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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); -- 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
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 -- 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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --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);
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(-)