* [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
@ 2020-03-03 9:12 Igor Mammedov
2020-03-05 20:07 ` Niek Linnenbank
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Igor Mammedov @ 2020-03-03 9:12 UTC (permalink / raw
To: qemu-devel; +Cc: b.galvani, peter.maydell, drjones, qemu-arm
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-03 9:12 [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked Igor Mammedov
@ 2020-03-05 20:07 ` Niek Linnenbank
2020-03-06 10:06 ` Igor Mammedov
2020-03-06 10:33 ` Philippe Mathieu-Daudé
2020-03-12 12:01 ` Peter Maydell
2 siblings, 1 reply; 7+ messages in thread
From: Niek Linnenbank @ 2020-03-05 20:07 UTC (permalink / raw
To: Igor Mammedov
Cc: Beniamino Galvani, Peter Maydell, drjones, qemu-arm,
QEMU Developers
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-05 20:07 ` Niek Linnenbank
@ 2020-03-06 10:06 ` Igor Mammedov
0 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2020-03-06 10:06 UTC (permalink / raw
To: Niek Linnenbank
Cc: Beniamino Galvani, Peter Maydell, drjones, qemu-arm,
QEMU Developers
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
> >
> >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-03 9:12 [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked Igor Mammedov
2020-03-05 20:07 ` Niek Linnenbank
@ 2020-03-06 10:33 ` Philippe Mathieu-Daudé
2020-03-06 10:48 ` Igor Mammedov
2020-03-12 12:01 ` Peter Maydell
2 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-06 10:33 UTC (permalink / raw
To: Igor Mammedov, qemu-devel; +Cc: b.galvani, peter.maydell, drjones, qemu-arm
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: ");
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-06 10:33 ` Philippe Mathieu-Daudé
@ 2020-03-06 10:48 ` Igor Mammedov
2020-03-10 19:49 ` Niek Linnenbank
0 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2020-03-06 10:48 UTC (permalink / raw
To: Philippe Mathieu-Daudé
Cc: b.galvani, peter.maydell, drjones, qemu-arm, qemu-devel
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: ");
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-06 10:48 ` Igor Mammedov
@ 2020-03-10 19:49 ` Niek Linnenbank
0 siblings, 0 replies; 7+ messages in thread
From: Niek Linnenbank @ 2020-03-10 19:49 UTC (permalink / raw
To: Igor Mammedov
Cc: Peter Maydell, drjones, QEMU Developers, Beniamino Galvani,
qemu-arm, Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked
2020-03-03 9:12 [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked Igor Mammedov
2020-03-05 20:07 ` Niek Linnenbank
2020-03-06 10:33 ` Philippe Mathieu-Daudé
@ 2020-03-12 12:01 ` Peter Maydell
2 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-03-12 12:01 UTC (permalink / raw
To: Igor Mammedov; +Cc: Beniamino Galvani, Andrew Jones, qemu-arm, QEMU Developers
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-12 12:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-03 9:12 [PATCH] hw/arm/cubieboard: make sure SOC object isn't leaked Igor Mammedov
2020-03-05 20:07 ` Niek Linnenbank
2020-03-06 10:06 ` Igor Mammedov
2020-03-06 10:33 ` Philippe Mathieu-Daudé
2020-03-06 10:48 ` Igor Mammedov
2020-03-10 19:49 ` Niek Linnenbank
2020-03-12 12:01 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.