Re: [PATCH v8 08/11] hw/core: deprecate old reset functions and introduce new ones

[Philippe Mathieu-Daudé]

On 1/23/20 2:28 PM, Damien Hedde wrote:
> Deprecate device_legacy_reset(), qdev_reset_all() and
> qbus_reset_all() to be replaced by new functions
> device_cold_reset() and bus_cold_reset() which uses resettable API.
> 
> Also introduce resettable_cold_reset_fn() which may be used as a
> replacement for qdev_reset_all_fn and qbus_reset_all_fn().
> 
> Following patches will be needed to look at legacy reset call sites
> and switch to resettable api. The legacy functions will be removed
> when unused.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/qdev-core.h  | 27 +++++++++++++++++++++++++++
>  include/hw/resettable.h |  9 +++++++++
>  hw/core/bus.c           |  5 +++++
>  hw/core/qdev.c          |  5 +++++
>  hw/core/resettable.c    |  5 +++++
>  5 files changed, 51 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1b4b420617..b84fcc32bf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev,
>                         qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
>                         void *opaque);
>  
> +/**
> + * @qdev_reset_all:
> + * Reset @dev. See @qbus_reset_all() for more details.
> + *
> + * Note: This function is deprecated and will be removed when it becomes 
> unused.
> + * Please use device_cold_reset() now.
> + */
>  void qdev_reset_all(DeviceState *dev);
>  void qdev_reset_all_fn(void *opaque);
>  
> @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque);
>   * 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.
> + *
> + * Note: This function is deprecated and will be removed when it becomes 
> unused.
> + * Please use bus_cold_reset() now.

Some time passed, so looking at this with some retrospective.

If there is an effort to introduce a new API replacing another one,
we should try convert all the uses of the old API to the new one,
instead of declaring it legacy.

Declare an API legacy/deprecated should be the last resort if there
is no way to remove it. I'd recommend to move the deprecated/legacy
declarations in a separate header, with the '_legacy' suffix.

Else:

1/ we never finish API conversions,
2/ the new API might not be ready for all the legacy API use cases,
3/ we end up having to maintain 2 different APIs.


So the recommendation is to use bus_cold_reset(), but it isn't
used anywhere...:

$ git grep bus_cold_reset
docs/devel/reset.rst:64:- ``bus_cold_reset()``
hw/core/bus.c:73:void bus_cold_reset(BusState *bus)
include/hw/qdev-core.h:715: * Please use bus_cold_reset() now.
include/hw/qdev-core.h:728: * bus_cold_reset:
include/hw/qdev-core.h:733:void bus_cold_reset(BusState *bus);

IMHO we shouldn't add new public prototypes without callers.

I see it is similar to device_cold_reset(), but TBH I'm scared
to be the first one using it.

Regards,

Phil.

>   */
>  void qbus_reset_all(BusState *bus);
>  void qbus_reset_all_fn(void *opaque);
>  
> +/**
> + * device_cold_reset:
> + * Reset device @dev and perform a recursive processing using the resettable
> + * interface. It triggers a RESET_TYPE_COLD.
> + */
> +void device_cold_reset(DeviceState *dev);
> +
> +/**
> + * bus_cold_reset:
> + *
> + * Reset bus @bus and perform a recursive processing using the resettable
> + * interface. It triggers a RESET_TYPE_COLD.
> + */
> +void bus_cold_reset(BusState *bus);