by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v2, part 1 5/9] ACPI, PCI: remove unused global list acpi_pci_roots in pci_root.c

On Tuesday, May 14, 2013 12:08:29 AM Jiang Liu wrote:
> Now the global list acpi_pci_roots pci_root.c is useless, remove it.

Well, are patches [1-4/9] needed for that or does it apply without them?

Rafael

> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/pci_root.c | 25 +++----------------------
> include/acpi/acpi_bus.h | 1 -
> 2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index b80e06e..91ddfd6 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -65,10 +65,6 @@ static struct acpi_scan_handler pci_root_handler = {
> .detach = acpi_pci_root_remove,
> };
>
> -/* Lock to protect both acpi_pci_roots lists */
> -static DEFINE_MUTEX(acpi_pci_root_lock);
> -static LIST_HEAD(acpi_pci_roots);
> -
> static DEFINE_MUTEX(osc_lock);
>
> /**
> @@ -423,7 +419,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
> }
> }
>
> - INIT_LIST_HEAD(&root->node);
> root->device = device;
> root->segment = segment & 0xFFFF;
> strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
> @@ -501,10 +496,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
> * TBD: Need PCI interface for enumeration/configuration of roots.
> */
>
> - mutex_lock(&acpi_pci_root_lock);
> - list_add_tail(&root->node, &acpi_pci_roots);
> - mutex_unlock(&acpi_pci_root_lock);
> -
> /*
> * Scan the Root Bridge
> * --------------------
> @@ -518,7 +509,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
> "Bus %04x:%02x not present in PCI namespace\n",
> root->segment, (unsigned int)root->secondary.start);
> result = -ENODEV;
> - goto out_del_root;
> + goto end;
> }
>
> /* ASPM setting */
> @@ -538,20 +529,13 @@ static int acpi_pci_root_add(struct acpi_device *device,
> if (system_state != SYSTEM_BOOTING) {
> pcibios_resource_survey_bus(root->bus);
> pci_assign_unassigned_bus_resources(root->bus);
> - }
> -
> - /* need to after hot-added ioapic is registered */
> - if (system_state != SYSTEM_BOOTING)
> + /* need to after hot-added ioapic is registered */
> pci_enable_bridges(root->bus);
> + }
>
> pci_bus_add_devices(root->bus);
> return 1;
>
> -out_del_root:
> - mutex_lock(&acpi_pci_root_lock);
> - list_del(&root->node);
> - mutex_unlock(&acpi_pci_root_lock);
> -
> end:
> kfree(root);
> return result;
> @@ -568,9 +552,6 @@ static void acpi_pci_root_remove(struct acpi_device *device)
>
> pci_remove_root_bus(root->bus);
>
> - mutex_lock(&acpi_pci_root_lock);
> - list_del(&root->node);
> - mutex_unlock(&acpi_pci_root_lock);
> kfree(root);
> }
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 22ba56e..4eb9a88 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -447,7 +447,6 @@ int register_acpi_bus_type(struct acpi_bus_type *);
> int unregister_acpi_bus_type(struct acpi_bus_type *);
>
> struct acpi_pci_root {
> - struct list_head node;
> struct acpi_device * device;
> struct pci_bus *bus;
> u16 segment;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-05-14 08:29:18

On Mon 20 May 2013 02:35:31 PM CST, Gu Zheng wrote:
>
> 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 <[email protected]>
>> Cc: Yinghai Lu <[email protected]>
>> ---
>> 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
Hi Gu Zheng,
I have checked to code again and didn't find the memory leakage
issue.
Could you please help to give more hints about the leakage?
Thanks!
Gerry

>
>>
>> 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);
>>
>
>