Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde <address@hidden> wrote:
>
> Deprecate old reset apis and make them use the new one while they
> are still used somewhere.
>
> Signed-off-by: Damien Hedde <address@hidden>
> ---
>  hw/core/qdev.c         | 22 +++-------------------
>  include/hw/qdev-core.h | 28 ++++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 559ced070d..e9e5f2d5f9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void 
> (*func)(Object *))
>      }
>  }
>
> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> -{
> -    device_legacy_reset(dev);
> -
> -    return 0;
> -}
> -
> -static int qbus_reset_one(BusState *bus, void *opaque)
> -{
> -    BusClass *bc = BUS_GET_CLASS(bus);
> -    if (bc->reset) {
> -        bc->reset(bus);
> -    }
> -    return 0;
> -}
> -
>  void qdev_reset_all(DeviceState *dev)
>  {
> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, 
> NULL);
> +    device_reset(dev, false);
>  }
>
>  void qdev_reset_all_fn(void *opaque)
> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
>
>  void qbus_reset_all(BusState *bus)
>  {
> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, 
> NULL);
> +    bus_reset(bus, false);
>  }
>
>  void qbus_reset_all_fn(void *opaque)
> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>              }
>          }
>          if (dev->hotplugged) {
> -            device_legacy_reset(dev);
> +            device_reset(dev, true);
>          }
>          dev->pending_deleted_event = false;

The other changes here don't change the semantics, but this
one does -- previously we were only resetting this specific
device and now we're resetting both the device and its children.
I think it belongs as its own patch in with the other
"remove device_legacy_reset call" patches.

>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index eeb75611c8..1670ae41bb 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -109,6 +109,11 @@ typedef struct DeviceClass {
>      bool hotpluggable;
>
>      /* callbacks */
> +    /*
> +     * Reset method here is deprecated and replaced by methods in the
> +     * resettable class interface to implement a multi-phase reset.
> +     * TODO: remove once every reset callback is unused
> +     */
>      DeviceReset reset;
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
> @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus);
>   */
>  bool bus_is_reset_cold(BusState *bus);
>
> -void qdev_reset_all(DeviceState *dev);
> -void qdev_reset_all_fn(void *opaque);
> -
>  /**
> - * @qbus_reset_all:
> - * @bus: Bus to be reset.
> + * qbus/qdev_reset_all:
> + * @bus/dev: Bus/Device to be reset.
>   *
> - * Reset @bus and perform a bus-level ("hard") reset of all devices connected
> + * Reset @bus/dev and perform a bus-level reset of all devices/buses 
> connected
>   * to it, including recursive processing of all buses below @bus itself.  A
>   * hard reset means that qbus_reset_all will reset all state of the device.
>   * For PCI devices, for example, this will include the base address registers
>   * or configuration space.
> + *
> + * Theses functions are deprecated, please use device/bus_reset or

"these"

> + * resettable_reset_* instead
> + * TODO: remove them when all occurence are removed

"occurrences" (two 'r's, plural with 's'), here and below

>   */

The comment here says that qbus_reset_all() does a "hard" reset,
which presumably is equivalent to a 'cold' reset, but the
code in our new implementation passes cold = false.

> +void qdev_reset_all(DeviceState *dev);
> +void qdev_reset_all_fn(void *opaque);
>  void qbus_reset_all(BusState *bus);
>  void qbus_reset_all_fn(void *opaque);
>
> @@ -489,9 +497,17 @@ void qdev_machine_init(void);
>   * device_legacy_reset:
>   *
>   * Reset a single device (by calling the reset method).
> + *
> + * This function is deprecated, please use device_reset() instead.
> + * TODO: remove the function when all occurences are removed.
>   */
>  void device_legacy_reset(DeviceState *dev);
>
> +/**
> + * device_class_set_parent_reset:
> + * TODO: remove the function when DeviceClass's reset method
> + * is not used anymore.
> + */
>  void device_class_set_parent_reset(DeviceClass *dc,
>                                     DeviceReset dev_reset,
>                                     DeviceReset *parent_reset);
> --
> 2.22.0

thanks
-- PMM