Patchwork Register only one qbus_reset_all_fn() for system bus

login
register
mail settings
Submitter Isaku Yamahata
Date April 2, 2011, 12:12 a.m.
Message ID <20110402001203.GA11748@valinux.co.jp>
Download mbox | patch
Permalink /patch/89371/
State New
Headers show

Comments

Isaku Yamahata - April 2, 2011, 12:12 a.m.
> 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.

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(-)
Dmitry Eremin-Solenikov - April 2, 2011, 2:47 p.m.
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
>
Isaku Yamahata - April 3, 2011, 10:26 a.m.
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.
Dmitry Eremin-Solenikov - April 4, 2011, 11:58 a.m.
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?
Isaku Yamahata - April 4, 2011, 1:54 p.m.
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.

Patch

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);