diff mbox

Fix duplicate device reset

Message ID 1311020546-9769-1-git-send-email-weil@mail.berlios.de
State Superseded
Headers show

Commit Message

Stefan Weil July 18, 2011, 8:22 p.m. UTC
qbus_reset_all_fn was registered twice, so a lot of device reset
functions were also called twice when QEMU started.

It is sufficient to call sysbus_get_default() which will
register qbus_reset_all_fn.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Isaku Yamahata July 19, 2011, 2:39 a.m. UTC | #1
Thank you for addressing this. Similar patches were proposed and
weren't merged unfortunately.

The reason why the qdev_register_reset() in vl.c is to keep the reset order.
The reset for main_system_bus shouldn't registered by qbus_create_inplace().
But the check, bus != main_system_bus, doesn't work as intended because
main_system_bus is NULL in early qdev creation.
So there are possible ways for the fix.

- Don't care the reset order
  your patch +
  remove "if (bus != main_system_bus)" in qbus_create_inplace()

- keep the reset order
  - instantiate main_system_bus early.
    So the check, bus != main_system_bus in qbus_create_inplace(), will work.
  or
  - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow

thanks,

On Mon, Jul 18, 2011 at 10:22:26PM +0200, Stefan Weil wrote:
> qbus_reset_all_fn was registered twice, so a lot of device reset
> functions were also called twice when QEMU started.
> 
> It is sufficient to call sysbus_get_default() which will
> register qbus_reset_all_fn.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  vl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index fcd7395..fb2f6db 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3301,7 +3301,7 @@ int main(int argc, char **argv, char **envp)
>  
>      /* TODO: once all bus devices are qdevified, this should be done
>       * when bus is created by qdev.c */
> -    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
> +    sysbus_get_default();
>      qemu_run_machine_init_done_notifiers();
>  
>      qemu_system_reset(VMRESET_SILENT);
> -- 
> 1.7.2.5
>
Stefan Weil July 19, 2011, 5:56 a.m. UTC | #2
Am 19.07.2011 04:39, schrieb Isaku Yamahata:
> Thank you for addressing this. Similar patches were proposed and
> weren't merged unfortunately.
>
> The reason why the qdev_register_reset() in vl.c is to keep the reset order.
> The reset for main_system_bus shouldn't registered by qbus_create_inplace().
> But the check, bus != main_system_bus, doesn't work as intended because
> main_system_bus is NULL in early qdev creation.
> So there are possible ways for the fix.
>
> - Don't care the reset order
>    your patch +
>    remove "if (bus != main_system_bus)" in qbus_create_inplace()
>
> - keep the reset order
>    - instantiate main_system_bus early.
>      So the check, bus != main_system_bus in qbus_create_inplace(), will work.
>    or
>    - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow
>
> thanks,

Hi,

my patch does not remove sysbus_get_default(),
so the reset order is kept because main_system_bus
is instantiated by this call.

Cheers,
Stefan
Isaku Yamahata July 19, 2011, 7:22 a.m. UTC | #3
On Tue, Jul 19, 2011 at 07:56:41AM +0200, Stefan Weil wrote:
> Am 19.07.2011 04:39, schrieb Isaku Yamahata:
>> Thank you for addressing this. Similar patches were proposed and
>> weren't merged unfortunately.
>>
>> The reason why the qdev_register_reset() in vl.c is to keep the reset order.
>> The reset for main_system_bus shouldn't registered by qbus_create_inplace().
>> But the check, bus != main_system_bus, doesn't work as intended because
>> main_system_bus is NULL in early qdev creation.
>> So there are possible ways for the fix.
>>
>> - Don't care the reset order
>>    your patch +
>>    remove "if (bus != main_system_bus)" in qbus_create_inplace()
>>
>> - keep the reset order
>>    - instantiate main_system_bus early.
>>      So the check, bus != main_system_bus in qbus_create_inplace(), will work.
>>    or
>>    - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow
>>
>> thanks,
>
> Hi,
>
> my patch does not remove sysbus_get_default(),
> so the reset order is kept because main_system_bus
> is instantiated by this call.

Yes, your patch doesn't change the order from the existing code.
I think it's not intended one.
During machine creation, someone may call sysbus_get_default().
So the reset for main_system_bus may not be the lastly registered.

The changeset of 80376c3f tries to keep the reset order, but failed.
That's the issue.
diff mbox

Patch

diff --git a/vl.c b/vl.c
index fcd7395..fb2f6db 100644
--- a/vl.c
+++ b/vl.c
@@ -3301,7 +3301,7 @@  int main(int argc, char **argv, char **envp)
 
     /* TODO: once all bus devices are qdevified, this should be done
      * when bus is created by qdev.c */
-    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
+    sysbus_get_default();
     qemu_run_machine_init_done_notifiers();
 
     qemu_system_reset(VMRESET_SILENT);