* [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
@ 2014-02-05 8:39 Markus Armbruster
2014-02-05 8:55 ` Andreas Färber
0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2014-02-05 8:39 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, afaerber, pbonzini
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;
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,
};
--
1.8.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 8:39 [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus" Markus Armbruster
@ 2014-02-05 8:55 ` Andreas Färber
2014-02-05 12:24 ` Markus Armbruster
2014-02-05 23:06 ` Peter Crosthwaite
0 siblings, 2 replies; 8+ messages in thread
From: Andreas Färber @ 2014-02-05 8:55 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel, peter.crosthwaite; +Cc: peter.maydell, pbonzini
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,
> };
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 8:55 ` Andreas Färber
@ 2014-02-05 12:24 ` Markus Armbruster
2014-02-05 13:00 ` Peter Maydell
2014-02-05 23:06 ` Peter Crosthwaite
1 sibling, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2014-02-05 12:24 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, peter.crosthwaite, qemu-devel, pbonzini
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.
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 12:24 ` Markus Armbruster
@ 2014-02-05 13:00 ` Peter Maydell
2014-02-05 14:28 ` Peter Maydell
2014-02-05 15:48 ` Paolo Bonzini
0 siblings, 2 replies; 8+ messages in thread
From: Peter Maydell @ 2014-02-05 13:00 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Peter Crosthwaite, Andreas Färber, QEMU Developers
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 13:00 ` Peter Maydell
@ 2014-02-05 14:28 ` Peter Maydell
2014-02-05 15:00 ` Markus Armbruster
2014-02-05 15:48 ` Paolo Bonzini
1 sibling, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2014-02-05 14:28 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Peter Crosthwaite, Andreas Färber, QEMU Developers
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 14:28 ` Peter Maydell
@ 2014-02-05 15:00 ` Markus Armbruster
0 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2014-02-05 15:00 UTC (permalink / raw)
To: Peter Maydell
Cc: Paolo Bonzini, Peter Crosthwaite, Andreas Färber, QEMU Developers
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 13:00 ` Peter Maydell
2014-02-05 14:28 ` Peter Maydell
@ 2014-02-05 15:48 ` Paolo Bonzini
1 sibling, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2014-02-05 15:48 UTC (permalink / raw)
To: Peter Maydell, Markus Armbruster
Cc: Peter Crosthwaite, Andreas Färber, QEMU Developers
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus"
2014-02-05 8:55 ` Andreas Färber
2014-02-05 12:24 ` Markus Armbruster
@ 2014-02-05 23:06 ` Peter Crosthwaite
1 sibling, 0 replies; 8+ messages in thread
From: Peter Crosthwaite @ 2014-02-05 23:06 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Paolo Bonzini, Markus Armbruster,
qemu-devel@nongnu.org Developers
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-05 23:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-05 8:39 [Qemu-devel] [PATCH] Revert "nand: Don't inherit from Sysbus" Markus Armbruster
2014-02-05 8:55 ` Andreas Färber
2014-02-05 12:24 ` Markus Armbruster
2014-02-05 13:00 ` Peter Maydell
2014-02-05 14:28 ` Peter Maydell
2014-02-05 15:00 ` Markus Armbruster
2014-02-05 15:48 ` Paolo Bonzini
2014-02-05 23:06 ` Peter Crosthwaite
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.