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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]

On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> Deprecate old reset apis and make them use the new one while they
> are still used somewhere.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  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);

So.. is this change in the device_reset() signature really necessary?
Even if there are compelling reasons to handle warm reset in the new
API, that doesn't been you need to change device_reset() itself from
its established meaning of a cold (i.e. as per power cycle) reset.
Warm resets are generally called in rather more specific circumstances
(often under guest software direction) so it seems likely that users
would want to engage with the new reset API directly.  Or we could
just create a device_warm_reset() wrapper.  That would also avoid the
bare boolean parameter, which is not great for readability (you have
to look up the signature to have any idea what it means).

>          }
>          dev->pending_deleted_event = false;
>  
> 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
> + * resettable_reset_* instead
> + * TODO: remove them when all occurence are removed
>   */
> +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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]