Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc" - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: BALATON Zoltan <balaton@eik.bme.hu>,
	Markus Armbruster <armbru@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	berrange@redhat.com, ehabkost@redhat.com,
	Magnus Damm <magnus.damm@gmail.com>,
	qemu-devel@nongnu.org,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	qemu-ppc@nongnu.org, pbonzini@redhat.com
Subject: Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"
Date: Mon, 18 May 2020 12:45:57 +0200	[thread overview]
Message-ID: <43c11226-9bbd-103d-4f9c-4b94c5d95f99@redhat.com> (raw)
In-Reply-To: <alpine.BSF.2.22.395.2005181238010.5961@zero.eik.bme.hu>

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);
>>