Re: [Qemu-devel] [PATCH 4/4] qdev: switch reset to post-order

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Dec 06, 2013 at 05:54:27PM +0100, Paolo Bonzini wrote:
> Post-order is the only sensible direction for the reset signals.
> For example, suppose pre-order is used and the parent has some data
> structures that cache children state (for example a list of active
> requests).  When the reset method is invoked on the parent, these caches
> could be in any state.
> 
> If post-order is used, on the other hand, these will be in a known state
> when the reset method is invoked on the parent.
> 
> This change means that it is no longer possible to block the visit of
> the devices, so the callback is changed to return void.  This is not
> a problem, because PCI was returning 1 exactly in order to achieve the
> same ordering that this patch implements.
> 
> PCI can then rely on the qdev core having sent a "reset signal" (whatever
> that means) to the device, and only do the PCI-specific initialization
> with pci_do_device_reset.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/qdev.c         |    6 +++---
>  hw/pci/pci.c           |   31 ++++++++++++++++---------------
>  include/hw/qdev-core.h |    2 +-
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 1c114b7..9ba8ab1 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -233,19 +233,19 @@ static int qbus_reset_one(BusState *bus, void *opaque)
>  {
>      BusClass *bc = BUS_GET_CLASS(bus);
>      if (bc->reset) {
> -        return bc->reset(bus);
> +        bc->reset(bus);
>      }
>      return 0;
>  }
>  
>  void qdev_reset_all(DeviceState *dev)
>  {
> -    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
> +    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
>  void qbus_reset_all(BusState *bus)
>  {
> -    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
> +    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
>  void qbus_reset_all_fn(void *opaque)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0efc544..e10d74b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -46,7 +46,7 @@
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
> -static int pcibus_reset(BusState *qbus);
> +static void pcibus_reset(BusState *qbus);
>  
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> @@ -165,16 +165,10 @@ void pci_device_deassert_intx(PCIDevice *dev)
>      }
>  }
>  
> -/*
> - * This function is called on #RST and FLR.
> - * FLR if PCI_EXP_DEVCTL_BCR_FLR is set
> - */
> -void pci_device_reset(PCIDevice *dev)
> +static void pci_do_device_reset(PCIDevice *dev)
>  {
>      int r;
>  
> -    qdev_reset_all(&dev->qdev);
> -
>      dev->irq_state = 0;
>      pci_update_irq_status(dev);
>      pci_device_deassert_intx(dev);
> @@ -207,27 +201,34 @@ void pci_device_reset(PCIDevice *dev)
>  }
>  
>  /*
> + * This function is called on #RST and FLR.
> + * FLR if PCI_EXP_DEVCTL_BCR_FLR is set
> + */
> +void pci_device_reset(PCIDevice *dev)
> +{
> +    qdev_reset_all(&dev->qdev);
> +    pci_do_device_reset(dev);
> +}
> +
> +/*
>   * Trigger pci bus reset under a given bus.
> - * To be called on RST# assert.
> + * Called via qbus_reset_all on RST# assert, after the devices
> + * have been reset qdev_reset_all-ed already.
>   */
> -static int pcibus_reset(BusState *qbus)
> +static void pcibus_reset(BusState *qbus)
>  {
>      PCIBus *bus = DO_UPCAST(PCIBus, qbus, qbus);
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
>          if (bus->devices[i]) {
> -            pci_device_reset(bus->devices[i]);
> +            pci_do_device_reset(bus->devices[i]);
>          }
>      }
>  
>      for (i = 0; i < bus->nirq; i++) {
>          assert(bus->irq_count[i] == 0);
>      }
> -
> -    /* topology traverse is done by pci_bus_reset().
> -       Tell qbus/qdev walker not to traverse the tree */
> -    return 1;
>  }
>  
>  static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 21ea2c6..409fd71 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -178,7 +178,7 @@ struct BusClass {
>       * bindings can be found at http://playground.sun.com/1275/bindings/.
>       */
>      char *(*get_fw_dev_path)(DeviceState *dev);
> -    int (*reset)(BusState *bus);
> +    void (*reset)(BusState *bus);
>      /* maximum devices allowed on the bus, 0: no limit. */
>      int max_dev;
>  };
> -- 
> 1.7.1


This seems to break a bunch of stuff:

/scm/qemu/hw/s390x/virtio-ccw.c: In function
‘virtual_css_bus_class_init’:
/scm/qemu/hw/s390x/virtio-ccw.c:47:14: error: assignment from
incompatible pointer type [-Werror]
     k->reset = virtual_css_bus_reset;
              ^