Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"

[Markus Armbruster <armbru@redhat.com>]

Andreas Färber <afaerber@suse.de> writes:

> Am 05.02.2014 09:39, schrieb Markus Armbruster:
>> This reverts commit 7426aa72c36c908a7d0eae3e38568bb0a70de479.
>> 
>> The commit goes into a sensible direction, but it violates qdev design
>> assumptions.  Symptom: "info qtree" crashes for all boards including
>> the device (akita, borzoi, spitz, terrier, tosa, axis-dev88).
>> 
>> Peter Crosthwaite is working on a fix, but it's not trivial.  Revert
>> the flawed patch for now.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  hw/block/nand.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/block/nand.c b/hw/block/nand.c
>> index a871ce0..a0232d1 100644
>> --- a/hw/block/nand.c
>> +++ b/hw/block/nand.c
>> @@ -21,7 +21,7 @@
>>  # include "hw/hw.h"
>>  # include "hw/block/flash.h"
>>  # include "sysemu/blockdev.h"
>> -#include "hw/qdev.h"
>> +# include "hw/sysbus.h"
>>  #include "qemu/error-report.h"
>>  
>>  # define NAND_CMD_READ0		0x00
>> @@ -54,8 +54,7 @@
>>  
>>  typedef struct NANDFlashState NANDFlashState;
>>  struct NANDFlashState {
>> -    DeviceState parent_obj;
>> -
>> +    SysBusDevice busdev;
>
> Negative on calling it busdev again, that surely has nothing to do with
> a crash since it's not being used anywhere in this patch.

I do not believe in messing with clean reverts such as this one.  Any
follow-up cleanup should be a separate patch.  The easiest way to tell
me what cleanup you want could be a patch :)

> I still have not seen a single backtrace of what is going wrong, only
> Paolo saying something about adding to main_system_bus in "the patch".
> Clearly that is not in this patch! Where is that happening and why is
> that so complicated for Peter C. to fix?

My commit message should suffice to reproduce: start a qemu for any of
the listed boards, run "info qtree".  But I'm happy to spell it out:

    $ QEMU_AUDIO_DRV=none ~/work/qemu/bld/arm-softmmu/qemu-system-arm -M akita -nodefaults -display none -monitor stdio -M akita -sd /dev/null -S
    QEMU 1.7.50 monitor - type 'help' for more information
    (qemu) info qtree
    bus: main-system-bus
      type System
      dev: scoop, id ""
        gpio-in 16
        gpio-out 16
        irq 0
        mmio 0000000010800000/0000000000001000
      dev: spitz-keyboard, id ""
        gpio-in 11
        gpio-out 7
        irq 0
      dev: nand, id ""
        manufacturer_id = 236
        chip_id = 241
        drive = <null>
    /work/armbru/qemu/hw/core/sysbus.c:209:sysbus_dev_print: Object 0x7f750ab9d910 is not an instance of type sys-bus-device
    Aborted (core dumped)

Backtrace:

    #0  0x00007fffee698c55 in raise () from /lib64/libc.so.6
    #1  0x00007fffee69a408 in abort () from /lib64/libc.so.6
    #2  0x00005555557bef59 in object_dynamic_cast_assert (obj=0x55555634d910, 
        typename=typename@entry=0x555555943e4d "sys-bus-device", file=file@entry=
        0x55555594a6f0 "/work/armbru/qemu/hw/core/sysbus.c", line=line@entry=209, 
        func=func@entry=0x55555594a940 <__func__.23193> "sysbus_dev_print")
        at /work/armbru/qemu/qom/object.c:484
    #3  0x00005555556b3c16 in sysbus_dev_print (mon=0x55555625fe00, 
        dev=<optimized out>, indent=4) at /work/armbru/qemu/hw/core/sysbus.c:209
    #4  0x00005555557a2e24 in bus_print_dev (indent=4, dev=0x55555634d910, mon=
        0x55555625fe00, bus=<optimized out>)
        at /work/armbru/qemu/qdev-monitor.c:599
    #5  qdev_print (indent=4, dev=0x55555634d910, mon=0x55555625fe00)
        at /work/armbru/qemu/qdev-monitor.c:621
    #6  qbus_print (mon=0x55555625fe00, bus=<optimized out>, indent=2)
        at /work/armbru/qemu/qdev-monitor.c:636
    #7  0x00005555558a8e39 in handle_user_command (mon=mon@entry=0x55555625fe00, 
        cmdline=<optimized out>) at /work/armbru/qemu/monitor.c:4144
    #8  0x00005555558a91cb in monitor_command_cb (opaque=0x55555625fe00, 
        cmdline=<optimized out>, readline_opaque=<optimized out>)
        at /work/armbru/qemu/monitor.c:4761
    #9  0x00005555559276b4 in readline_handle_byte (rs=0x555556273c90, 
        ch=<optimized out>) at /work/armbru/qemu/util/readline.c:371
    ---Type <return> to continue, or q <return> to quit---
    #10 0x00005555558a8f04 in monitor_read (opaque=<optimized out>, 
        buf=<optimized out>, size=<optimized out>)
        at /work/armbru/qemu/monitor.c:4744
    #11 0x00005555557a6eba in qemu_chr_be_write (len=<optimized out>, buf=
        0x7fffffffcbd0 "\r", s=0x55555625e400) at /work/armbru/qemu/qemu-char.c:165
    #12 fd_chr_read (chan=<optimized out>, cond=<optimized out>, opaque=
        0x55555625e400) at /work/armbru/qemu/qemu-char.c:848
    #13 0x00007ffff76f7a55 in g_main_context_dispatch ()
       from /lib64/libglib-2.0.so.0
    #14 0x0000555555775001 in glib_pollfds_poll ()
        at /work/armbru/qemu/main-loop.c:189
    #15 os_host_main_loop_wait (timeout=<optimized out>)
        at /work/armbru/qemu/main-loop.c:234
    #16 main_loop_wait (nonblocking=<optimized out>)
        at /work/armbru/qemu/main-loop.c:483
    #17 0x0000555555601ed1 in main_loop () at /work/armbru/qemu/vl.c:2018
    #18 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
        at /work/armbru/qemu/vl.c:4410

As to why it's complicated for Peter to fix, here's what Peter wrote:
"That series got very big on me with complications.  I think near term
we just proceed with the revert.  Sorry for the delay."  I have no
reason to second-guess him.

Promptly reverting patches that cause regressions when a fix isn't ready
is standard operating procedure.  We can delay a revert a reasonable
amount of time to deliberate what to do, and perhaps find a fix.  We did
that, and then some: four weeks.  We should revert, and try again.
Neither harm nor shame in that.

A cleanup of nand is quite welcome, but there's absolutely no rush.

[...]