Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <address@hidden> wrote:
>
> This patch fix a bug in mac_via where it failed to actually realize devices 
> it was using.
> And move the init codes which inits the mos6522 objects and properties on 
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it 
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the 
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan <address@hidden>
> ---
> Cc: Laurent Vivier <address@hidden>
> Cc: Mark Cave-Ayland <address@hidden>
> ---

> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
>  static void mac_via_realize(DeviceState *dev, Error **errp)
>  {
>      MacVIAState *m = MAC_VIA(dev);
> -    MOS6522State *ms;
>      struct tm tm;
>      int ret;
> +    Error *err = NULL;
>
> -    /* Init VIAs 1 and 2 */
> -    sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1,
> -                          sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +    qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default());
> +    qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default());

Rather than manually setting the parent bus, you can use
sysbus_init_child_obj() instead of object_initialize_child() --
it is a convenience function that does both object_initialize_child()
and qdev_set_parent_bus() for you.

> -    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", 
> &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>
> -    /* Pass through mos6522 output IRQs */
> -    ms = MOS6522(&m->mos6522_via1);
> -    object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> -                              SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort);
> -    ms = MOS6522(&m->mos6522_via2);
> -    object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> -                              SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort);
> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", 
> &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>
>      /* Pass through mos6522 input IRQs */
>      qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      MacVIAState *m = MAC_VIA(obj);
> +    MOS6522State *ms;
>
>      /* MMIO */
>      memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
>      /* ADB */
>      qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus),
>                          TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +    /* Init VIAs 1 and 2 */
> +    object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1,
> +                            sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
> +                            &error_abort, NULL);
> +    object_initialize_child(OBJECT(m), "via2", &m->mos6522_via2,
> +                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
> +                            &error_abort, NULL);
> +
> +    /* Pass through mos6522 output IRQs */
> +    ms = MOS6522(&m->mos6522_via1);
> +    object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
> +                              SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort);

There's no point in using the MOS6522() cast to get a MOS6522*,
because the only thing you do with it is immediately cast it
to an Object* with the OBJECT() cast. Just use the OBJECT cast directly.

> +    ms = MOS6522(&m->mos6522_via2);
> +    object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
> +                              SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort);
>  }
>
>  static void postload_update_cb(void *opaque, int running, RunState state)

thanks
-- PMM