Re: [PATCH v2 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster wrote:
> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
> fulong2e.
> 
> In theory, a device becomes real only on realize.  In practice, the
> transition from unreal to real is a fuzzy one.  The work to make a
> device real can be spread between realize methods (fine),
> instance_init methods (wrong), and board code wiring up the device
> (fine as long as it effectively happens on realize).  Depending on
> what exactly is done where, a device can work even when we neglect
> to realize it.
> 
> This one appears to work.  Nevertheless, it's a clear misuse of the
> interface.  Even when it works today (more or less by chance), it can
> break tomorrow.
> 
> Fix by realizing it right away.  Visible in "info qom-tree"; here's
> the change for sam460ex:
> 
>      /machine (sam460ex-machine)
>        [...]
>        /unattached (container)
>          [...]
>     -    /device[14] (sii3112)
>     +    /device[14] (i2c-ddc)
>     +    /device[15] (sii3112)
>          [rest of device[*] renumbered...]
> 
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Fixes: c82c7336de58876862e6b4dccbda29e9240fd388
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/display/ati.c   | 2 ++
>  hw/display/sm501.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 065f197678..5c71e5f295 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,8 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>      bitbang_i2c_init(&s->bbi2c, i2cbus);
>      I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>      i2c_set_slave_address(i2cddc, 0x50);
> +    object_property_set_bool(OBJECT(i2cddc), true, "realized",
> +                             &error_abort);
>  
>      /* mmio register space */
>      memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..fbedc56715 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,8 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>      /* ddc */
>      I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>      i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +    object_property_set_bool(OBJECT(ddc), true, "realized",
> +                             &error_abort);
>  
>      /* mmio */
>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", 
> MMIO_SIZE);
>