Re: [PATCH v2 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 28 May 2020 at 12:13, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
>> Affects machines mac99 with via=cuda (default) and g3beige.
>>
>> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
>> Affects machine mac99 with via=pmu and via=pmu-adb,
>>
>> 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.
>>
>> These onetwo appear 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 them in cuda_realize() and pmu_realize(),
>> respectively.
>>
>> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/macio/cuda.c | 10 +++++-----
>>  hw/misc/macio/pmu.c  | 10 +++++-----
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..763a785f1a 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -33,6 +33,7 @@
>>  #include "hw/misc/macio/cuda.h"
>>  #include "qemu/timer.h"
>>  #include "sysemu/runstate.h"
>> +#include "qapi/error.h"
>>  #include "qemu/cutils.h"
>>  #include "qemu/log.h"
>>  #include "qemu/module.h"
>> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error 
>> **errp)
>>  {
>>      CUDAState *s = CUDA(dev);
>>      SysBusDevice *sbd;
>> -    MOS6522State *ms;
>> -    DeviceState *d;
>>      struct tm tm;
>>
>> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
>> +                             &error_abort);
>
> Still disagree with barfing on failure when we have a perfectly
> good way to return the failure indication.

My patch is a strict improvement: it fixes a bug, and it does not add
ways to fail (the object_property_set_bool() above can't actually fail).

You're asking for additional improvement.  "One may always ask, and one
may always say no."

Since there is nothing to clean up here, I'll stick in the useless error
handling so we can move on.

If the error handling you ask for involved cleanup, I'd say no.

Incorrect unreachable cleanup is worse than &error_abort.  I'm not going
to waste time on unreachable and untestable error handling unless it's
utterly trivial, and I'm certainly not going to waste time on creating
more elaborate time bombs.  I *am* going to waste time managing
expectations, if I have to :)

I feel I have to now, because I feel I've once again stretched my
employer's (awesomely generous!) patience with me doing work that won't
ever go into any of our products to the limit.