Patchwork [v3,06/12] qdev: move deletion of children from finalize to unparent

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 25, 2013, 11:46 a.m.
Message ID <1359114420-16149-7-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/215663/
State New
Headers show

Comments

Paolo Bonzini - Jan. 25, 2013, 11:46 a.m.
A device will never be finalized as long as it has a reference from
other devices that sit on its buses.  To ensure that the references
go away, deassociate a bus from its children in the unparent callback
for the bus.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)
Andreas Färber - Jan. 25, 2013, 1:14 p.m.
Am 25.01.2013 12:46, schrieb Paolo Bonzini:
> A device will never be finalized as long as it has a reference from
> other devices that sit on its buses.  To ensure that the references
> go away, deassociate a bus from its children in the unparent callback
> for the bus.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

To help me and others understand this, the children often have
back-references to the bus blocking the bus' finalization? Is that what
you're tackling here?

Remind me, when exactly would the unparent hook being called in case of
a PCI host bridge? prep_pci create a PCI dev to represent itself, 1 ref,
and puts it on the bus. At some point, the bus is unparented (removed
from, e.g., /machine/unassigned, I interpret) and with this patch the
children are unparented and unref'ed. That would still leave the
original object_initialize() ref behind, so that I would still need to
unref it in a finalizefn, no?

Regards,
Andreas

> ---
>  hw/qdev.c | 37 +++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 78eedf0..364386d 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -433,6 +433,25 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
>      }
>  }
>  
> +static void bus_unparent(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    BusChild *kid;
> +
> +    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
> +        DeviceState *dev = kid->child;
> +        qdev_free(dev);
> +    }
> +    if (bus->parent) {
> +        QLIST_REMOVE(bus, sibling);
> +        bus->parent->num_child_bus--;
> +        bus->parent = NULL;
> +    } else {
> +        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
> +        qemu_unregister_reset(qbus_reset_all_fn, bus);
> +    }
> +}
> +
>  void qbus_create_inplace(BusState *bus, const char *typename,
>                           DeviceState *parent, const char *name)
>  {
> @@ -805,22 +824,15 @@ static void qbus_initfn(Object *obj)
>      QTAILQ_INIT(&bus->children);
>  }
>  
> +static void bus_class_init(ObjectClass *class, void *data)
> +{
> +    class->unparent = bus_unparent;
> +}
> +
>  static void qbus_finalize(Object *obj)
>  {
>      BusState *bus = BUS(obj);
> -    BusChild *kid;
>  
> -    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
> -        DeviceState *dev = kid->child;
> -        qdev_free(dev);
> -    }
> -    if (bus->parent) {
> -        QLIST_REMOVE(bus, sibling);
> -        bus->parent->num_child_bus--;
> -    } else {
> -        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
> -        qemu_unregister_reset(qbus_reset_all_fn, bus);
> -    }
>      g_free((char *)bus->name);
>  }
>  
> @@ -832,6 +844,7 @@ static const TypeInfo bus_info = {
>      .class_size = sizeof(BusClass),
>      .instance_init = qbus_initfn,
>      .instance_finalize = qbus_finalize,
> +    .class_init = bus_class_init,
>  };
>  
>  static void qdev_register_types(void)
>

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 78eedf0..364386d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -433,6 +433,25 @@  static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
     }
 }
 
+static void bus_unparent(Object *obj)
+{
+    BusState *bus = BUS(obj);
+    BusChild *kid;
+
+    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
+        DeviceState *dev = kid->child;
+        qdev_free(dev);
+    }
+    if (bus->parent) {
+        QLIST_REMOVE(bus, sibling);
+        bus->parent->num_child_bus--;
+        bus->parent = NULL;
+    } else {
+        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
+        qemu_unregister_reset(qbus_reset_all_fn, bus);
+    }
+}
+
 void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name)
 {
@@ -805,22 +824,15 @@  static void qbus_initfn(Object *obj)
     QTAILQ_INIT(&bus->children);
 }
 
+static void bus_class_init(ObjectClass *class, void *data)
+{
+    class->unparent = bus_unparent;
+}
+
 static void qbus_finalize(Object *obj)
 {
     BusState *bus = BUS(obj);
-    BusChild *kid;
 
-    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
-        DeviceState *dev = kid->child;
-        qdev_free(dev);
-    }
-    if (bus->parent) {
-        QLIST_REMOVE(bus, sibling);
-        bus->parent->num_child_bus--;
-    } else {
-        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
-        qemu_unregister_reset(qbus_reset_all_fn, bus);
-    }
     g_free((char *)bus->name);
 }
 
@@ -832,6 +844,7 @@  static const TypeInfo bus_info = {
     .class_size = sizeof(BusClass),
     .instance_init = qbus_initfn,
     .instance_finalize = qbus_finalize,
+    .class_init = bus_class_init,
 };
 
 static void qdev_register_types(void)