On Mon, 14 Dec 2020 at 16:02, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Fri, 27 Nov 2020 at 07:19, Gan Qixin <ganqi...@huawei.com> wrote:
> >
> > When running device-introspect-test, a memory leak occurred in the
> > a10_pit_init
> > function, so use ptimer_free() in the finalize function to avoid it.
> >
> > ASAN shows memory leak stack:
> >
> > Indirect leak of 288 byte(s) in 6 object(s) allocated from:
> > #0 0xffffab97e1f0 in __interceptor_calloc (/lib64/libasan.so.5+0xee1f0)
> > #1 0xffffab256800 in g_malloc0 (/lib64/libglib-2.0.so.0+0x56800)
> > #2 0xaaabf555db84 in timer_new_full /qemu/include/qemu/timer.h:523
> > #3 0xaaabf555db84 in timer_new /qemu/include/qemu/timer.h:544
> > #4 0xaaabf555db84 in timer_new_ns /qemu/include/qemu/timer.h:562
> > #5 0xaaabf555db84 in ptimer_init /qemu/hw/core/ptimer.c:433
> > #6 0xaaabf57415e8 in a10_pit_init /qemu/hw/timer/allwinner-a10-pit.c:278
> > #7 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515
> > #8 0xaaabf633ca04 in object_initialize_child_with_propsv
> > /qemu/qom/object.c:564
> > #9 0xaaabf633cc08 in object_initialize_child_with_props
> > /qemu/qom/object.c:547
> > #10 0xaaabf5b94680 in aw_a10_init /qemu/hw/arm/allwinner-a10.c:49
> > #11 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515
> > #12 0xaaabf633a1e0 in object_new_with_type /qemu/qom/object.c:729
> >
> > Reported-by: Euler Robot <euler.ro...@huawei.com>
> > Signed-off-by: Gan Qixin <ganqi...@huawei.com>
> > ---
> > Cc: Beniamino Galvani <b.galv...@gmail.com>
> > ---
> > hw/timer/allwinner-a10-pit.c | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index f84fc0ea25..be211983b0 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -279,6 +279,16 @@ static void a10_pit_init(Object *obj)
> > }
> > }
> >
> > +static void a10_pit_finalize(Object *obj)
> > +{
> > + AwA10PITState *s = AW_A10_PIT(obj);
> > + int i;
> > +
> > + for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
> > + ptimer_free(s->timer[i]);
> > + }
> > +}
> > +
> > static void a10_pit_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -290,11 +300,12 @@ static void a10_pit_class_init(ObjectClass *klass,
> > void *data)
> > }
> >
> > static const TypeInfo a10_pit_info = {
> > - .name = TYPE_AW_A10_PIT,
> > - .parent = TYPE_SYS_BUS_DEVICE,
> > - .instance_size = sizeof(AwA10PITState),
> > - .instance_init = a10_pit_init,
> > - .class_init = a10_pit_class_init,
> > + .name = TYPE_AW_A10_PIT,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(AwA10PITState),
> > + .instance_init = a10_pit_init,
> > + .instance_finalize = a10_pit_finalize,
> > + .class_init = a10_pit_class_init,
> > };
>
> Please don't make unrelated whitespace changes like this in a patch.
> We don't line up the assignments in this sort of struct -- this is
> deliberate, so that if a new line is added whose field name happens
> to be longer than those used already, the patch does not have to
> touch all the lines in the struct to maintain the formatting.
> Instead you get a readable diff where only the new line changes,
> not all the others.
Hmm. Having said that I see that the other 6 devices touched
by this series did use the line-up-the-assignments style.
Anyway, the style this device was using is the right one.
thanks
-- PMM
