| Message ID | 20200303091254.22373-1-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/cubieboard: make sure SOC object isn't leaked | expand |
Hi Igor, On Tue, Mar 3, 2020 at 10:13 AM Igor Mammedov <imammedo@redhat.com> wrote: > SOC object returned by object_new() is leaked in current code. > Set SOC parent explicitly to board and then unref to SOC object > to make sure that refererence returned by object_new() is taken > care of. > > The SOC object will be kept alive by its parent (machine) and > will be automatically freed when MachineState is destroyed. > Thanks for reporting this issue. In the Allwinner H3 series, I actually inherited this pattern as well, so I'll include this fix as well in the next version of that series. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Andrew Jones <drjones@redhat.com> > --- > hw/arm/cubieboard.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > index 089f9a30c1..12f8ac798d 100644 > --- a/hw/arm/cubieboard.c > +++ b/hw/arm/cubieboard.c > @@ -30,9 +30,14 @@ static struct arm_boot_info cubieboard_binfo = { > > static void cubieboard_init(MachineState *machine) > { > - AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > + AwA10State *a10; > Error *err = NULL; > > + a10 = AW_A10(object_new(TYPE_AW_A10)); > + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), > + &error_abort); > + object_unref(OBJECT(a10)); > I see that there are a few machines which also do object_property_add_child() to add its SoC object to the machine, but they do not do the object_unref(). Can you explain why it is needed here? Or do the other machines still have a leak due to the missing object_unref()? Examples are: hw/arm/sabrelite.c hw/arm/mcimx7d-sabre.c hw/arm/mcimx6ul-evk.c Regards, Niek > + > object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); > if (err != NULL) { > error_reportf_err(err, "Couldn't set phy address: "); > -- > 2.18.1 > > >
On Thu, 5 Mar 2020 21:07:08 +0100 Niek Linnenbank <nieklinnenbank@gmail.com> wrote: > Hi Igor, > [...] > > + a10 = AW_A10(object_new(TYPE_AW_A10)); > > + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), > > + &error_abort); > > + object_unref(OBJECT(a10)); > > > > I see that there are a few machines which also do > object_property_add_child() to add its > SoC object to the machine, but they do not do the object_unref(). Can you > explain why it is needed here? object_new() returns object with ref == 1, and it's responsibility of the caller to take care of it. If caller won't track and call object_unref() eventually on returned pointer, the object will leak. (object_unref() destroys object when 'ref' reaches 0 ) > Or do the other machines still have a leak due to the missing > object_unref()? Examples are: > hw/arm/sabrelite.c > hw/arm/mcimx7d-sabre.c > hw/arm/mcimx6ul-evk.c > > Regards, > Niek > > > > + > > object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); > > if (err != NULL) { > > error_reportf_err(err, "Couldn't set phy address: "); > > -- > > 2.18.1 > > > > > > >
On 3/3/20 10:12 AM, Igor Mammedov wrote: > SOC object returned by object_new() is leaked in current code. > Set SOC parent explicitly to board and then unref to SOC object > to make sure that refererence returned by object_new() is taken > care of. > > The SOC object will be kept alive by its parent (machine) and > will be automatically freed when MachineState is destroyed. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Andrew Jones <drjones@redhat.com> > --- > hw/arm/cubieboard.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > index 089f9a30c1..12f8ac798d 100644 > --- a/hw/arm/cubieboard.c > +++ b/hw/arm/cubieboard.c > @@ -30,9 +30,14 @@ static struct arm_boot_info cubieboard_binfo = { > > static void cubieboard_init(MachineState *machine) > { > - AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > + AwA10State *a10; > Error *err = NULL; > What about using object_new_with_props similarly to commit e4c81e3a451f? a10 = AW_A10(object_new_with_props(TYPE_AW_A10, OBJECT(s), "soc", &err, NULL)); if (err != NULL) { error_propagate(errp, err); return; } > + a10 = AW_A10(object_new(TYPE_AW_A10)); > + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), > + &error_abort); > + object_unref(OBJECT(a10)); > + > object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); > if (err != NULL) { > error_reportf_err(err, "Couldn't set phy address: "); >
On Fri, 6 Mar 2020 11:33:07 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/3/20 10:12 AM, Igor Mammedov wrote: > > SOC object returned by object_new() is leaked in current code. > > Set SOC parent explicitly to board and then unref to SOC object > > to make sure that refererence returned by object_new() is taken > > care of. > > > > The SOC object will be kept alive by its parent (machine) and > > will be automatically freed when MachineState is destroyed. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reported-by: Andrew Jones <drjones@redhat.com> > > --- > > hw/arm/cubieboard.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > > index 089f9a30c1..12f8ac798d 100644 > > --- a/hw/arm/cubieboard.c > > +++ b/hw/arm/cubieboard.c > > @@ -30,9 +30,14 @@ static struct arm_boot_info cubieboard_binfo = { > > > > static void cubieboard_init(MachineState *machine) > > { > > - AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > > + AwA10State *a10; > > Error *err = NULL; > > > > What about using object_new_with_props similarly to commit e4c81e3a451f? I prefer explicit object_property_add_child() and would use above only if there are list of properties to feed to it. > a10 = AW_A10(object_new_with_props(TYPE_AW_A10, OBJECT(s), "soc", > &err, NULL)); > if (err != NULL) { > error_propagate(errp, err); > return; > } > > > + a10 = AW_A10(object_new(TYPE_AW_A10)); > > + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), > > + &error_abort); > > + object_unref(OBJECT(a10)); > > + > > object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); > > if (err != NULL) { > > error_reportf_err(err, "Couldn't set phy address: "); > > >
Hi Igor, On Fri, Mar 6, 2020 at 11:49 AM Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 6 Mar 2020 11:33:07 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 3/3/20 10:12 AM, Igor Mammedov wrote: > > > SOC object returned by object_new() is leaked in current code. > > > Set SOC parent explicitly to board and then unref to SOC object > > > to make sure that refererence returned by object_new() is taken > > > care of. > > > > > > The SOC object will be kept alive by its parent (machine) and > > > will be automatically freed when MachineState is destroyed. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reported-by: Andrew Jones <drjones@redhat.com> > > > --- > > > hw/arm/cubieboard.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > > > index 089f9a30c1..12f8ac798d 100644 > > > --- a/hw/arm/cubieboard.c > > > +++ b/hw/arm/cubieboard.c > > > @@ -30,9 +30,14 @@ static struct arm_boot_info cubieboard_binfo = { > > > > > > static void cubieboard_init(MachineState *machine) > > > { > > > - AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > > > + AwA10State *a10; > > > Error *err = NULL; > > > > > > > What about using object_new_with_props similarly to commit e4c81e3a451f? > I prefer explicit object_property_add_child() and would use > above only if there are list of properties to feed to it. > > > > a10 = AW_A10(object_new_with_props(TYPE_AW_A10, OBJECT(s), "soc", > > &err, NULL)); > > if (err != NULL) { > > error_propagate(errp, err); > > return; > > } > > > > > + a10 = AW_A10(object_new(TYPE_AW_A10)); > > > + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), > > > + &error_abort); > > > + object_unref(OBJECT(a10)); > Thanks, I added this same solution to the Allwinner H3 series, and its working fine there. Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com> > > > + > > > object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); > > > if (err != NULL) { > > > error_reportf_err(err, "Couldn't set phy address: "); > > > > > > > >
On Tue, 3 Mar 2020 at 09:13, Igor Mammedov <imammedo@redhat.com> wrote: > > SOC object returned by object_new() is leaked in current code. > Set SOC parent explicitly to board and then unref to SOC object > to make sure that refererence returned by object_new() is taken > care of. > > The SOC object will be kept alive by its parent (machine) and > will be automatically freed when MachineState is destroyed. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Andrew Jones <drjones@redhat.com> > --- > hw/arm/cubieboard.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Applied to target-arm.next, thanks. -- PMM
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c index 089f9a30c1..12f8ac798d 100644 --- a/hw/arm/cubieboard.c +++ b/hw/arm/cubieboard.c @@ -30,9 +30,14 @@ static struct arm_boot_info cubieboard_binfo = { static void cubieboard_init(MachineState *machine) { - AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); + AwA10State *a10; Error *err = NULL; + a10 = AW_A10(object_new(TYPE_AW_A10)); + object_property_add_child(OBJECT(machine), "soc", OBJECT(a10), + &error_abort); + object_unref(OBJECT(a10)); + object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); if (err != NULL) { error_reportf_err(err, "Couldn't set phy address: ");
SOC object returned by object_new() is leaked in current code. Set SOC parent explicitly to board and then unref to SOC object to make sure that refererence returned by object_new() is taken care of. The SOC object will be kept alive by its parent (machine) and will be automatically freed when MachineState is destroyed. Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reported-by: Andrew Jones <drjones@redhat.com> --- hw/arm/cubieboard.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)