On 05/14/2013 12:08 AM, Jiang Liu wrote:
> This patch makes PCI host bridge/bus creating and destroying logic
> symmetric by using device_initialize()/device_add()/device_del()/put_device()
> pairs as discussed in thread at:
> http://comments.gmane.org/gmane.linux.kernel.pci/22073
>
> It also fixes a bug in error recovery path in pci_create_root_bus()
> which may kfree() an in-use host bridge object.
>
> Signed-off-by: Jiang Liu <jiang....@huawei.com>
> Cc: Yinghai Lu <ying...@kernel.org>
> ---
> drivers/pci/probe.c | 84
> +++++++++++++++++++++++-----------------------------
> drivers/pci/remove.c | 3 +-
> 2 files changed, 39 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index bc075a3..a2617c2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -451,7 +451,7 @@ void pci_read_bridge_bases(struct pci_bus *child)
> }
> }
>
> -static struct pci_bus * pci_alloc_bus(void)
> +static struct pci_bus *pci_alloc_bus(struct pci_ops *ops, void *sd, int bus)
> {
> struct pci_bus *b;
>
> @@ -464,10 +464,32 @@ static struct pci_bus * pci_alloc_bus(void)
> INIT_LIST_HEAD(&b->resources);
> b->max_bus_speed = PCI_SPEED_UNKNOWN;
> b->cur_bus_speed = PCI_SPEED_UNKNOWN;
> + b->sysdata = sd;
> + b->ops = ops;
> + b->number = bus;
> + b->busn_res.start = bus;
> + b->busn_res.end = 0xff;
> + b->busn_res.flags = IORESOURCE_BUS;
> + b->dev.class = &pcibus_class;
> + dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> + device_initialize(&b->dev);
> }
> +
> return b;
> }
>
> +static void pci_release_host_bridge_dev(struct device *dev)
> +{
> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
> +
> + if (bridge->release_fn)
> + bridge->release_fn(bridge);
> +
> + pci_free_resource_list(&bridge->windows);
> +
> + kfree(bridge);
> +}
> +
> static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> {
> struct pci_host_bridge *bridge;
> @@ -476,6 +498,10 @@ static struct pci_host_bridge
> *pci_alloc_host_bridge(struct pci_bus *b)
> if (bridge) {
> INIT_LIST_HEAD(&bridge->windows);
> bridge->bus = b;
> + bridge->dev.release = pci_release_host_bridge_dev;
> + dev_set_name(&bridge->dev, "pci%04x:%02x",
> + pci_domain_nr(b), b->number);
> + device_initialize(&bridge->dev);
> }
>
> return bridge;
> @@ -628,28 +654,13 @@ static struct pci_bus *pci_alloc_child_bus(struct
> pci_bus *parent,
> /*
> * Allocate a new bus, and inherit stuff from the parent..
> */
> - child = pci_alloc_bus();
> + child = pci_alloc_bus(parent->ops, parent->sysdata, busnr);
> if (!child)
> return NULL;
>
> child->parent = parent;
> - child->ops = parent->ops;
> - child->sysdata = parent->sysdata;
> child->bus_flags = parent->bus_flags;
> -
> - /* initialize some portions of the bus device, but don't register it
> - * now as the parent is not properly set up yet.
> - */
> - child->dev.class = &pcibus_class;
> - dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> -
> - /*
> - * Set up the primary, secondary and subordinate
> - * bus numbers.
> - */
> - child->number = child->busn_res.start = busnr;
> child->primary = parent->busn_res.start;
> - child->busn_res.end = 0xff;
>
> if (!bridge) {
> child->dev.parent = parent->bridge;
> @@ -670,7 +681,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus
> *parent,
> bridge->subordinate = child;
>
> add_dev:
> - ret = device_register(&child->dev);
> + ret = device_add(&child->dev);
> WARN_ON(ret < 0);
>
> pcibios_add_bus(child);
> @@ -1190,18 +1201,6 @@ int pci_cfg_space_size(struct pci_dev *dev)
> return PCI_CFG_SPACE_SIZE;
> }
>
> -static void pci_release_bus_bridge_dev(struct device *dev)
> -{
> - struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
> -
> - if (bridge->release_fn)
> - bridge->release_fn(bridge);
> -
> - pci_free_resource_list(&bridge->windows);
> -
> - kfree(bridge);
> -}
> -
> struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> {
> struct pci_dev *dev;
> @@ -1688,13 +1687,10 @@ struct pci_bus *pci_create_root_bus(struct device
> *parent, int bus,
> char bus_addr[64];
> char *fmt;
>
> - b = pci_alloc_bus();
> + b = pci_alloc_bus(ops, sysdata, bus);
> if (!b)
> return NULL;
>
> - b->sysdata = sysdata;
> - b->ops = ops;
> - b->number = b->busn_res.start = bus;
> b2 = pci_find_bus(pci_domain_nr(b), bus);
> if (b2) {
> /* If we already got to this bus through a different bridge,
> ignore it */
> @@ -1706,27 +1702,22 @@ struct pci_bus *pci_create_root_bus(struct device
> *parent, int bus,
> if (!bridge)
> goto err_out;
>
> - bridge->dev.parent = parent;
> - bridge->dev.release = pci_release_bus_bridge_dev;
> - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> error = pcibios_root_bridge_prepare(bridge);
> if (error)
> goto bridge_dev_reg_err;
>
> - error = device_register(&bridge->dev);
> + bridge->dev.parent = parent;
> + error = device_add(&bridge->dev);
> if (error)
> goto bridge_dev_reg_err;
> +
> b->bridge = get_device(&bridge->dev);
> device_enable_async_suspend(b->bridge);
> pci_set_bus_of_node(b);
> -
> if (!parent)
> set_dev_node(b->bridge, pcibus_to_node(b));
> -
> - b->dev.class = &pcibus_class;
> b->dev.parent = b->bridge;
> - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> - error = device_register(&b->dev);
> + error = device_add(&b->dev);
> if (error)
> goto class_dev_reg_err;
>
> @@ -1769,12 +1760,11 @@ struct pci_bus *pci_create_root_bus(struct device
> *parent, int bus,
> return b;
>
> class_dev_reg_err:
> - put_device(&bridge->dev);
> - device_unregister(&bridge->dev);
> + device_del(&bridge->dev);
> bridge_dev_reg_err:
> - kfree(bridge);
> + put_device(&bridge->dev);
> err_out:
> - kfree(b);
> + put_device(&b->dev);
> return NULL;
> }
Hi Jiang,
I think the changes here may lead to memleak, doesn't it?
Best regards,
Gu
>
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8fc54b7..b0ce875 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -52,7 +52,8 @@ void pci_remove_bus(struct pci_bus *bus)
> up_write(&pci_bus_sem);
> pci_remove_legacy_files(bus);
> pcibios_remove_bus(bus);
> - device_unregister(&bus->dev);
> + device_del(&bus->dev);
> + put_device(&bus->dev);
> }
> EXPORT_SYMBOL(pci_remove_bus);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
