| Message ID | 1391589562-9010-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 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? Andreas > uint8_t manf_id, chip_id; > uint8_t buswidth; /* in BYTES */ > int size, pages; > @@ -441,7 +440,7 @@ static void nand_class_init(ObjectClass *klass, void *data) > > static const TypeInfo nand_info = { > .name = TYPE_NAND, > - .parent = TYPE_DEVICE, > + .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(NANDFlashState), > .class_init = nand_class_init, > };
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. [...]
On 5 February 2014 12:24, Markus Armbruster <armbru@redhat.com> wrote: > 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. I agree in general here, which is why I'm going to apply this patch. While we're talking about regressions: Paolo, do we have a good fix for the PPC boot regression yet or should we revert the patch which caused that? thanks -- PMM
On 5 February 2014 13:00, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 February 2014 12:24, Markus Armbruster <armbru@redhat.com> wrote: >> 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. > > I agree in general here, which is why I'm going to apply this patch. Following conversation on IRC and Andreas' recent patch: given that the current state of affairs (ie 'info qtest crashes') has been in master for six months, there doesn't seem to be a critically urgent need to act immediately, so I'm going to give it a few days to check that we have a consensus on the best way to deal with this. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 5 February 2014 13:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 5 February 2014 12:24, Markus Armbruster <armbru@redhat.com> wrote: >>> 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. >> >> I agree in general here, which is why I'm going to apply this patch. > > Following conversation on IRC and Andreas' recent patch: > given that the current state of affairs (ie 'info qtest crashes') has been > in master for six months, there doesn't seem to be a critically urgent > need to act immediately, so I'm going to give it a few days to check > that we have a consensus on the best way to deal with this. Waiting a few more days for a fix is fine with me.
Il 05/02/2014 14:00, Peter Maydell ha scritto: > While we're talking about regressions: Paolo, do we have a good > fix for the PPC boot regression yet or should we revert the patch which > caused that? Yes, Antony tested the patch on Xen. I'll send the patch as soon as possible. Paolo
On Wed, Feb 5, 2014 at 6:55 PM, Andreas Färber <afaerber@suse.de> wrote: > 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 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? > Im not going after the info-qtree side fix. I was looking at the proper busification of NAND. There are possible core fixes to restore the info-qtree behaviour without this revert, but even if you do, the information presented by info qtree is bogus, because nand devices to not attach to root system buses. Regards, Peter > Andreas > >> uint8_t manf_id, chip_id; >> uint8_t buswidth; /* in BYTES */ >> int size, pages; >> @@ -441,7 +440,7 @@ static void nand_class_init(ObjectClass *klass, void *data) >> >> static const TypeInfo nand_info = { >> .name = TYPE_NAND, >> - .parent = TYPE_DEVICE, >> + .parent = TYPE_SYS_BUS_DEVICE, >> .instance_size = sizeof(NANDFlashState), >> .class_init = nand_class_init, >> }; > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
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; uint8_t manf_id, chip_id; uint8_t buswidth; /* in BYTES */ int size, pages; @@ -441,7 +440,7 @@ static void nand_class_init(ObjectClass *klass, void *data) static const TypeInfo nand_info = { .name = TYPE_NAND, - .parent = TYPE_DEVICE, + .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(NANDFlashState), .class_init = nand_class_init, };