1. Patchew
  2. QEMU
  3. [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.
  4. View patch

[PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

Pan Nengyuan posted 3 patches 5 years, 6 months ago
Maintainers: David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Richard Henderson <rth@twiddle.net>, Laurent Vivier <laurent@vivier.eu>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, David Gibson <david@gibson.dropbear.id.au>
There is a newer version of this series
[PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
Posted by Pan Nengyuan 5 years, 6 months ago 
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
---
v2->v1:
- no changes in this patch.
v3->v2:
- remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
v4->v3:
- split patch into two, this patch fix the memleaks.
---
 hw/misc/mos6522.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..c1cd154a84 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
     for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
         s->timers[i].index = i;
     }
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+    MOS6522State *s = MOS6522(dev);
 
     s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
     s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
 
     dc->reset = mos6522_reset;
     dc->vmsd = &vmstate_mos6522;
+    dc->realize = mos6522_realize;
     device_class_set_props(dc, mos6522_properties);
     mdc->parent_reset = dc->reset;
     mdc->set_sr_int = mos6522_set_sr_int;
-- 
2.18.2
Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
Posted by David Gibson 5 years, 6 months ago 
On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote:
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>

Applied to ppc-for-5.0.

Probably the memory region stuff should be in realize() rather than
init() as well, but that can be fixed later.

> ---
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> ---
> v2->v1:
> - no changes in this patch.
> v3->v2:
> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
> v4->v3:
> - split patch into two, this patch fix the memleaks.
> ---
>  hw/misc/mos6522.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 19e154b870..c1cd154a84 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
>      for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
>          s->timers[i].index = i;
>      }
> +}
> +
> +static void mos6522_realize(DeviceState *dev, Error **errp)
> +{
> +    MOS6522State *s = MOS6522(dev);
>  
>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>  
>      dc->reset = mos6522_reset;
>      dc->vmsd = &vmstate_mos6522;
> +    dc->realize = mos6522_realize;
>      device_class_set_props(dc, mos6522_properties);
>      mdc->parent_reset = dc->reset;
>      mdc->set_sr_int = mos6522_set_sr_int;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
Posted by Pan Nengyuan 5 years, 6 months ago 

On 3/6/2020 6:56 AM, David Gibson wrote:
> On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote:
>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Applied to ppc-for-5.0.

Thanks.

And this patch depend to another fix (patch2/3: https://patchwork.kernel.org/patch/11421229/). Otherwise, it'll be invalid for this move.
I forgot cc it to you, but I think it should let you known.

> 
> Probably the memory region stuff should be in realize() rather than
> init() as well, but that can be fixed later.
> 
>> ---
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: qemu-ppc@nongnu.org
>> ---
>> v2->v1:
>> - no changes in this patch.
>> v3->v2:
>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>> v4->v3:
>> - split patch into two, this patch fix the memleaks.
>> ---
>>  hw/misc/mos6522.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 19e154b870..c1cd154a84 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
>>      for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
>>          s->timers[i].index = i;
>>      }
>> +}
>> +
>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MOS6522State *s = MOS6522(dev);
>>  
>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>>  
>>      dc->reset = mos6522_reset;
>>      dc->vmsd = &vmstate_mos6522;
>> +    dc->realize = mos6522_realize;
>>      device_class_set_props(dc, mos6522_properties);
>>      mdc->parent_reset = dc->reset;
>>      mdc->set_sr_int = mos6522_set_sr_int;
>
Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
Posted by David Gibson 5 years, 6 months ago 
On Fri, Mar 06, 2020 at 09:56:52AM +1100, David Gibson wrote:
> On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote:
> > There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> > 
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Applied to ppc-for-5.0.
> 
> Probably the memory region stuff should be in realize() rather than
> init() as well, but that can be fixed later.

....and removed again.  This causes SEGVs during make
check-qtest-ppc64.

> 
> > ---
> > Cc: Laurent Vivier <laurent@vivier.eu>
> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Cc: David Gibson <david@gibson.dropbear.id.au>
> > Cc: qemu-ppc@nongnu.org
> > ---
> > v2->v1:
> > - no changes in this patch.
> > v3->v2:
> > - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
> > v4->v3:
> > - split patch into two, this patch fix the memleaks.
> > ---
> >  hw/misc/mos6522.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 19e154b870..c1cd154a84 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
> >      for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
> >          s->timers[i].index = i;
> >      }
> > +}
> > +
> > +static void mos6522_realize(DeviceState *dev, Error **errp)
> > +{
> > +    MOS6522State *s = MOS6522(dev);
> >  
> >      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
> >      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> > @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
> >  
> >      dc->reset = mos6522_reset;
> >      dc->vmsd = &vmstate_mos6522;
> > +    dc->realize = mos6522_realize;
> >      device_class_set_props(dc, mos6522_properties);
> >      mdc->parent_reset = dc->reset;
> >      mdc->set_sr_int = mos6522_set_sr_int;
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.