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 <ehabk...@redhat.com> > Inspired-by: Thomas Huth <th...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> > --- > 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