Re: [PATCH] qbus: register reset handler for qbus whose parent is NULL

On Sun, Dec 19, 2010 at 03:19:22PM +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> > Stefan Weil reported the regression caused by
> > ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> > 
> > > The second regression also occurs with MIPS malta.
> > > Networking no longer works with the default pcnet nic.
> > >
> > > This is caused because the reset function for pcnet is no
> > > longer called during system boot. The result in an invalid
> > > mac address (all zero) and a non-working nic.
> > >
> > > For this second regression I still have no simple solution.
> > > Of course mips_malta.c should be converted to qdev which
> > > would fix both problems (but only for malta system emulation).
> > 
> > The issue is, it is assumed that all qbuses, qdeves are under
> > main_system_bus. But there are qbuses whose parent is NULL. So it
> > is necessary to trigger reset for those qbuses.
> > (On the other hand, if NULL is passed to qdev_create(), its parent bus
> > is main_system_bus.)
> > Ideally those buses should be moved under bus controller
> > device which is qdev. But it's not done yet.
> > So register qbus reset handler for qbus whose parent is NULL.
> > 
> > Reported-by: Stefan Weil <weil@mail.berlios.de>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Tested-by: Stefan Weil <weil@mail.berlios.de>
> 
> So - I think the following on top addresses the issues. Comments?
> Compile-tested only.

And maybe this on top:

On Sun, Dec 19, 2010 at 03:24:32PM +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 19, 2010 at 03:19:22PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> > > Stefan Weil reported the regression caused by
> > > ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> > > 
> > > > The second regression also occurs with MIPS malta.
> > > > Networking no longer works with the default pcnet nic.
> > > >
> > > > This is caused because the reset function for pcnet is no
> > > > longer called during system boot. The result in an invalid
> > > > mac address (all zero) and a non-working nic.
> > > >
> > > > For this second regression I still have no simple solution.
> > > > Of course mips_malta.c should be converted to qdev which
> > > > would fix both problems (but only for malta system emulation).
> > > 
> > > The issue is, it is assumed that all qbuses, qdeves are under
> > > main_system_bus. But there are qbuses whose parent is NULL. So it
> > > is necessary to trigger reset for those qbuses.
> > > (On the other hand, if NULL is passed to qdev_create(), its parent bus
> > > is main_system_bus.)
> > > Ideally those buses should be moved under bus controller
> > > device which is qdev. But it's not done yet.
> > > So register qbus reset handler for qbus whose parent is NULL.
> > > 
> > > Reported-by: Stefan Weil <weil@mail.berlios.de>
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > Tested-by: Stefan Weil <weil@mail.berlios.de>
> > 
> > So - I think the following on top addresses the issues. Comments?
> > Compile-tested only.
> 
> And maybe this on top:

I think main_system_bus is never freed. So assert(bus != main_system_bus)
is appropriate.


> diff --git a/hw/qdev.c b/hw/qdev.c
> index e04bd4c..96aba18 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -782,7 +782,7 @@ void qbus_free(BusState *bus)
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
>          bus->parent->num_child_bus--;
> -    } else {
> +    } else if (bus != main_system_bus) {
>          /* TODO: once all bus devices are qdevified, only reset handler for
>           * main_system_bus should be unregistered here. */
>          qemu_unregister_reset(qbus_reset_all, bus);
>