| Message ID | 20110402001203.GA11748@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: >> Have you verified that all bus devices have been qdevified since this >> code has been added? I wouldn't bet it is the case. > > I think his analysis is valid. So how about the following patch. Could you please point me to an example of devices for which this check is required. > From ee27041a238d51247e30100d1909066978cd8858 Mon Sep 17 00:00:00 2001 > Message-Id: > <ee27041a238d51247e30100d1909066978cd8858.1301703026.git.yamahata@valinux.co.jp> > From: Isaku Yamahata <yamahata@valinux.co.jp> > Date: Sat, 2 Apr 2011 09:04:54 +0900 > Subject: [PATCH] qdev: Register only one qbus_reset_all_fn() for system bus > > This is the revised version of Dmitry's. > his report is as follows and this patch fixes it. >> Currently reset handler is registered for System bus twice: once during >> bus creation and once in vl.c. Remove the second qemu_register_reset() >> invocation. Also while we are at it, remove incorrect check at >> qbus_create_inplace(): when system bus is created, main_system_bus is >> NULL (as it's not yet created, it cannot be set), so the check is just >> wrong. > > the check bus == main_system_bus in qbus_create_inplace() was wrong > because sysbus_get_default() creates bus for main_system_bus and then > set main_system_bus to it. > So main_system_bus == NULL in qbus_create_inplace() when creating > main_system_bus. So this patch fixes the check whether creating > main_system_bus or not by seeing BusInfo. > > Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/qdev.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/hw/qdev.c b/hw/qdev.c > index 1aa1ea0..659de23 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -762,7 +762,12 @@ void qbus_create_inplace(BusState *bus, BusInfo *info, > if (parent) { > QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling); > parent->num_child_bus++; > - } else if (bus != main_system_bus) { > + } else if (info != &system_bus_info) { > + /* see if bus == main_system_bus > + * Here we can't check bus == main_system_bus because > + * main_system_bus == NULL here before setting it by > + * sysbus_get_default() > + */ > /* TODO: once all bus devices are qdevified, > only reset handler for main_system_bus should be registered > here. */ > qemu_register_reset(qbus_reset_all_fn, bus); > -- > 1.7.1.1 > > > > -- > yamahata >
On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote: > On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: > >> Have you verified that all bus devices have been qdevified since this > >> code has been added? I wouldn't bet it is the case. > > > > I think his analysis is valid. So how about the following patch. > > Could you please point me to an example of devices for which this check is > required. Although I don't have any example, I bet to not change the reset order. If you check all the devices, it's good.
On 4/3/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: > On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote: >> On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: >> >> Have you verified that all bus devices have been qdevified since this >> >> code has been added? I wouldn't bet it is the case. >> > >> > I think his analysis is valid. So how about the following patch. >> >> Could you please point me to an example of devices for which this check is >> required. > > Although I don't have any example, I bet to not change the reset order. > If you check all the devices, it's good. The question is which devices to check as lots of devices are already converted to qdev. Is it correct that we should check only devices which register a child bus with parent device set, and the thing that we should check is the fact that the parent reset function also causes the bus reset?
On Mon, Apr 04, 2011 at 03:58:39PM +0400, Dmitry Eremin-Solenikov wrote: > On 4/3/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: > > On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote: > >> On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote: > >> >> Have you verified that all bus devices have been qdevified since this > >> >> code has been added? I wouldn't bet it is the case. > >> > > >> > I think his analysis is valid. So how about the following patch. > >> > >> Could you please point me to an example of devices for which this check is > >> required. > > > > Although I don't have any example, I bet to not change the reset order. > > If you check all the devices, it's good. > > The question is which devices to check as lots of devices are already > converted to qdev. Is it correct that we should check only devices > which register a child bus with parent device set, and the thing that we > should check is the fact that the parent reset function also causes > the bus reset? qbus whose parent is NULL, non-qdev devices and qdev devices which uses qemu_register_reset() instead of DeviceInfo::reset.
diff --git a/hw/qdev.c b/hw/qdev.c index 1aa1ea0..659de23 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -762,7 +762,12 @@ void qbus_create_inplace(BusState *bus, BusInfo *info, if (parent) { QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling); parent->num_child_bus++; - } else if (bus != main_system_bus) { + } else if (info != &system_bus_info) { + /* see if bus == main_system_bus + * Here we can't check bus == main_system_bus because + * main_system_bus == NULL here before setting it by + * sysbus_get_default() + */ /* TODO: once all bus devices are qdevified, only reset handler for main_system_bus should be registered here. */ qemu_register_reset(qbus_reset_all_fn, bus);