Re: [Qemu-ppc] [Qemu-devel] [PATCH v2 06/16] hw/arm/bcm2835: Use object_initialize_child for correct ref. counting

[Paolo Bonzini]

On 07/05/19 11:34, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> This patch was generated using the following Coccinelle script
> (with a bit of manual fix-up for overly long lines):
> 
>  @use_object_initialize_child@
>  expression parent_obj;
>  expression child_ptr;
>  expression child_name;
>  expression child_type;
>  expression child_size;
>  expression errp;
>  @@
>  (
>  -   object_initialize(child_ptr, child_size, child_type);
>  +   object_initialize_child(parent_obj, child_name,  child_ptr, child_size,
>  +                           child_type, &error_abort, NULL);
>      ... when != parent_obj
>  -   object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), 
> NULL);
>      ...
>  ?-  object_unref(OBJECT(child_ptr));
>  |
>  -   object_initialize(child_ptr, child_size, child_type);
>  +   object_initialize_child(parent_obj, child_name,  child_ptr, child_size,
>  +                            child_type, errp, NULL);
>      ... when != parent_obj
>  -   object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), 
> errp);
>      ...
>  ?-  object_unref(OBJECT(child_ptr));
>  )
> 
>  @use_sysbus_init_child_obj@
>  expression parent_obj;
>  expression dev;
>  expression child_ptr;
>  expression child_name;
>  expression child_type;
>  expression child_size;
>  expression errp;
>  @@
>  (
>  -   object_initialize_child(parent_obj, child_name, child_ptr, child_size,
>  -                           child_type, errp, NULL);
>  +   sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
>  +                         child_type);
>      ...
>  -   qdev_set_parent_bus(DEVICE(child_ptr), sysbus_get_default());
>  |
>  -   object_initialize_child(parent_obj, child_name, child_ptr, child_size,
>  -                           child_type, errp, NULL);
>  +   sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
>  +                         child_type);
>  -   dev = DEVICE(child_ptr);
>  -   qdev_set_parent_bus(dev, sysbus_get_default());
>  )
> 
> While the object_initialize() function doesn't take an
> 'Error *errp' argument, the object_initialize_child() does.
> Since this code is used when a machine is created (and is not
> yet running), we deliberately choose to use the &error_abort
> argument instead of ignoring errors if an object creation failed.
> This choice also matches when using sysbus_init_child_obj(),
> since its code is:
> 
>   void sysbus_init_child_obj(Object *parent,
>                              const char *childname, void *child,
>                              size_t childsize, const char *childtype)
>   {
>       object_initialize_child(parent, childname, child, childsize,
>                               childtype, &error_abort, NULL);
> 
>       qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
>   }
> 
> Suggested-by: Eduardo Habkost <address@hidden>
> Inspired-by: Thomas Huth <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/bcm2835_peripherals.c | 53 ++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 2931a82a25a..0fb54c7964e 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -41,44 +41,36 @@ static void bcm2835_peripherals_init(Object *obj)
>                         MBOX_CHAN_COUNT << MBOX_AS_CHAN_SHIFT);
>  
>      /* Interrupt Controller */
> -    object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
> -    object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
>  
>      /* UART0 */
> -    object_initialize(&s->uart0, sizeof(s->uart0), TYPE_PL011);
> -    object_property_add_child(obj, "uart0", OBJECT(&s->uart0), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->uart0), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "uart0", &s->uart0, sizeof(s->uart0),
> +                          TYPE_PL011);
>  
>      /* AUX / UART1 */
> -    object_initialize(&s->aux, sizeof(s->aux), TYPE_BCM2835_AUX);
> -    object_property_add_child(obj, "aux", OBJECT(&s->aux), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->aux), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "aux", &s->aux, sizeof(s->aux),
> +                          TYPE_BCM2835_AUX);
>  
>      /* Mailboxes */
> -    object_initialize(&s->mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
> -    object_property_add_child(obj, "mbox", OBJECT(&s->mboxes), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->mboxes), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "mbox", &s->mboxes, sizeof(s->mboxes),
> +                          TYPE_BCM2835_MBOX);
>  
>      object_property_add_const_link(OBJECT(&s->mboxes), "mbox-mr",
>                                     OBJECT(&s->mbox_mr), &error_abort);
>  
>      /* Framebuffer */
> -    object_initialize(&s->fb, sizeof(s->fb), TYPE_BCM2835_FB);
> -    object_property_add_child(obj, "fb", OBJECT(&s->fb), NULL);
> +    sysbus_init_child_obj(obj, "fb", &s->fb, sizeof(s->fb), TYPE_BCM2835_FB);
>      object_property_add_alias(obj, "vcram-size", OBJECT(&s->fb), 
> "vcram-size",
>                                &error_abort);
> -    qdev_set_parent_bus(DEVICE(&s->fb), sysbus_get_default());
>  
>      object_property_add_const_link(OBJECT(&s->fb), "dma-mr",
>                                     OBJECT(&s->gpu_bus_mr), &error_abort);
>  
>      /* Property channel */
> -    object_initialize(&s->property, sizeof(s->property), 
> TYPE_BCM2835_PROPERTY);
> -    object_property_add_child(obj, "property", OBJECT(&s->property), NULL);
> +    sysbus_init_child_obj(obj, "property", &s->property, sizeof(s->property),
> +                          TYPE_BCM2835_PROPERTY);
>      object_property_add_alias(obj, "board-rev", OBJECT(&s->property),
>                                "board-rev", &error_abort);
> -    qdev_set_parent_bus(DEVICE(&s->property), sysbus_get_default());
>  
>      object_property_add_const_link(OBJECT(&s->property), "fb",
>                                     OBJECT(&s->fb), &error_abort);
> @@ -86,32 +78,27 @@ static void bcm2835_peripherals_init(Object *obj)
>                                     OBJECT(&s->gpu_bus_mr), &error_abort);
>  
>      /* Random Number Generator */
> -    object_initialize(&s->rng, sizeof(s->rng), TYPE_BCM2835_RNG);
> -    object_property_add_child(obj, "rng", OBJECT(&s->rng), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "rng", &s->rng, sizeof(s->rng),
> +                          TYPE_BCM2835_RNG);
>  
>      /* Extended Mass Media Controller */
> -    object_initialize(&s->sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
> -    object_property_add_child(obj, "sdhci", OBJECT(&s->sdhci), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->sdhci), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "sdhci", &s->sdhci, sizeof(s->sdhci),
> +                          TYPE_SYSBUS_SDHCI);
>  
>      /* SDHOST */
> -    object_initialize(&s->sdhost, sizeof(s->sdhost), TYPE_BCM2835_SDHOST);
> -    object_property_add_child(obj, "sdhost", OBJECT(&s->sdhost), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->sdhost), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "sdhost", &s->sdhost, sizeof(s->sdhost),
> +                          TYPE_BCM2835_SDHOST);
>  
>      /* DMA Channels */
> -    object_initialize(&s->dma, sizeof(s->dma), TYPE_BCM2835_DMA);
> -    object_property_add_child(obj, "dma", OBJECT(&s->dma), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->dma), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "dma", &s->dma, sizeof(s->dma),
> +                          TYPE_BCM2835_DMA);
>  
>      object_property_add_const_link(OBJECT(&s->dma), "dma-mr",
>                                     OBJECT(&s->gpu_bus_mr), &error_abort);
>  
>      /* GPIO */
> -    object_initialize(&s->gpio, sizeof(s->gpio), TYPE_BCM2835_GPIO);
> -    object_property_add_child(obj, "gpio", OBJECT(&s->gpio), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "gpio", &s->gpio, sizeof(s->gpio),
> +                          TYPE_BCM2835_GPIO);
>  
>      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",
>                                     OBJECT(&s->sdhci.sdbus), &error_abort);
> 

Reviewed-by: Paolo Bonzini <address@hidden>