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

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: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

---
Changes v1 -> v2:
- unregister reset handler in qbus_free()
- update the comment
- remove ugly void* cast with wrapper
- assert() in qbus_free() because main_system_bus is never freed
---
 hw/qdev.c |   14 +++++++++++++-
 hw/qdev.h |    2 ++
 vl.c      |    4 +++-
 3 files changed, 18 insertions(+), 2 deletions(-)

On Mon, Dec 20, 2010 at 02:33:35PM +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: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> ---
> Changes v1 -> v2:
> - unregister reset handler in qbus_free()
> - update the comment
> - remove ugly void* cast with wrapper
> - assert() in qbus_free() because main_system_bus is never freed
> ---
>  hw/qdev.c |   14 +++++++++++++-
>  hw/qdev.h |    2 ++
>  vl.c      |    4 +++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6fc9b02..d93a6c4 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -333,6 +333,12 @@ void qbus_reset_all(BusState *bus)
>      qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
> +void qbus_reset_all_fn(void *opaque)
> +{
> +    BusState *bus = opaque;
> +    qbus_reset_all(bus);
> +}
> +
>  /* can be used as ->unplug() callback for the simple cases */
>  int qdev_simple_unplug_cb(DeviceState *dev)
>  {
> @@ -754,8 +760,11 @@ 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) {
> +        /* 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);
>      }
> -
>  }
>  
>  BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
> @@ -778,6 +787,9 @@ void qbus_free(BusState *bus)
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
>          bus->parent->num_child_bus--;
> +    } else {
> +        assert(bus != main_system_bus); /* main_system_bus is never freed */
> +        qemu_unregister_reset(qbus_reset_all_fn, bus);
>      }
>      qemu_free((void*)bus->name);
>      if (bus->qdev_allocated) {
> diff --git a/hw/qdev.h b/hw/qdev.h
> index aaaf55a..b239bb4 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -199,6 +199,8 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
>                         qbus_walkerfn *busfn, void *opaque);
>  void qdev_reset_all(DeviceState *dev);
>  void qbus_reset_all(BusState *bus);

qbus_reset_all is unused now?

> +void qbus_reset_all_fn(void *opaque);
> +


>  void qbus_free(BusState *bus);
>  
>  #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
> diff --git a/vl.c b/vl.c
> index c4d3fc0..8d6bab4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3088,7 +3088,9 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
> +    /* 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());
>      qemu_run_machine_init_done_notifiers();
>  
>      qemu_system_reset();
> -- 
> 1.7.1.1

On Mon, Dec 20, 2010 at 08:35:24AM +0200, Michael S. Tsirkin wrote:
> > diff --git a/hw/qdev.h b/hw/qdev.h
> > index aaaf55a..b239bb4 100644
> > --- a/hw/qdev.h
> > +++ b/hw/qdev.h
> > @@ -199,6 +199,8 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
> >                         qbus_walkerfn *busfn, void *opaque);
> >  void qdev_reset_all(DeviceState *dev);
> >  void qbus_reset_all(BusState *bus);
> 
> qbus_reset_all is unused now?

Yes, right now.
I kept it for consistency and type safety. Maybe a matter of taste.
If you don't like it, it can be dropped.

On Mon, Dec 20, 2010 at 02:33:35PM +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: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>


Applied, thanks!

> ---
> Changes v1 -> v2:
> - unregister reset handler in qbus_free()
> - update the comment
> - remove ugly void* cast with wrapper
> - assert() in qbus_free() because main_system_bus is never freed
> ---
>  hw/qdev.c |   14 +++++++++++++-
>  hw/qdev.h |    2 ++
>  vl.c      |    4 +++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6fc9b02..d93a6c4 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -333,6 +333,12 @@ void qbus_reset_all(BusState *bus)
>      qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
> +void qbus_reset_all_fn(void *opaque)
> +{
> +    BusState *bus = opaque;
> +    qbus_reset_all(bus);
> +}
> +
>  /* can be used as ->unplug() callback for the simple cases */
>  int qdev_simple_unplug_cb(DeviceState *dev)
>  {
> @@ -754,8 +760,11 @@ 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) {
> +        /* 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);
>      }
> -
>  }
>  
>  BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
> @@ -778,6 +787,9 @@ void qbus_free(BusState *bus)
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
>          bus->parent->num_child_bus--;
> +    } else {
> +        assert(bus != main_system_bus); /* main_system_bus is never freed */
> +        qemu_unregister_reset(qbus_reset_all_fn, bus);
>      }
>      qemu_free((void*)bus->name);
>      if (bus->qdev_allocated) {
> diff --git a/hw/qdev.h b/hw/qdev.h
> index aaaf55a..b239bb4 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -199,6 +199,8 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
>                         qbus_walkerfn *busfn, void *opaque);
>  void qdev_reset_all(DeviceState *dev);
>  void qbus_reset_all(BusState *bus);
> +void qbus_reset_all_fn(void *opaque);
> +
>  void qbus_free(BusState *bus);
>  
>  #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
> diff --git a/vl.c b/vl.c
> index c4d3fc0..8d6bab4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3088,7 +3088,9 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
> +    /* 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());
>      qemu_run_machine_init_done_notifiers();
>  
>      qemu_system_reset();
> -- 
> 1.7.1.1