* [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting
@ 2019-02-21 18:38 Philippe Mathieu-Daudé
2019-02-22 5:08 ` Thomas Huth
2019-02-22 5:57 ` Markus Armbruster
0 siblings, 2 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-21 18:38 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Cédric Le Goater, Antony Pavlov, Andrew Baumann,
Andrew Jeffery, Joel Stanley, Thomas Huth, Eduardo Habkost,
Peter Maydell, Philippe Mathieu-Daudé,
Philippe Mathieu-Daudé
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 <ehabkost@redhat.com>
Inspired-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@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",
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
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting
2019-02-21 18:38 [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting Philippe Mathieu-Daudé
@ 2019-02-22 5:08 ` Thomas Huth
2019-02-22 5:57 ` Markus Armbruster
1 sibling, 0 replies; 3+ messages in thread
From: Thomas Huth @ 2019-02-22 5:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, qemu-arm
Cc: Cédric Le Goater, Antony Pavlov, Andrew Baumann,
Andrew Jeffery, Joel Stanley, Eduardo Habkost, Peter Maydell,
Philippe Mathieu-Daudé
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 <ehabkost@redhat.com>
> Inspired-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting
2019-02-21 18:38 [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting Philippe Mathieu-Daudé
2019-02-22 5:08 ` Thomas Huth
@ 2019-02-22 5:57 ` Markus Armbruster
1 sibling, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2019-02-22 5:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-arm, Peter Maydell, Thomas Huth, Eduardo Habkost,
Antony Pavlov, Andrew Jeffery, Philippe Mathieu-Daudé,
Andrew Baumann, Joel Stanley, Cédric Le Goater
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 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 <ehabkost@redhat.com>
> Inspired-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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);
This flips from "ignore errors" to "abort on error". Quite probably an
improvement, but should be mentioned and justified in the commit
message.
[...]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-22 5:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-21 18:38 [Qemu-devel] [PATCH] hw/arm: Use object_initialize_child for correct reference counting Philippe Mathieu-Daudé
2019-02-22 5:08 ` Thomas Huth
2019-02-22 5:57 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).