[PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Igor Mammedov]

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;
 
+    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: ");
-- 
2.18.1

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

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
>
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 3152 bytes --]

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Igor Mammedov]

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
> >
> >
> >  
> 

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Philippe Mathieu-Daudé]

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: ");
> 

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Igor Mammedov]

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: ");
> >   
> 

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

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: ");
> > >
> >
>
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 3722 bytes --]

Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked

[Peter Maydell]

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