Re: [Qemu-arm] [PATCH] hw/arm: Use object_initialize_child for correct reference counting

[Thomas Huth]

On 21/02/2019 19.38, Philippe Mathieu-Daudé wrote:
> As Thomas Huth explained:
> "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:
> 
>  @use_object_initialize_child@
>  identifier parent_obj;
>  expression child;
>  expression propname;
>  expression child_type;
>  expression errp;
>  @@
>  (
>  -    object_initialize(&child, sizeof(child), child_type);
>  -    object_property_add_child(parent_obj, propname, OBJECT(&child), NULL);
>  +    object_initialize_child(parent_obj, propname,  &child, sizeof(child),
>  +                            child_type, &error_abort, NULL);
>  |
>  -    object_initialize(&child, sizeof(child), child_type);
>  -    object_property_add_child(parent_obj, propname, OBJECT(&child), errp);
>  +    object_initialize_child(parent_obj, propname,  &child, sizeof(child),
>  +                            child_type, errp, NULL);
>  )
> 
> and a bit of manual fix-up for overly long lines.
> 
> Suggested-by: Eduardo Habkost <address@hidden>
> Inspired-by: Thomas Huth <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/aspeed_soc.c          | 43 ++++++++++++++++++------------------
>  hw/arm/bcm2835_peripherals.c | 41 +++++++++++++++++-----------------
>  hw/arm/digic.c               |  4 ++--
>  3 files changed, 45 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index a27233d487..81665f2948 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -106,11 +106,11 @@ static void aspeed_soc_init(Object *obj)
>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>      int i;
>  
> -    object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type);
> -    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
> +                            sc->info->cpu_type, &error_abort, NULL);
>  
> -    object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
> -    object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
> +    object_initialize_child(obj, "scu", &s->scu, sizeof(s->scu),
> +                            TYPE_ASPEED_SCU, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
>      qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
>                           sc->info->silicon_rev);
> @@ -121,35 +121,35 @@ static void aspeed_soc_init(Object *obj)
>      object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
>                                "hw-prot-key", &error_abort);
>  
> -    object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
> -    object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
> +    object_initialize_child(obj, "vic", &s->vic, sizeof(s->vic),
> +                            TYPE_ASPEED_VIC, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
>  
> -    object_initialize(&s->timerctrl, sizeof(s->timerctrl), 
> TYPE_ASPEED_TIMER);
> -    object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
> +    object_initialize_child(obj, "timerctrl", &s->timerctrl,
> +                            sizeof(s->timerctrl), TYPE_ASPEED_TIMER,
> +                            &error_abort, NULL);
>      object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
>                                     OBJECT(&s->scu), &error_abort);
>      qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
>  
> -    object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
> -    object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
> +    object_initialize_child(obj, "i2c", &s->i2c, sizeof(s->i2c),
> +                            TYPE_ASPEED_I2C, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
>  
> -    object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);
> -    object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);
> +    object_initialize_child(obj, "fmc", &s->fmc, sizeof(s->fmc),
> +                            sc->info->fmc_typename, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->fmc), sysbus_get_default());
>      object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
>                                &error_abort);
>  
>      for (i = 0; i < sc->info->spis_num; i++) {
> -        object_initialize(&s->spi[i], sizeof(s->spi[i]),
> -                          sc->info->spi_typename[i]);
> -        object_property_add_child(obj, "spi[*]", OBJECT(&s->spi[i]), NULL);
> +        object_initialize_child(obj, "spi[*]", &s->spi[i], sizeof(s->spi[i]),
> +                                sc->info->spi_typename[i], &error_abort, 
> NULL);
>          qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
>      }
>  
> -    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
> -    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
> +    object_initialize_child(obj, "sdmc", &s->sdmc, sizeof(s->sdmc),
> +                            TYPE_ASPEED_SDMC, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>      qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
>                           sc->info->silicon_rev);
> @@ -159,15 +159,16 @@ static void aspeed_soc_init(Object *obj)
>                                "max-ram-size", &error_abort);
>  
>      for (i = 0; i < sc->info->wdts_num; i++) {
> -        object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
> -        object_property_add_child(obj, "wdt[*]", OBJECT(&s->wdt[i]), NULL);
> +        object_initialize_child(obj, "wdt[*]", &s->wdt[i], sizeof(s->wdt[i]),
> +                                TYPE_ASPEED_WDT, &error_abort, NULL);
>          qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default());
>          qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
>                                      sc->info->silicon_rev);
>      }
>  
> -    object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
> -    object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL);
> +    object_initialize_child(obj, "ftgmac100", &s->ftgmac100,
> +                            sizeof(s->ftgmac100), TYPE_FTGMAC100,
> +                            &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->ftgmac100), sysbus_get_default());
>  }
>  
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 6be7660e8c..0355a41d8b 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -41,8 +41,8 @@ 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);
> +    object_initialize_child(obj, "ic", &s->ic, sizeof(s->ic), 
> TYPE_BCM2835_IC,
> +                            &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
>  
>      /* UART0 */
> @@ -51,21 +51,21 @@ static void bcm2835_peripherals_init(Object *obj)
>      qdev_set_parent_bus(DEVICE(s->uart0), sysbus_get_default());
>  
>      /* AUX / UART1 */
> -    object_initialize(&s->aux, sizeof(s->aux), TYPE_BCM2835_AUX);
> -    object_property_add_child(obj, "aux", OBJECT(&s->aux), NULL);
> +    object_initialize_child(obj, "aux", &s->aux, sizeof(s->aux),
> +                            TYPE_BCM2835_AUX, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->aux), sysbus_get_default());
>  
>      /* Mailboxes */
> -    object_initialize(&s->mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
> -    object_property_add_child(obj, "mbox", OBJECT(&s->mboxes), NULL);
> +    object_initialize_child(obj, "mbox", &s->mboxes, sizeof(s->mboxes),
> +                            TYPE_BCM2835_MBOX, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->mboxes), sysbus_get_default());
>  
>      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);
> +    object_initialize_child(obj, "fb", &s->fb, sizeof(s->fb), 
> TYPE_BCM2835_FB,
> +                            &error_abort, NULL);
>      object_property_add_alias(obj, "vcram-size", OBJECT(&s->fb), 
> "vcram-size",
>                                &error_abort);
>      qdev_set_parent_bus(DEVICE(&s->fb), sysbus_get_default());
> @@ -74,8 +74,9 @@ static void bcm2835_peripherals_init(Object *obj)
>                                     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);
> +    object_initialize_child(obj, "property", &s->property,
> +                            sizeof(s->property), TYPE_BCM2835_PROPERTY,
> +                            &error_abort, NULL);
>      object_property_add_alias(obj, "board-rev", OBJECT(&s->property),
>                                "board-rev", &error_abort);
>      qdev_set_parent_bus(DEVICE(&s->property), sysbus_get_default());
> @@ -86,31 +87,31 @@ 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);
> +    object_initialize_child(obj, "rng", &s->rng, sizeof(s->rng),
> +                            TYPE_BCM2835_RNG, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
>  
>      /* Extended Mass Media Controller */
> -    object_initialize(&s->sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
> -    object_property_add_child(obj, "sdhci", OBJECT(&s->sdhci), NULL);
> +    object_initialize_child(obj, "sdhci", &s->sdhci, sizeof(s->sdhci),
> +                            TYPE_SYSBUS_SDHCI, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->sdhci), sysbus_get_default());
>  
>      /* SDHOST */
> -    object_initialize(&s->sdhost, sizeof(s->sdhost), TYPE_BCM2835_SDHOST);
> -    object_property_add_child(obj, "sdhost", OBJECT(&s->sdhost), NULL);
> +    object_initialize_child(obj, "sdhost", &s->sdhost, sizeof(s->sdhost),
> +                            TYPE_BCM2835_SDHOST, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->sdhost), sysbus_get_default());
>  
>      /* DMA Channels */
> -    object_initialize(&s->dma, sizeof(s->dma), TYPE_BCM2835_DMA);
> -    object_property_add_child(obj, "dma", OBJECT(&s->dma), NULL);
> +    object_initialize_child(obj, "dma", &s->dma, sizeof(s->dma),
> +                            TYPE_BCM2835_DMA, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->dma), sysbus_get_default());
>  
>      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);
> +    object_initialize_child(obj, "gpio", &s->gpio, sizeof(s->gpio),
> +                            TYPE_BCM2835_GPIO, &error_abort, NULL);
>      qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
>  
>      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",

Ack so far.

> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index 726abb9b48..14409ef080 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -35,8 +35,8 @@ static void digic_init(Object *obj)
>      DeviceState *dev;
>      int i;
>  
> -    object_initialize(&s->cpu, sizeof(s->cpu), "arm946-" TYPE_ARM_CPU);
> -    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
> +                            "arm946-" TYPE_ARM_CPU, &error_abort, NULL);
>  
>      for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
>  #define DIGIC_TIMER_NAME_MLEN    11
> 

I think digic_init() needs some more manual tweaking. You can see two
more instances of object_initialize() + add_child() later in that
function, just with some code in between. That either needs to be
re-arranged a little bit, or we should add an object_unref() each time
at the end there.

Also there is one more spot in xlnx_zynqmp_create_rpu() in xlnx-zynqmp.c
that should also be fixed, I think.

Eduardo also listed the machine init functions last year:

 https://patchwork.ozlabs.org/patch/943333/#1953608

Do we want to fix them, too? I think it's currently not really necessary
there since we don't clean up machines anyway, but maybe it would still
be a good idea to avoid that people copy-n-paste bad code snippets...
Well, maybe something for a separate patch, at least.

 Thomas