On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote:
> On 02/03/2020 13:21, Peter Maydell wrote:
>
>> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengy...@huawei.com> wrote:
>>>
>>> There are some memleaks when we call 'device_list_properties'. This patch
>>> move timer_new from init into realize to fix it.
>>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this
>>> move to be valid.
>>>
>>> Reported-by: Euler Robot <euler.ro...@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengy...@huawei.com>
>>> ---
>>> Cc: Laurent Vivier <laur...@vivier.eu>
>>> ---
>>> 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.
>>
>> Hi; this is really fixing two bugs in one patch:
>>
>>> ---
>>> hw/misc/mac_via.c | 5 +++++
>>> hw/misc/mos6522.c | 6 ++++++
>>> 2 files changed, 11 insertions(+)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index b7d0012794..1d72d4ef35 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error
>>> **errp)
>>> sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>>
>>> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>>> + &error_abort);
>>> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>>> + &error_abort);
>>> +
>>> /* Pass through mos6522 output IRQs */
>>> ms = MOS6522(&m->mos6522_via1);
>>> object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>
>> This is fixing a bug in mac_via where it failed to actually
>> realize devices it was using. That's a dependency for the bug
>> you're trying to fix, but it's a separate one and should be
>> in its own patch.
>
> Sigh. Thanks for this - I actually discovered this a little while back and
> have some
> local patches to do the same, but due to lack of time I never managed to tidy
> them up
> for submission.
Hmm, maybe you can take this other changes(fix memleaks) into your local
patches and send it together?
Or If you have no time, I can help to do it about this device. :)
>
>> You also want to pass the error back up to the caller, rather
>> than using error_abort. To do that, at the top of the function:
>>
>> Error *err = NULL;
>>
>> and then here you can do:
>> object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>> &err);
>> if (err) {
>> error_propagate(errp, err);
>> return;
>> }
>>
>> The existing code which inits the mos6522 objects and
>> calls object_property_add_alias on them which is in
>> the mac_via realize function should be moved to the init
>> function. (init should init child objects and set up
>> properties; realize should realize them.)
>
> And I believe at some point I had a patch lying around to do this too...
>
>> This is all fixing the incorrect creation of the mos6522
>> devices in mac_via (which has been wrong since the mac_via
>> was first added in commit 6dca62a0000f9), so that can all
>> be one patch I think.
>>
>>> +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;
>>
>> The changes to hw/misc/mos_6522.c are good, but should be in
>> their own patch.
>
>
> ATB,
>
> Mark.
> .
>
