Re: [PATCH 10/24] macio: Bury unwanted "macio-gpio" devices

[Markus Armbruster]

Mark Cave-Ayland <address@hidden> writes:

> On 18/05/2020 06:03, Markus Armbruster wrote:
>
>> These devices go with the "via-pmu" device, which is controlled by
>> property "has-pmu".  macio_newworld_init() creates it unconditionally,
>> because the property has not been set then.  macio_newworld_realize()
>> realizes it only when the property is true.  Works, although it can
>> leave an unrealized device hanging around in the QOM composition tree.
>> Affects machine mac99 with via=cuda (default).
>> 
>> Bury the unwanted device by making macio_newworld_realize() unparent
>> it.  Visible in "info qom-tree":
>> 
>>      /machine (mac99-machine)
>>        [...]
>>        /unattached (container)
>>          /device[9] (macio-newworld)
>>            [...]
>>            /escc-legacy-port[8] (qemu:memory-region)
>>            /escc-legacy-port[9] (qemu:memory-region)
>>            /escc-legacy[0] (qemu:memory-region)
>>     -      /gpio (macio-gpio)
>>     -        /gpio[0] (qemu:memory-region)
>>            /ide[0] (macio-ide)
>>              /ide.0 (IDE)
>>              /pmac-ide[0] (qemu:memory-region)
>> 
>> Cc: Mark Cave-Ayland <address@hidden>
>> Cc: David Gibson <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/misc/macio/macio.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index 3779865ab2..b3dddf8be7 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -368,6 +368,8 @@ static void macio_newworld_realize(PCIDevice *d, Error 
>> **errp)
>>          memory_region_add_subregion(&s->bar, 0x16000,
>>                                      sysbus_mmio_get_region(sysbus_dev, 0));
>>      } else {
>> +        object_unparent(OBJECT(&ns->gpio));
>> +
>>          /* CUDA */
>>          object_initialize_child(OBJECT(s), "cuda", &s->cuda, 
>> sizeof(s->cuda),
>>                                  TYPE_CUDA, &error_abort, NULL);
>
> This one is a little more interesting because it comes back to the previous
> discussions around if you have a device that contains other devices, should 
> you init
> all the children in your container device init, and the realize all your 
> children in
> your container device realize?

You have to initialize them in the container's instance_init method to
make their properties accessible.

You have to realize them in the container's realize method if
realization can fail, or if it has visible side effects.

Many, many places keep initialization and realization together.
Historical reasons, ignorance, laziness, all excusable.

Doing both in realize is safe (I think), but you'll have to refactor
when you need to expose the properties for configuration.  Cleaning that
up proactively feels unnecessary.

Doing both in instance_init necessitates a fragile, non-local
correctness argument around "can't fail" and "doesn't do anything
untoward".  Best avoided, I think.

> If so I guess this patch isn't technically wrong, but it is somewhat 
> misleading given
> that the existing init/realize pattern here is incorrect. Perhaps it should 
> go ahead
> and make everything work the "right way"?

The code being patched here works the nice way: instance_init method
macio_newworld_init() initializes ns->gpio, and realize method
macio_realize_ide() realizes it.  Let's keep it that way.