sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
neglect to realize it.  Affects machines sam460ex, shix, r2d, and
fulong2e.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing the
device, yet nobody noticed.

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: 4a1f253adb45ac6019971193d5077c4d5d55886a
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>
---
 hw/display/ati.c   | 1 +
 hw/display/sm501.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 58ec8291d4..7c2177d7b3 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -929,6 +929,7 @@ 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);
+    qdev_init_nofail(DEVICE(i2cddc));
 
     /* 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..132e75b641 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1816,6 +1816,7 @@ 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);
+    qdev_init_nofail(DEVICE(ddc));
 
     /* mmio */
     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
-- 
2.21.1

On Mon, 18 May 2020, 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.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.

No idea why it worked but guests can read EDID info fine with or without 
this patch, so

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Maybe device is created and working after init as it has nothing special 
to do at realize (it doesn't even have a realize method) so all realize 
would do is to link it in qtree?

Regards,
BALATON Zoltan

> 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: 4a1f253adb45ac6019971193d5077c4d5d55886a
> 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>
> ---
> hw/display/ati.c   | 1 +
> hw/display/sm501.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 58ec8291d4..7c2177d7b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,7 @@ 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);
> +    qdev_init_nofail(DEVICE(i2cddc));
>
>     /* 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..132e75b641 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,7 @@ 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);
> +    qdev_init_nofail(DEVICE(ddc));
>
>     /* mmio */
>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 18 May 2020, 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.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing the
>> device, yet nobody noticed.
>
> No idea why it worked but guests can read EDID info fine with or
> without this patch, so
>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Thanks!

> Maybe device is created and working after init as it has nothing
> special to do at realize (it doesn't even have a realize method) so
> all realize would do is to link it in qtree?

Plausible.

On Mon, 18 May 2020, 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.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.
>
> 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: 4a1f253adb45ac6019971193d5077c4d5d55886a

One of these is probably meant to be c82c7336de58876862e6b4dccbda29e9240fd388
although I'm not sure having a Fixes tag makes sense for this commit.

> 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>
> ---
> hw/display/ati.c   | 1 +
> hw/display/sm501.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 58ec8291d4..7c2177d7b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,7 @@ 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);
> +    qdev_init_nofail(DEVICE(i2cddc));
>
>     /* 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..132e75b641 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,7 @@ 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);
> +    qdev_init_nofail(DEVICE(ddc));
>
>     /* mmio */
>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>

On 5/18/20 12:39 PM, BALATON Zoltan wrote:
> On Mon, 18 May 2020, 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.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing the
>> device, yet nobody noticed.
>>
>> 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: 4a1f253adb45ac6019971193d5077c4d5d55886a
> 
> One of these is probably meant to be 
> c82c7336de58876862e6b4dccbda29e9240fd388

:)

> although I'm not sure having a Fixes tag makes sense for this commit.

AFAIK the 'Fixes' tag is not well defined in QEMU.
I personally find it handy to navigate between commits in gitk, not 
having to go via unrelated commits, which is why I use it.
Linux kernel seems to have a stricter approach, only using it for 
security bug fixes. For this QEMU uses 'Cc: qemu-stable'.

Do we need to clarify its use on the wiki?

> 
>> 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>
>> ---
>> hw/display/ati.c   | 1 +
>> hw/display/sm501.c | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 58ec8291d4..7c2177d7b3 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -929,6 +929,7 @@ 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);
>> +    qdev_init_nofail(DEVICE(i2cddc));
>>
>>     /* 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..132e75b641 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -1816,6 +1816,7 @@ 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);
>> +    qdev_init_nofail(DEVICE(ddc));
>>
>>     /* mmio */
>>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", 
>> MMIO_SIZE);
>>

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 5/18/20 12:39 PM, BALATON Zoltan wrote:
>> On Mon, 18 May 2020, 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.
>>>
>>> I wonder how this ever worked.  If the "device becomes real only on
>>> realize" thing actually works, then we've always been missing the
>>> device, yet nobody noticed.
>>>
>>> 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: 4a1f253adb45ac6019971193d5077c4d5d55886a
>>
>> One of these is probably meant to be
>> c82c7336de58876862e6b4dccbda29e9240fd388

Pasto, thanks!

> :)
>
>> although I'm not sure having a Fixes tag makes sense for this commit.
>
> AFAIK the 'Fixes' tag is not well defined in QEMU.

True.

> I personally find it handy to navigate between commits in gitk, not
> having to go via unrelated commits, which is why I use it.
> Linux kernel seems to have a stricter approach, only using it for
> security bug fixes. For this QEMU uses 'Cc: qemu-stable'.

We cc: qemu-stable for show-stoppers without security impact, too.

> Do we need to clarify its use on the wiki?

If we can build rough consensus on how we want it used, yes.